client_rust icon indicating copy to clipboard operation
client_rust copied to clipboard

Using `()` as label set generates invalid format

Open Victor-N-Suadicani opened this issue 2 years ago • 7 comments

I tried using () as the label set for a family, like Family<(), Histogram, ...>. Unfortunately this resulted in the following line in my metrics output:

histogram_name_bucket{,le="1.0"} 44

This results in a expected label name, got "COMMA" error when Prometheus tries to collect the metrics.

I would expect this to "just work" and just not insert any label (i.e. basically not have the leading comma in {,le="1.0"})

Victor-N-Suadicani avatar Jul 27 '22 09:07 Victor-N-Suadicani

I tried using () as the label set for a family, like Family<(), Histogram, ...>.

What is your reasoning for not using a plain Histogram?

mxinden avatar Jul 27 '22 09:07 mxinden

I've built a small wrapping library around client_rust for internal use at my job, to streamline and standardize our usage. Basically I'm not using a plain Histogram as this is using some generic code which allows you to use any label type you want. I figured that in the case you didn't want any labels, you could just use () - but that turned out not to work :sweat_smile:

Victor-N-Suadicani avatar Jul 27 '22 10:07 Victor-N-Suadicani

One solution could be to only add a , here in case the previous label has a length larger than 0.

https://github.com/prometheus/client_rust/blob/6b7a3458b47a406d15f2ae0a678c611969c4645d/src/encoding/text.rs#L277-L285

mxinden avatar Jul 27 '22 10:07 mxinden

Tangentially related, but I noticed that &str also implements encode - does that assume that you must have the &str be equal to "label_name=\"label_value\"? I'm assuming that if you just set it equal to "foobar" then prometheus will complain that the label has no value (must labels have values?).

But yea that might be one solution I suppose. Alternatively, maybe it should be the responsibility of the Encode implementation to insert the comma?

Victor-N-Suadicani avatar Jul 27 '22 10:07 Victor-N-Suadicani

Tangentially related, but I noticed that &str also implements encode - does that assume that you must have the &str be equal to "label_name=\"label_value\"? I'm assuming that if you just set it equal to "foobar" then prometheus will complain that the label has no value (must labels have values?).

That assumption is correct.

But yea that might be one solution I suppose. Alternatively, maybe it should be the responsibility of the Encode implementation to insert the comma?

That would as well be a viable solution.

mxinden avatar Jul 29 '22 06:07 mxinden

I honestly find it a bit surprising that &str implements Encode in that case. I guess it's the same with f32 and the other primitives that implement Encode. encode should at least return Err when it doesn't match the expected format, I would say.

I feel like (&str, &str) should implement Encode but not &str. Currently the tuple impl requires the items to also implement Encode - maybe it should just implement Encode for (K, V) where K: Display, V: Display?

Victor-N-Suadicani avatar Jul 29 '22 07:07 Victor-N-Suadicani

I agree. Today I am using Encode to encode the following:

  • Label pair
  • Label key
  • Label value
  • Metric value

Each of these use-cases should probably use their own EncodeXXX trait instead of a single shared one.

mxinden avatar Jul 29 '22 07:07 mxinden