Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impl EncodeLabelValue for Option<T> #137

Closed
wants to merge 1 commit into from

Conversation

emschwartz
Copy link
Contributor

@emschwartz emschwartz commented Apr 21, 2023

Alternatively, the EncodeLabelSet derive macro could add code that checks whether the value of the Option and only encodes the Some value

Signed-off-by: Evan Schwartz <3262610+emschwartz@users.noreply.github.com>
@drbrain
Copy link

drbrain commented Apr 21, 2023

I've implemented an equivalent of this where I use this crate to allow metric_name{option_label=""}, It'll be great to have this as a built-in.

For derived EncodeLabelSet I think this could produce either metric_name{} (a None means "no label at all") or metric_name{option_label=""} (None means "label with empty value") depending on if you wanted a label to be joinable with other metrics or not. I've done some work in #136 which might make it easier to configure the derive macro to choose the desired behavior.

@mxinden
Copy link
Member

mxinden commented May 9, 2023

Can you expand on the use-case for a metric with a label key but an empty string as label value? Ideally the use-case and the corresponding Prometheus query?

Thank you for your upstream work!

@emschwartz
Copy link
Contributor Author

I'm working on the Autometrics project, which generates metrics from function names.

We have a feature where you can define Service-Level Objectives in your code and then attach them to metrics. This works by adding some "objective"-related labels to the metrics produced by each function. My label set thus contains some values that will always be there and some that are optional.

To be honest, it doesn't matter to me whether they show up as label keys with empty values or just aren't attached at all (and I'm not sure if it makes a difference to Prometheus as querying label_key="" apparently has the same effect as leaving off that part of the query). It might make slightly more sense to leave off the label keys entirely.

Here is a detailed write-up of the SLO feature in autometrics-rs: https://fiberplane.com/blog/autometrics-rs-0-3-defining-service-level-objectives-in-rust-source-code

@mxinden
Copy link
Member

mxinden commented May 14, 2023

Thanks @emschwartz and @drbrain for the additional input.

and I'm not sure if it makes a difference to Prometheus as querying label_key="" apparently has the same effect as leaving off that part of the query

Backing this up:

Empty label values SHOULD be treated as if the label was not present.

https://github.com/OpenObservability/OpenMetrics/blob/main/specification/OpenMetrics.md#label

With that, I am fine either way. In case neither of you objects to this pull request as is, would you @emschwartz mind bumping the minor version and adding a changelog entry?

@mxinden
Copy link
Member

mxinden commented May 16, 2023

Released through #141 as v0.21.1.

@mxinden mxinden closed this May 16, 2023
mxinden added a commit to mxinden/rust-libp2p that referenced this pull request May 16, 2023
@emschwartz
Copy link
Contributor Author

Thanks @mxinden!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants