Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: correctly handle empty family labels #175

Closed
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions examples/custom-metric.rs
Original file line number Diff line number Diff line change
@@ -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;

Expand All @@ -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 {
Expand Down
6 changes: 5 additions & 1 deletion src/encoding.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>),
Expand Down Expand Up @@ -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(())
}
Expand Down
69 changes: 60 additions & 9 deletions src/encoding/text.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand All @@ -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 {
Expand Down Expand Up @@ -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()?;
Expand Down Expand Up @@ -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("}")?;
Expand Down Expand Up @@ -710,6 +735,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() {
Expand Down Expand Up @@ -899,6 +925,31 @@ mod tests {
parse_with_python_client(encoded);
}

#[test]
fn encode_histogram_family_with_empty_struct_family_labels() {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added this second test to give a more realistic use case of 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());
Comment on lines +931 to +933
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


#[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();
Expand Down
6 changes: 3 additions & 3 deletions src/metrics/counter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
4 changes: 2 additions & 2 deletions src/metrics/histogram.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -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 {
Expand Down