protobuf: Fix label encoding

[ackintosh]

Fixes #124

[ackintosh]

I have added a test case to reproduce #124. I was looking into this but couldn't find how to fix. 😓

[divagant-martian]

Code is a bit hard to get due to lack of documentation / generic names. Based on the text::MetricEncoder field family_labels, I assume the protobuf::MetricEncoder labels are also meant to be shared labels.

So if I add this diff

diff --git a/src/encoding/protobuf.rs b/src/encoding/protobuf.rs
index f40338b..d1f58b2 100644
--- a/src/encoding/protobuf.rs
+++ b/src/encoding/protobuf.rs
@@ -193,6 +193,7 @@ impl<'a> MetricEncoder<'a> {
         &'b mut self,
         label_set: &S,
     ) -> Result<MetricEncoder<'b>, std::fmt::Error> {
+        self.labels.clear();
         label_set.encode(
             LabelSetEncoder {
                 labels: self.labels,
@@ -563,7 +564,7 @@ mod tests {
         family
             .get_or_create(&vec![
                 ("method".to_string(), "GET".to_string()),
-                ("status".to_string(), "200".to_string()),
+                ("status".to_string(), "404".to_string()),
             ])
             .inc();
 
@@ -591,7 +592,7 @@ mod tests {
         assert_eq!("method", metric.labels[0].name);
         assert_eq!("GET", metric.labels[0].value);
         assert_eq!("status", metric.labels[1].name);
-        assert_eq!("200", metric.labels[1].value);
+        assert_eq!("404", metric.labels[1].value);
 
         match extract_metric_point_value(&metric_set) {
             openmetrics_data_model::metric_point::Value::CounterValue(value) => {

the test you added gets the correct metric labels, but the test encoding::protobuf::tests::encode_counter_family_with_prefix_with_label misses the prefix label.

So I think the problem is that the "family" labels are being populated with normal labels somewhere, can't point to where yet

[ackintosh]

I'm not fully sure if I understand current implementation, we should separate labels to e.g. const_labels and family_labels like text encoding? @mxinden How do you think? 🙏

[mxinden]

@divagant-martian proposal above, namely to clear() the self.labels on a call to encode_family sounds good to me. This is inline with what the text encoder does, namely to override any existing family labels:

client_rust/src/encoding/text.rs

Line 210 in 9141c11

family_labels: Some(label_set),

@ackintosh would you mind applying the suggestion from @divagant-martian?

[ackintosh]

I have applied the suggestion but the encode_counter_family_with_prefix_with_label test fails. 🤔

https://github.com/prometheus/client_rust/actions/runs/3958633415/jobs/6780437752#step:9:575

---- encoding::protobuf::tests::encode_counter_family_with_prefix_with_label stdout ----
thread 'encoding::protobuf::tests::encode_counter_family_with_prefix_with_label' panicked at 'assertion failed: (left == right)
left: 3,
right: 2', src/encoding/protobuf.rs:645:9

[divagant-martian]

I said this test would fail in my comment

the test you added gets the correct metric labels, but the test encoding::protobuf::tests::encode_counter_family_with_prefix_with_label misses the prefix label.

So I think the problem is that the "family" labels are being populated with normal labels somewhere, can't point to where yet

So what I think should happen is that this field should be empty in the new test since none of the labels are shared, but populated with the prefix label in the second test. Still don't know where this change should be done tho

[ackintosh]

@mxinden @divagant-martian The bug should have been fixed, please have a look. 🙏

[divagant-martian]

Thanks @ackintosh I'm not sure if I'm the right person to review, but the test look legit and they run alright, so that's great!

Whether done here or a different PR I think it would be valuable for future us to add documentation to the protofub related structs and fields. In the context of this PR, in particular the labels fields

[ackintosh]

I have added a doc for encoding::protobuf::MetricEncoder for now. 🙂

[mxinden]

Thanks for the help here @ackintosh and @divagant-martian!

[ackintosh]

@mxinden Could you please cut a new release? I'm thinking of including this fix in rust-libp2p.

[divagant-martian]

friendly ping @mxinden for whenever you have the time or if you are waiting on important changes for a release 🖤

[mxinden]

Sorry for the delay. Tagged and published with #133.