Skip to content

Commit

Permalink
fix(protobuf): Do not share label storage across families (#123)
Browse files Browse the repository at this point in the history 
Signed-off-by: ackintosh <[email protected]>
  • Loading branch information
@ackintosh
ackintosh authored Mar 6, 2023
1 parent 51de116 commit e35e99d
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 15 deletions.
5 changes: 5 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
my_metric{a="42",b="42",unique="42"} 42
```

### Fixed

- Fix label encoding in protobuf feature. See [PR 123].

[PR 82]: https://github.com/prometheus/client_rust/pull/82
[PR 118]: https://github.com/prometheus/client_rust/pull/118
[PR 123]: https://github.com/prometheus/client_rust/pull/123

## [0.19.0]

Expand Down
60 changes: 45 additions & 15 deletions src/encoding/protobuf.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ fn encode_metric(
let encoder = MetricEncoder {
family: &mut family.metrics,
metric_type: super::EncodeMetric::metric_type(metric),
labels: &mut labels,
labels,
};

super::EncodeMetric::encode(metric, encoder.into())?;
Expand All @@ -105,11 +105,17 @@ impl From<MetricType> for openmetrics_data_model::MetricType {
}
}

/// Encoder for protobuf encoding.
///
/// This is an inner type for [`super::MetricEncoder`].
#[derive(Debug)]
pub(crate) struct MetricEncoder<'a> {
/// OpenMetrics metric type of the metric.
metric_type: MetricType,
/// Vector of OpenMetrics metrics to which encoded metrics are added.
family: &'a mut Vec<openmetrics_data_model::Metric>,
labels: &'a mut Vec<openmetrics_data_model::Label>,
/// Labels to be added to each metric.
labels: Vec<openmetrics_data_model::Label>,
}

impl<'a> MetricEncoder<'a> {
Expand Down Expand Up @@ -193,17 +199,18 @@ impl<'a> MetricEncoder<'a> {
&'b mut self,
label_set: &S,
) -> Result<MetricEncoder<'b>, std::fmt::Error> {
let mut labels = self.labels.clone();
label_set.encode(
LabelSetEncoder {
labels: self.labels,
labels: &mut labels,
}
.into(),
)?;

Ok(MetricEncoder {
metric_type: self.metric_type,
family: self.family,
labels: self.labels,
labels,
})
}

Expand Down Expand Up @@ -406,6 +413,7 @@ mod tests {
use crate::metrics::info::Info;
use crate::registry::Unit;
use std::borrow::Cow;
use std::collections::HashSet;
use std::sync::atomic::AtomicI64;

#[test]
Expand All @@ -426,7 +434,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::CounterValue(value) => {
let expected = openmetrics_data_model::counter_value::Total::IntValue(1);
assert_eq!(Some(expected), value.total);
Expand Down Expand Up @@ -456,7 +464,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::CounterValue(value) => {
// The counter should be encoded as `DoubleValue`
let expected = openmetrics_data_model::counter_value::Total::DoubleValue(1.0);
Expand Down Expand Up @@ -507,7 +515,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::CounterValue(value) => {
// The counter should be encoded as `DoubleValue`
let expected = openmetrics_data_model::counter_value::Total::DoubleValue(1.0);
Expand Down Expand Up @@ -545,7 +553,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::GaugeValue(value) => {
let expected = openmetrics_data_model::gauge_value::Value::IntValue(1);
assert_eq!(Some(expected), value.value);
Expand All @@ -567,6 +575,13 @@ mod tests {
])
.inc();

family
.get_or_create(&vec![
("method".to_string(), "POST".to_string()),
("status".to_string(), "200".to_string()),
])
.inc();

let metric_set = encode(&registry).unwrap();

let family = metric_set.metric_families.first().unwrap();
Expand All @@ -578,14 +593,21 @@ mod tests {
extract_metric_type(&metric_set)
);

// The order of the labels is not deterministic so we are testing the
// value to be either
let mut potential_method_value = HashSet::new();
potential_method_value.insert("GET");
potential_method_value.insert("POST");

// the first metric
let metric = family.metrics.first().unwrap();
assert_eq!(2, metric.labels.len());
assert_eq!("method", metric.labels[0].name);
assert_eq!("GET", metric.labels[0].value);
assert!(potential_method_value.remove(&metric.labels[0].value.as_str()));
assert_eq!("status", metric.labels[1].name);
assert_eq!("200", metric.labels[1].value);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::CounterValue(value) => {
let expected = openmetrics_data_model::counter_value::Total::IntValue(1);
assert_eq!(Some(expected), value.total);
Expand All @@ -594,6 +616,14 @@ mod tests {
}
_ => panic!("wrong value type"),
}

// the second metric
let metric2 = &family.metrics[1];
assert_eq!(2, metric2.labels.len());
assert_eq!("method", metric2.labels[0].name);
assert!(potential_method_value.remove(&metric2.labels[0].value.as_str()));
assert_eq!("status", metric2.labels[1].name);
assert_eq!("200", metric2.labels[1].value);
}

#[test]
Expand Down Expand Up @@ -632,7 +662,7 @@ mod tests {
assert_eq!("status", metric.labels[2].name);
assert_eq!("200", metric.labels[2].value);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::CounterValue(value) => {
let expected = openmetrics_data_model::counter_value::Total::IntValue(1);
assert_eq!(Some(expected), value.total);
Expand Down Expand Up @@ -661,7 +691,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::HistogramValue(value) => {
assert_eq!(
Some(openmetrics_data_model::histogram_value::Sum::DoubleValue(
Expand Down Expand Up @@ -694,7 +724,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::HistogramValue(value) => {
let exemplar = value.buckets.first().unwrap().exemplar.as_ref().unwrap();
assert_eq!(1.0, exemplar.value);
Expand Down Expand Up @@ -795,7 +825,7 @@ mod tests {
extract_metric_type(&metric_set)
);

match extract_metric_point_value(metric_set) {
match extract_metric_point_value(&metric_set) {
openmetrics_data_model::metric_point::Value::InfoValue(value) => {
assert_eq!(1, value.info.len());

Expand All @@ -813,7 +843,7 @@ mod tests {
}

fn extract_metric_point_value(
metric_set: openmetrics_data_model::MetricSet,
metric_set: &openmetrics_data_model::MetricSet,
) -> openmetrics_data_model::metric_point::Value {
let metric = metric_set
.metric_families
Expand Down

0 comments on commit e35e99d

Please sign in to comment.