fix: correctly handle empty family labels #175

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 <[email protected]>

…te uninhabited type Signed-off-by: Tyler Levine <[email protected]>

Signed-off-by: Tyler Levine <[email protected]>

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

Open

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 <[email protected]>

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

6d4f123

…te uninhabited type Signed-off-by: Tyler Levine <[email protected]>

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

8f1b569

Signed-off-by: Tyler Levine <[email protected]>

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: correctly handle empty family labels #175

fix: correctly handle empty family labels #175

mxinden commented Oct 31, 2023

mxinden left a comment

mxinden Oct 31, 2023

tylerlevine Oct 31, 2023

mxinden Oct 31, 2023

tylerlevine Oct 31, 2023 •

edited

Loading

tylerlevine Nov 1, 2023

mxinden Dec 5, 2023

tylerlevine Dec 18, 2023

mxinden Jan 4, 2024

tylerlevine Jan 8, 2024

tylerlevine Oct 31, 2023

mxinden Jan 9, 2024

tylerlevine Jan 31, 2024

mxinden commented Jul 17, 2024

tylerlevine commented Aug 5, 2024

mxinden left a comment

mxinden commented Sep 6, 2024

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

fix: correctly handle empty family labels #175

fix: correctly handle empty family labels #175

Conversation

mxinden commented Oct 31, 2023

mxinden left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tylerlevine Oct 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mxinden commented Jul 17, 2024

tylerlevine commented Aug 5, 2024

mxinden left a comment

Choose a reason for hiding this comment

mxinden commented Sep 6, 2024

tylerlevine Oct 31, 2023 •

edited

Loading