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

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

Author: mxinden

CHANGELOG.md

@@ -28,6 +28,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 [PR 216]: https://github.com/prometheus/client_rust/pull/216
 [PR 217]: https://github.com/prometheus/client_rust/pull/217
 
+### Fixed
+
+- Don't prepend `,` when encoding empty family label set.
+  See [PR 175].
+
+[PR 175]: https://github.com/prometheus/client_rust/pull/175
+
 ## [0.22.3]
 
 ### Added

examples/custom-metric.rs

@@ -1,4 +1,4 @@
-use prometheus_client::encoding::{text::encode, EncodeMetric, MetricEncoder};
+use prometheus_client::encoding::{text::encode, EncodeMetric, MetricEncoder, NoLabelSet};
 use prometheus_client::metrics::MetricType;
 use prometheus_client::registry::Registry;
 
@@ -20,7 +20,7 @@ impl EncodeMetric for MyCustomMetric {
         // E.g. every CPU cycle spend in this method delays the response send to
         // the Prometheus server.
 
-        encoder.encode_counter::<(), _, u64>(&rand::random::<u64>(), None)
+        encoder.encode_counter::<NoLabelSet, _, u64>(&rand::random::<u64>(), None)
     }
 
     fn metric_type(&self) -> prometheus_client::metrics::MetricType {

src/encoding.rs

@@ -247,6 +247,10 @@ pub trait EncodeLabel {
 #[derive(Debug)]
 pub struct LabelEncoder<'a>(LabelEncoderInner<'a>);
 
+/// Uninhabited type to represent the lack of a label set for a metric
+#[derive(Debug)]
+pub enum NoLabelSet {}
+
 #[derive(Debug)]
 enum LabelEncoderInner<'a> {
     Text(text::LabelEncoder<'a>),
@@ -352,7 +356,7 @@ impl<T: EncodeLabel> EncodeLabelSet for Vec<T> {
     }
 }
 
-impl EncodeLabelSet for () {
+impl EncodeLabelSet for NoLabelSet {
     fn encode(&self, _encoder: LabelSetEncoder) -> Result<(), std::fmt::Error> {
         Ok(())
     }

src/encoding/text.rs

@@ -37,7 +37,7 @@
 //! assert_eq!(expected_msg, buffer);
 //! ```
 
-use crate::encoding::{EncodeExemplarValue, EncodeLabelSet};
+use crate::encoding::{EncodeExemplarValue, EncodeLabelSet, NoLabelSet};
 use crate::metrics::exemplar::Exemplar;
 use crate::metrics::MetricType;
 use crate::registry::{Prefix, Registry, Unit};
@@ -324,7 +324,7 @@ impl<'a> MetricEncoder<'a> {
 
         self.write_suffix("total")?;
 
-        self.encode_labels::<()>(None)?;
+        self.encode_labels::<NoLabelSet>(None)?;
 
         v.encode(
             &mut CounterValueEncoder {
@@ -348,7 +348,7 @@ impl<'a> MetricEncoder<'a> {
     ) -> Result<(), std::fmt::Error> {
         self.write_prefix_name_unit()?;
 
-        self.encode_labels::<()>(None)?;
+        self.encode_labels::<NoLabelSet>(None)?;
 
         v.encode(
             &mut GaugeValueEncoder {
@@ -404,14 +404,14 @@ impl<'a> MetricEncoder<'a> {
     ) -> Result<(), std::fmt::Error> {
         self.write_prefix_name_unit()?;
         self.write_suffix("sum")?;
-        self.encode_labels::<()>(None)?;
+        self.encode_labels::<NoLabelSet>(None)?;
         self.writer.write_str(" ")?;
         self.writer.write_str(dtoa::Buffer::new().format(sum))?;
         self.newline()?;
 
         self.write_prefix_name_unit()?;
         self.write_suffix("count")?;
-        self.encode_labels::<()>(None)?;
+        self.encode_labels::<NoLabelSet>(None)?;
         self.writer.write_str(" ")?;
         self.writer.write_str(itoa::Buffer::new().format(count))?;
         self.newline()?;
@@ -512,12 +512,37 @@ impl<'a> MetricEncoder<'a> {
             additional_labels.encode(LabelSetEncoder::new(self.writer).into())?;
         }
 
-        if let Some(labels) = &self.family_labels {
-            if !self.const_labels.is_empty() || additional_labels.is_some() {
-                self.writer.write_str(",")?;
+        /// Writer impl which prepends a comma on the first call to write output to the wrapped writer
+        struct CommaPrependingWriter<'a> {
+            writer: &'a mut dyn Write,
+            should_prepend: bool,
+        }
+
+        impl Write for CommaPrependingWriter<'_> {
+            fn write_str(&mut self, s: &str) -> std::fmt::Result {
+                if self.should_prepend {
+                    self.writer.write_char(',')?;
+                    self.should_prepend = false;
+                }
+                self.writer.write_str(s)
             }
+        }
 
-            labels.encode(LabelSetEncoder::new(self.writer).into())?;
+        if let Some(labels) = self.family_labels {
+            // if const labels or additional labels have been written, a comma must be prepended before writing the family labels.
+            // However, it could be the case that the family labels are `Some` and yet empty, so the comma should _only_
+            // be prepended if one of the `Write` methods are actually called when attempting to write the family labels.
+            // Therefore, wrap the writer on `Self` with a CommaPrependingWriter if other labels have been written and
+            // there may be a need to prepend an extra comma before writing additional labels.
+            if !self.const_labels.is_empty() || additional_labels.is_some() {
+                let mut writer = CommaPrependingWriter {
+                    writer: self.writer,
+                    should_prepend: true,
+                };
+                labels.encode(LabelSetEncoder::new(&mut writer).into())?;
+            } else {
+                labels.encode(LabelSetEncoder::new(self.writer).into())?;
+            };
         }
 
         self.writer.write_str("}")?;
@@ -709,6 +734,7 @@ mod tests {
     use crate::metrics::{counter::Counter, exemplar::CounterWithExemplar};
     use pyo3::{prelude::*, types::PyModule};
     use std::borrow::Cow;
+    use std::fmt::Error;
     use std::sync::atomic::{AtomicI32, AtomicU32};
 
     #[test]
@@ -899,6 +925,31 @@ mod tests {
         parse_with_python_client(encoded);
     }
 
+    #[test]
+    fn encode_histogram_family_with_empty_struct_family_labels() {
+        let mut registry = Registry::default();
+        let family =
+            Family::new_with_constructor(|| Histogram::new(exponential_buckets(1.0, 2.0, 10)));
+        registry.register("my_histogram", "My histogram", family.clone());
+
+        #[derive(Eq, PartialEq, Hash, Debug, Clone)]
+        struct EmptyLabels {}
+
+        impl EncodeLabelSet for EmptyLabels {
+            fn encode(&self, _encoder: crate::encoding::LabelSetEncoder) -> Result<(), Error> {
+                Ok(())
+            }
+        }
+
+        family.get_or_create(&EmptyLabels {}).observe(1.0);
+
+        let mut encoded = String::new();
+
+        encode(&mut encoded, &registry).unwrap();
+
+        parse_with_python_client(encoded);
+    }
+
     #[test]
     fn encode_histogram_with_exemplars() {
         let mut registry = Registry::default();

src/metrics/counter.rs

@@ -2,7 +2,7 @@
 //!
 //! See [`Counter`] for details.
 
-use crate::encoding::{EncodeMetric, MetricEncoder};
+use crate::encoding::{EncodeMetric, MetricEncoder, NoLabelSet};
 
 use super::{MetricType, TypedMetric};
 use std::marker::PhantomData;
@@ -204,7 +204,7 @@ where
     A: Atomic<N>,
 {
     fn encode(&self, mut encoder: MetricEncoder) -> Result<(), std::fmt::Error> {
-        encoder.encode_counter::<(), _, u64>(&self.get(), None)
+        encoder.encode_counter::<NoLabelSet, _, u64>(&self.get(), None)
     }
 
     fn metric_type(&self) -> MetricType {
@@ -236,7 +236,7 @@ where
     N: crate::encoding::EncodeCounterValue,
 {
     fn encode(&self, mut encoder: MetricEncoder) -> Result<(), std::fmt::Error> {
-        encoder.encode_counter::<(), _, u64>(&self.value, None)
+        encoder.encode_counter::<NoLabelSet, _, u64>(&self.value, None)
     }
 
     fn metric_type(&self) -> MetricType {

src/metrics/histogram.rs

@@ -2,7 +2,7 @@
 //!
 //! See [`Histogram`] for details.
 
-use crate::encoding::{EncodeMetric, MetricEncoder};
+use crate::encoding::{EncodeMetric, MetricEncoder, NoLabelSet};
 
 use super::{MetricType, TypedMetric};
 use parking_lot::{MappedRwLockReadGuard, RwLock, RwLockReadGuard};
@@ -133,7 +133,7 @@ pub fn linear_buckets(start: f64, width: f64, length: u16) -> impl Iterator<Item
 impl EncodeMetric for Histogram {
     fn encode(&self, mut encoder: MetricEncoder) -> Result<(), std::fmt::Error> {
         let (sum, count, buckets) = self.get();
-        encoder.encode_histogram::<()>(sum, count, &buckets, None)
+        encoder.encode_histogram::<NoLabelSet>(sum, count, &buckets, None)
     }
 
     fn metric_type(&self) -> MetricType {