From cdfda5c27908fce09be57b243c6f3d613e56886b Mon Sep 17 00:00:00 2001 From: Tyler Levine Date: Mon, 30 Oct 2023 14:31:14 -0700 Subject: [PATCH 1/3] fix(encoding): correctly handle empty family labels Signed-off-by: Tyler Levine --- src/encoding/text.rs | 58 +++++++++++++++++++++++++++++++++++++++++--- 1 file changed, 54 insertions(+), 4 deletions(-) diff --git a/src/encoding/text.rs b/src/encoding/text.rs index 955396a..d82fdd6 100644 --- a/src/encoding/text.rs +++ b/src/encoding/text.rs @@ -513,11 +513,15 @@ impl<'a> MetricEncoder<'a> { } if let Some(labels) = &self.family_labels { - 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())?; } self.writer.write_str("}")?; @@ -710,6 +714,7 @@ mod tests { use pyo3::{prelude::*, types::PyModule}; use std::borrow::Cow; use std::sync::atomic::{AtomicI32, AtomicU32}; + use std::fmt::Error; #[test] fn encode_counter() { @@ -899,6 +904,51 @@ mod tests { parse_with_python_client(encoded); } + #[test] + fn encode_histogram_family_with_empty_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()); + + family + .get_or_create(&()) + .observe(1.0); + + let mut encoded = String::new(); + + encode(&mut encoded, ®istry).unwrap(); + + 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, ®istry).unwrap(); + + parse_with_python_client(encoded); + } + #[test] fn encode_histogram_with_exemplars() { let mut registry = Registry::default(); From 6d4f123a1bc8d88a9fd2f005117a00e17c26bc92 Mon Sep 17 00:00:00 2001 From: Tyler Levine Date: Mon, 8 Jan 2024 13:04:43 -0800 Subject: [PATCH 2/3] refactor(encoding): replace `EncodeLabelSet` impl for `()` with private uninhabited type Signed-off-by: Tyler Levine --- src/encoding.rs | 5 ++++- src/encoding/text.rs | 32 ++++++-------------------------- src/metrics/counter.rs | 6 +++--- src/metrics/histogram.rs | 4 ++-- 4 files changed, 15 insertions(+), 32 deletions(-) diff --git a/src/encoding.rs b/src/encoding.rs index c9c9a83..8f8c7de 100644 --- a/src/encoding.rs +++ b/src/encoding.rs @@ -247,6 +247,9 @@ pub trait EncodeLabel { #[derive(Debug)] pub struct LabelEncoder<'a>(LabelEncoderInner<'a>); +/// Uninhabited type to represent the lack of a label set for a metric +pub(crate) enum NoLabelSet {} + #[derive(Debug)] enum LabelEncoderInner<'a> { Text(text::LabelEncoder<'a>), @@ -352,7 +355,7 @@ impl EncodeLabelSet for Vec { } } -impl EncodeLabelSet for () { +impl EncodeLabelSet for NoLabelSet { fn encode(&self, _encoder: LabelSetEncoder) -> Result<(), std::fmt::Error> { Ok(()) } diff --git a/src/encoding/text.rs b/src/encoding/text.rs index d82fdd6..5d8b991 100644 --- a/src/encoding/text.rs +++ b/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::(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::(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::(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::(None)?; self.writer.write_str(" ")?; self.writer.write_str(itoa::Buffer::new().format(count))?; self.newline()?; @@ -904,24 +904,6 @@ mod tests { parse_with_python_client(encoded); } - #[test] - fn encode_histogram_family_with_empty_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()); - - family - .get_or_create(&()) - .observe(1.0); - - let mut encoded = String::new(); - - encode(&mut encoded, ®istry).unwrap(); - - parse_with_python_client(encoded); - } - #[test] fn encode_histogram_family_with_empty_struct_family_labels() { let mut registry = Registry::default(); @@ -938,9 +920,7 @@ mod tests { } } - family - .get_or_create(&EmptyLabels {}) - .observe(1.0); + family.get_or_create(&EmptyLabels {}).observe(1.0); let mut encoded = String::new(); diff --git a/src/metrics/counter.rs b/src/metrics/counter.rs index d1fee30..7016c36 100644 --- a/src/metrics/counter.rs +++ b/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, { fn encode(&self, mut encoder: MetricEncoder) -> Result<(), std::fmt::Error> { - encoder.encode_counter::<(), _, u64>(&self.get(), None) + encoder.encode_counter::(&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::(&self.value, None) } fn metric_type(&self) -> MetricType { diff --git a/src/metrics/histogram.rs b/src/metrics/histogram.rs index 66f4496..1e418ad 100644 --- a/src/metrics/histogram.rs +++ b/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 Result<(), std::fmt::Error> { let (sum, count, buckets) = self.get(); - encoder.encode_histogram::<()>(sum, count, &buckets, None) + encoder.encode_histogram::(sum, count, &buckets, None) } fn metric_type(&self) -> MetricType { From 8f1b569f9cf30b8849aab9b290026827a6ee73c0 Mon Sep 17 00:00:00 2001 From: Tyler Levine Date: Tue, 30 Jul 2024 12:47:35 -0700 Subject: [PATCH 3/3] chore: lazily add a comma if needed before writing family labels Signed-off-by: Tyler Levine --- examples/custom-metric.rs | 4 ++-- src/encoding.rs | 3 ++- src/encoding/text.rs | 35 ++++++++++++++++++++++++++++------- 3 files changed, 32 insertions(+), 10 deletions(-) diff --git a/examples/custom-metric.rs b/examples/custom-metric.rs index 9ad17a5..a61ff8a 100644 --- a/examples/custom-metric.rs +++ b/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::(), None) + encoder.encode_counter::(&rand::random::(), None) } fn metric_type(&self) -> prometheus_client::metrics::MetricType { diff --git a/src/encoding.rs b/src/encoding.rs index 8f8c7de..c644f82 100644 --- a/src/encoding.rs +++ b/src/encoding.rs @@ -248,7 +248,8 @@ pub trait EncodeLabel { pub struct LabelEncoder<'a>(LabelEncoderInner<'a>); /// Uninhabited type to represent the lack of a label set for a metric -pub(crate) enum NoLabelSet {} +#[derive(Debug)] +pub enum NoLabelSet {} #[derive(Debug)] enum LabelEncoderInner<'a> { diff --git a/src/encoding/text.rs b/src/encoding/text.rs index 5d8b991..8af67a5 100644 --- a/src/encoding/text.rs +++ b/src/encoding/text.rs @@ -512,18 +512,39 @@ impl<'a> MetricEncoder<'a> { additional_labels.encode(LabelSetEncoder::new(self.writer).into())?; } - if let Some(labels) = &self.family_labels { - let mut string_writer = String::new(); - labels.encode(LabelSetEncoder::new(&mut string_writer).into())?; + /// 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, + } - if !string_writer.is_empty() { - if !self.const_labels.is_empty() || additional_labels.is_some() { - self.writer.write_str(",")?; + 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(string_writer.as_str())?; + self.writer.write_str(s) } } + 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("}")?; Ok(())