fix: correctly handle empty family labels by mxinden · Pull Request #175 · prometheus/client_rust

mxinden · 2023-10-31T13:36:16Z

No description provided.

mxinden

Thank you @tylerlevine for reporting and providing a patch!

mxinden · 2023-10-31T13:38:51Z

src/encoding/text.rs

+        let family =
+            Family::new_with_constructor(|| Histogram::new(exponential_buckets(1.0, 2.0, 10)));
+        registry.register("my_histogram", "My histogram", family.clone());


Just to make sure we are on the same page, you don't need to wrap a Histogram in a Family.

Suggested change

let family =

Family::new_with_constructor(|| Histogram::new(exponential_buckets(1.0, 2.0, 10)));

registry.register("my_histogram", "My histogram", family.clone());

let histogram = Histogram::new(exponential_buckets(1.0, 2.0, 10));

registry.register("my_histogram", "My histogram", historgam.clone());

This should fix the encoding failure, correct?

Ah - this makes more sense now. I didn't realize that the Family wrapper was optional. Using the histogram directly indeed does fix the encoding failure.

mxinden · 2023-10-31T13:39:49Z

src/encoding/text.rs

+        registry.register("my_histogram", "My histogram", family.clone());
+
+        family
+            .get_or_create(&())


In what use-case would you want a Family with a () as a label set?

To remove this footgun, shall we remove the EncodeLabelSet implementation for ()?

client_rust/src/encoding.rs

Lines 352 to 356 in 23c31bd

impl EncodeLabelSet for () {

fn encode(&self, _encoder: LabelSetEncoder) -> Result<(), std::fmt::Error> {

Ok(())

}

}

Now that I understand that the Family wrapper is optional, I agree that using () as the label set doesn't make much sense.

However, when I originally ran into this issue I was using an empty struct with the #[derive(EncodeLabelSet)] attribute, which has the same erroneous behavior. I was intending to go back and add any needed labels to the struct later but noticed this issue before I could do that. I switched to () when I was simplifying my code for posting here.

Removing the EncodeLabelSet implementation from () seems like a good start, but ideally a fix would cover the empty struct case as well.

I took a look at removing the EncodeLabelSet implementation for (), but it looks like that impl is relied on in at least these four places:

client_rust/src/encoding/text.rs

Line 189 in 1f3923a

self.encode_labels::<()>(None)?;

client_rust/src/encoding/text.rs

Line 213 in 1f3923a

self.encode_labels::<()>(None)?;

client_rust/src/encoding/text.rs

Line 269 in 1f3923a

self.encode_labels::<()>(None)?;

client_rust/src/encoding/text.rs

Line 276 in 1f3923a

self.encode_labels::<()>(None)?;

I would assume that we can replace () with a private uninhabited type that implements EncodeLabelSet. Something along the lines of:

enum Never {} impl EncodeLabelSet for Never { // ... } // ... // Then in the above cases: self.encode_labels::<Never>(None)?;

Want to give this a shot @tylerlevine ?

Good idea. Yes, let me give it a try.

Friendly ping @tylerlevine. What do you think of the suggestion above?

@mxinden Looks like that works fine. I cleaned up a bit and updated the PR.

tylerlevine · 2023-10-31T23:54:13Z

src/encoding/text.rs

+    }
+
+    #[test]
+    fn encode_histogram_family_with_empty_struct_family_labels() {


Added this second test to give a more realistic use case of empty family labels.

mxinden · 2024-01-09T15:43:46Z

src/encoding/text.rs

-            if !self.const_labels.is_empty() || additional_labels.is_some() {
-                self.writer.write_str(",")?;
+            let mut string_writer = String::new();
+            labels.encode(LabelSetEncoder::new(&mut string_writer).into())?;
+
+            if !string_writer.is_empty() {
+                if !self.const_labels.is_empty() || additional_labels.is_some() {
+                    self.writer.write_str(",")?;
+                }
+                self.writer.write_str(string_writer.as_str())?;
            }
-
-            labels.encode(LabelSetEncoder::new(self.writer).into())?;


Sorry for only raising this now.

This change is not ideal, as it doubles the allocations required for self.family_labels, i.e. adds unnecessary allocations to the happy path.

I see multiple ways to still remove the bug (i.e. an invalid dangling ,) without the allocation:

Fail to derive EncodeLabelSet on an empty struct. This is a partial fix only as one could still go through e.g. a Vec<(String, String)>.

Wrap self.writer with a thin wrapper, that simply slips in a , in case any of its std::fmt::Write methods are called. In other words that adds a , at very first in case family_labels actually attempts to write anything.

Does the above make sense? I am in favor of (2). What do you think?

Yes, that makes perfect sense to me - thanks for the suggestions. Apologies for my inactivity on this PR, but I'll go for option 2 once I get some time to work on this again. The approach seems pretty straightforward to me.

mxinden · 2024-07-17T07:28:10Z

Friendly ping @tylerlevine.

Signed-off-by: Tyler Levine <tyler.levine@falconx.io>

…te uninhabited type Signed-off-by: Tyler Levine <tyler.levine@falconx.io>

Signed-off-by: Tyler Levine <tyler.levine@falconx.io>

tylerlevine · 2024-08-05T20:35:02Z

Friendly ping @tylerlevine.

Thanks for the ping @mxinden - I've updated the PR to implement suggestion 2 from your earlier comment.

mxinden

Looks good to me. Thank you!

Just missing a changelog entry, otherwise this is good to go.

mxinden · 2024-09-06T10:36:18Z

Merged through #224. Thank you @tylerlevine for the work!

mxinden commented Oct 31, 2023

View reviewed changes

mxinden mentioned this pull request Oct 31, 2023

Histogram with empty family labels creates unparseable metrics output #174

Closed

tylerlevine reviewed Oct 31, 2023

View reviewed changes

tylerlevine force-pushed the histogram-empty-family-labels-fix branch 2 times, most recently from cf8671e to baadd90 Compare January 8, 2024 21:11

mxinden commented Jan 9, 2024

View reviewed changes

tylerlevine force-pushed the histogram-empty-family-labels-fix branch 2 times, most recently from 372291e to f4af9cd Compare August 5, 2024 20:32

tylerlevine added 3 commits August 5, 2024 13:33

fix(encoding): correctly handle empty family labels

cdfda5c

Signed-off-by: Tyler Levine <tyler.levine@falconx.io>

refactor(encoding): replace EncodeLabelSet impl for () with priva…

6d4f123

…te uninhabited type Signed-off-by: Tyler Levine <tyler.levine@falconx.io>

chore: lazily add a comma if needed before writing family labels

8f1b569

Signed-off-by: Tyler Levine <tyler.levine@falconx.io>

tylerlevine force-pushed the histogram-empty-family-labels-fix branch from f4af9cd to 8f1b569 Compare August 5, 2024 20:33

mxinden commented Aug 18, 2024

View reviewed changes

mxinden mentioned this pull request Sep 6, 2024

fix(encoding): correctly handle empty family labels #224

Merged

mxinden closed this Sep 6, 2024

	impl EncodeLabelSet for () {
	fn encode(&self, _encoder: LabelSetEncoder) -> Result<(), std::fmt::Error> {
	Ok(())
	}
	}

Conversation

mxinden commented Oct 31, 2023

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tylerlevine Oct 31, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mxinden commented Jul 17, 2024

Uh oh!

tylerlevine commented Aug 5, 2024

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden commented Sep 6, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tylerlevine Oct 31, 2023 •

edited

Loading