From 8f95bb986414a6dad34b8458c514a0d9066abbce Mon Sep 17 00:00:00 2001 From: Ethan Olchik Date: Tue, 21 Jul 2026 17:01:11 +0100 Subject: [PATCH 1/6] feat(metrics): validate and sanitize names, labels, and exemplars --- foundations-metrics/src/collect.rs | 310 ++++++++++++++++- foundations-metrics/src/encoding/mod.rs | 198 ++++++++++- foundations-metrics/src/encoding/text.rs | 207 +++++++++++- foundations-metrics/src/labels/serializer.rs | 27 +- foundations-metrics/src/lib.rs | 1 + foundations-metrics/src/validation.rs | 332 +++++++++++++++++++ 6 files changed, 1048 insertions(+), 27 deletions(-) create mode 100644 foundations-metrics/src/validation.rs diff --git a/foundations-metrics/src/collect.rs b/foundations-metrics/src/collect.rs index 864f920a..f26c16bf 100644 --- a/foundations-metrics/src/collect.rs +++ b/foundations-metrics/src/collect.rs @@ -1,6 +1,10 @@ use foundations_metrics_registry::{iter, proto::LabelPair}; use crate::MetricFamily; +use crate::diagnostics::report_collect_error; +use crate::validation::{ + LABEL_NAME_GRAMMAR, ValidationContext, is_valid_label_name, sanitize_metric_family, +}; /// Options that control which registered metrics are collected and how the /// service name is represented. @@ -28,6 +32,16 @@ pub enum ServiceNameFormat<'a> { /// Collects the currently registered metrics into the canonical protobuf model. pub fn collect(options: CollectionOptions) -> Vec { + if options.service_name.is_some() + && let ServiceNameFormat::LabelWithName(label_name) = options.service_name_format + && !is_valid_label_name(label_name) + { + report_collect_error(format_args!( + "non-fatal error while collecting metrics: invalid configured service label name {label_name:?}; expected {LABEL_NAME_GRAMMAR}; skipped all metric families" + )); + return Vec::new(); + } + let mut collected = Vec::new(); for registered in iter() { @@ -56,15 +70,35 @@ pub fn collect(options: CollectionOptions) -> Vec { }; for family in &mut families { - for metric in &mut family.metric { - metric.label.insert(0, service_label.clone()); - } + let family_name = family.name.as_deref().unwrap_or_default(); + family.metric.retain_mut(|metric| { + let mut has_same_value = false; + for label in &metric.label { + if label.name.as_deref() != Some(label_name) { + continue; + } + + if label.value.as_deref() != Some(service_name) { + report_collect_error(format_args!( + "non-fatal error while collecting metrics: skipped row in metric family {family_name:?}; service label {label_name:?} already has a different value" + )); + return false; + } + has_same_value = true; + } + + if !has_same_value { + metric.label.insert(0, service_label.clone()); + } + true + }); } } ServiceNameFormat::MetricPrefix => {} } } + families.retain_mut(|family| sanitize_metric_family(family, ValidationContext::Collection)); collected.extend(families); } @@ -73,13 +107,17 @@ pub fn collect(options: CollectionOptions) -> Vec { #[cfg(test)] mod tests { - use foundations_metrics_registry::proto::{Metric, MetricType}; + use foundations_metrics_registry::proto::{ + Bucket, Counter, Exemplar, Gauge, Histogram, LabelPair, Metric, MetricType, + }; use super::*; use crate::{EncodeMetric, RegistrationMetadata, register}; struct TestMetric(&'static str); + struct TestFamilyMetric(MetricFamily); + impl EncodeMetric for TestMetric { fn encode(&self) -> Vec { vec![MetricFamily { @@ -92,6 +130,12 @@ mod tests { } } + impl EncodeMetric for TestFamilyMetric { + fn encode(&self) -> Vec { + vec![self.0.clone()] + } + } + fn register_test_metric(name: &'static str, metadata: RegistrationMetadata) { register( Box::new(TestMetric(name)) as Box, @@ -99,6 +143,20 @@ mod tests { ); } + fn register_test_family(family: MetricFamily) { + register( + Box::new(TestFamilyMetric(family)) as Box, + RegistrationMetadata::default(), + ); + } + + fn label(name: &str, value: &str) -> LabelPair { + LabelPair { + name: Some(name.to_owned()), + value: Some(value.to_owned()), + } + } + #[test] fn filters_optional_metrics_and_applies_service_prefix() { register_test_metric("collect_required_metric", RegistrationMetadata::default()); @@ -161,4 +219,248 @@ mod tests { assert_eq!(label.value.as_deref(), Some("test_service")); } } + + #[test] + fn rejects_invalid_final_service_prefixed_family_names() { + register_test_metric( + "collect_invalid_prefix_metric", + RegistrationMetadata::default(), + ); + + let families = collect(CollectionOptions { + include_optional: false, + service_name: Some("invalid-service"), + service_name_format: ServiceNameFormat::MetricPrefix, + }); + + assert!(!families.iter().any(|family| { + family.name.as_deref() == Some("invalid-service_collect_invalid_prefix_metric") + })); + } + + #[test] + fn invalid_service_label_name_rejects_the_whole_collection() { + register_test_metric( + "collect_invalid_service_label_metric", + RegistrationMetadata::default(), + ); + + let families = collect(CollectionOptions { + include_optional: false, + service_name: Some("test_service"), + service_name_format: ServiceNameFormat::LabelWithName("service:name"), + }); + + assert!(families.is_empty()); + } + + #[test] + fn service_label_insertion_is_idempotent_and_drops_different_values() { + let service_label_name = "collect_service_collision_label"; + register_test_family(MetricFamily { + name: Some("collect_service_collision_metric".to_owned()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![ + Metric { + label: vec![label("id", "same"), label(service_label_name, "wanted")], + gauge: Some(Gauge { value: Some(1.0) }), + ..Default::default() + }, + Metric { + label: vec![label("id", "different"), label(service_label_name, "other")], + gauge: Some(Gauge { value: Some(2.0) }), + ..Default::default() + }, + Metric { + label: vec![label("id", "absent")], + gauge: Some(Gauge { value: Some(3.0) }), + ..Default::default() + }, + ], + unit: None, + }); + + let families = collect(CollectionOptions { + include_optional: false, + service_name: Some("wanted"), + service_name_format: ServiceNameFormat::LabelWithName(service_label_name), + }); + let family = families + .iter() + .find(|family| family.name.as_deref() == Some("collect_service_collision_metric")) + .expect("test family should be collected"); + + assert_eq!(family.metric.len(), 2); + let same = family + .metric + .iter() + .find(|metric| metric.label[0].value.as_deref() == Some("same")) + .expect("same-value row should remain"); + assert_eq!( + same.label + .iter() + .filter(|label| label.name.as_deref() == Some(service_label_name)) + .count(), + 1 + ); + assert_eq!(same.label[0].name.as_deref(), Some("id")); + + let absent = family + .metric + .iter() + .find(|metric| { + metric + .label + .iter() + .any(|label| label.value.as_deref() == Some("absent")) + }) + .expect("row without a service label should remain"); + assert_eq!( + absent.label[0], + label(service_label_name, "wanted"), + "new service labels remain prepended" + ); + } + + #[test] + fn collection_skips_invalid_duplicate_and_reserved_row_labels() { + register_test_family(MetricFamily { + name: Some("collect_row_validation_gauge".to_owned()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![ + Metric { + label: vec![label("id", "valid")], + gauge: Some(Gauge { value: Some(1.0) }), + ..Default::default() + }, + Metric { + label: vec![label("bad\nname", "invalid")], + gauge: Some(Gauge { value: Some(2.0) }), + ..Default::default() + }, + Metric { + label: vec![label("dup", "a"), label("dup", "b")], + gauge: Some(Gauge { value: Some(3.0) }), + ..Default::default() + }, + ], + unit: None, + }); + register_test_family(MetricFamily { + name: Some("collect_row_validation_histogram".to_owned()), + help: None, + r#type: Some(MetricType::Histogram as i32), + metric: vec![ + Metric { + histogram: Some(Histogram::default()), + ..Default::default() + }, + Metric { + label: vec![label("le", "1")], + histogram: Some(Histogram::default()), + ..Default::default() + }, + ], + unit: None, + }); + let families = collect(CollectionOptions { + include_optional: false, + service_name: None, + service_name_format: ServiceNameFormat::MetricPrefix, + }); + + for name in [ + "collect_row_validation_gauge", + "collect_row_validation_histogram", + ] { + let family = families + .iter() + .find(|family| family.name.as_deref() == Some(name)) + .expect("valid family should remain"); + assert_eq!(family.metric.len(), 1, "family {name}"); + } + } + + #[test] + fn collection_drops_only_invalid_exemplars() { + register_test_family(MetricFamily { + name: Some("collect_counter_exemplar_validation".to_owned()), + help: None, + r#type: Some(MetricType::Counter as i32), + metric: vec![Metric { + counter: Some(Counter { + value: Some(1.0), + exemplar: Some(Exemplar { + label: vec![label("trace:id", "bad")], + value: Some(2.0), + timestamp: None, + }), + created_timestamp: None, + }), + ..Default::default() + }], + unit: None, + }); + register_test_family(MetricFamily { + name: Some("collect_histogram_exemplar_validation".to_owned()), + help: None, + r#type: Some(MetricType::Histogram as i32), + metric: vec![Metric { + histogram: Some(Histogram { + bucket: vec![Bucket { + exemplar: Some(Exemplar { + label: vec![label("dup", "a"), label("dup", "b")], + ..Default::default() + }), + ..Default::default() + }], + exemplars: vec![ + Exemplar { + label: vec![label("bad name", "bad")], + ..Default::default() + }, + Exemplar { + label: vec![label("trace_id", "good")], + ..Default::default() + }, + ], + ..Default::default() + }), + ..Default::default() + }], + unit: None, + }); + + let families = collect(CollectionOptions { + include_optional: false, + service_name: None, + service_name_format: ServiceNameFormat::MetricPrefix, + }); + let counter = families + .iter() + .find(|family| family.name.as_deref() == Some("collect_counter_exemplar_validation")) + .expect("counter family should remain"); + assert!( + counter.metric[0] + .counter + .as_ref() + .unwrap() + .exemplar + .is_none() + ); + + let histogram = families + .iter() + .find(|family| family.name.as_deref() == Some("collect_histogram_exemplar_validation")) + .expect("histogram family should remain"); + let histogram = histogram.metric[0].histogram.as_ref().unwrap(); + assert!(histogram.bucket[0].exemplar.is_none()); + assert_eq!(histogram.exemplars.len(), 1); + assert_eq!( + histogram.exemplars[0].label[0].name.as_deref(), + Some("trace_id") + ); + } } diff --git a/foundations-metrics/src/encoding/mod.rs b/foundations-metrics/src/encoding/mod.rs index 3fc45674..4fa50864 100644 --- a/foundations-metrics/src/encoding/mod.rs +++ b/foundations-metrics/src/encoding/mod.rs @@ -3,25 +3,213 @@ mod text; use prost::Message; use crate::MetricFamily; +use crate::validation::{ValidationContext, sanitized_metric_family}; pub use text::encode_to_text; /// Encodes metric families as length-delimited Prometheus protobuf messages. pub fn encode_to_protobuf(families: &[MetricFamily]) -> Vec { - families - .iter() - .flat_map(Message::encode_length_delimited_to_vec) - .collect() + let mut output = Vec::new(); + for family in families { + if let Some(family) = sanitized_metric_family(family, ValidationContext::ProtobufEncoding) { + family + .encode_length_delimited(&mut output) + .expect("encoding a protobuf message to a Vec cannot fail"); + } + } + output } #[cfg(test)] mod tests { use foundations_metrics_registry::proto::{ - Bucket, Gauge, Histogram, LabelPair, Metric, MetricType, Quantile, Summary, + Bucket, Counter, Exemplar, Gauge, Histogram, LabelPair, Metric, MetricType, Quantile, + Summary, }; use super::*; + fn label(name: &str, value: &str) -> LabelPair { + LabelPair { + name: Some(name.to_owned()), + value: Some(value.to_owned()), + } + } + + fn decode_families(mut bytes: &[u8]) -> Vec { + let mut families = Vec::new(); + while !bytes.is_empty() { + families.push( + MetricFamily::decode_length_delimited(&mut bytes) + .expect("encoded family should decode"), + ); + } + families + } + + #[test] + fn fully_valid_protobuf_output_is_unchanged() { + let families = [MetricFamily { + name: Some("valid:counter".to_owned()), + help: Some("Valid counter.".to_owned()), + r#type: Some(MetricType::Counter as i32), + metric: vec![Metric { + label: vec![label("_label", "value")], + counter: Some(Counter { + value: Some(1.0), + exemplar: Some(Exemplar::default()), + created_timestamp: None, + }), + ..Default::default() + }], + unit: None, + }]; + let expected: Vec<_> = families + .iter() + .flat_map(Message::encode_length_delimited_to_vec) + .collect(); + + let encoded = encode_to_protobuf(&families); + assert_eq!(encoded, expected); + assert!( + decode_families(&encoded)[0].metric[0] + .counter + .as_ref() + .unwrap() + .exemplar + .is_some(), + "empty exemplars retain their existing protobuf behavior" + ); + } + + #[test] + fn protobuf_defensively_omits_invalid_families_and_rows_and_strips_exemplars() { + let families = [ + MetricFamily { + name: Some("bad\nfamily".to_owned()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![Metric { + gauge: Some(Gauge { value: Some(99.0) }), + ..Default::default() + }], + unit: None, + }, + MetricFamily { + name: Some("protobuf_counter".to_owned()), + help: None, + r#type: Some(MetricType::Counter as i32), + metric: vec![ + Metric { + label: vec![label("id", "kept")], + counter: Some(Counter { + value: Some(1.0), + exemplar: Some(Exemplar { + label: vec![label("trace:id", "bad")], + ..Default::default() + }), + created_timestamp: None, + }), + ..Default::default() + }, + Metric { + label: vec![label("bad name", "dropped")], + counter: Some(Counter { + value: Some(2.0), + ..Default::default() + }), + ..Default::default() + }, + Metric { + label: vec![label("dup", "a"), label("dup", "b")], + counter: Some(Counter { + value: Some(3.0), + ..Default::default() + }), + ..Default::default() + }, + ], + unit: None, + }, + MetricFamily { + name: Some("protobuf_histogram".to_owned()), + help: None, + r#type: Some(MetricType::Histogram as i32), + metric: vec![ + Metric { + histogram: Some(Histogram { + bucket: vec![Bucket { + exemplar: Some(Exemplar { + label: vec![label("dup", "a"), label("dup", "b")], + ..Default::default() + }), + ..Default::default() + }], + exemplars: vec![ + Exemplar { + label: vec![label("bad#name", "bad")], + ..Default::default() + }, + Exemplar { + label: vec![label("trace_id", "good")], + ..Default::default() + }, + ], + ..Default::default() + }), + ..Default::default() + }, + Metric { + label: vec![label("le", "1")], + histogram: Some(Histogram::default()), + ..Default::default() + }, + ], + unit: None, + }, + MetricFamily { + name: Some("protobuf_sibling".to_owned()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![Metric { + gauge: Some(Gauge { value: Some(4.0) }), + ..Default::default() + }], + unit: None, + }, + ]; + + let decoded = decode_families(&encode_to_protobuf(&families)); + assert_eq!( + decoded + .iter() + .filter_map(|family| family.name.as_deref()) + .collect::>(), + ["protobuf_counter", "protobuf_histogram", "protobuf_sibling",] + ); + + assert_eq!(decoded[0].metric.len(), 1); + assert!( + decoded[0].metric[0] + .counter + .as_ref() + .unwrap() + .exemplar + .is_none() + ); + + assert_eq!(decoded[1].metric.len(), 1); + let histogram = decoded[1].metric[0].histogram.as_ref().unwrap(); + assert!(histogram.bucket[0].exemplar.is_none()); + assert_eq!(histogram.exemplars.len(), 1); + assert_eq!( + histogram.exemplars[0].label[0].name.as_deref(), + Some("trace_id") + ); + + assert_eq!(decoded[2].metric.len(), 1); + } + #[test] fn preserves_summary_and_gauge_histogram_families() { let families = [ diff --git a/foundations-metrics/src/encoding/text.rs b/foundations-metrics/src/encoding/text.rs index 66e1072e..71f92c6c 100644 --- a/foundations-metrics/src/encoding/text.rs +++ b/foundations-metrics/src/encoding/text.rs @@ -5,13 +5,16 @@ use foundations_metrics_registry::proto::{ }; use crate::diagnostics::report_collect_error; +use crate::validation::{ValidationContext, sanitized_metric_family}; /// Encodes metric families as OpenMetrics text. pub fn encode_to_text(families: &[MetricFamily]) -> String { let mut output = String::new(); for family in families { - encode_family(&mut output, family); + if let Some(family) = sanitized_metric_family(family, ValidationContext::TextEncoding) { + encode_family(&mut output, &family); + } } output.push_str("# EOF\n"); @@ -19,12 +22,10 @@ pub fn encode_to_text(families: &[MetricFamily]) -> String { } fn encode_family(output: &mut String, family: &MetricFamily) { - let Some(name) = family.name.as_deref().filter(|name| !name.is_empty()) else { - report_collect_error(format_args!( - "non-fatal error while encoding OpenMetrics text: skipped a metric family without a name" - )); - return; - }; + let name = family + .name + .as_deref() + .expect("metric family names are validated before text encoding"); let Some(metric_type) = family .r#type .and_then(|value| MetricType::try_from(value).ok()) @@ -377,12 +378,19 @@ fn report_missing_value(name: &str, expected: &str) { #[cfg(test)] mod tests { use foundations_metrics_registry::proto::{ - Bucket, Counter, Gauge, Histogram, LabelPair, Metric, MetricFamily, MetricType, Quantile, - Summary, + Bucket, Counter, Exemplar, Gauge, Histogram, LabelPair, Metric, MetricFamily, MetricType, + Quantile, Summary, }; use super::*; + fn label(name: &str, value: &str) -> LabelPair { + LabelPair { + name: Some(name.to_owned()), + value: Some(value.to_owned()), + } + } + #[test] fn omits_the_help_line_when_there_is_no_help_text() { let families = [MetricFamily { @@ -625,4 +633,185 @@ build_info{version=\"1.2.3\"} 1.0\n\ # EOF\n" ); } + + #[test] + fn invalid_family_names_cannot_inject_metadata_and_valid_siblings_remain() { + let families = [ + MetricFamily { + name: Some("bad\n# HELP injected metadata".to_owned()), + help: Some("should not be written".to_owned()), + r#type: Some(MetricType::Gauge as i32), + metric: vec![Metric { + gauge: Some(Gauge { value: Some(99.0) }), + ..Default::default() + }], + unit: None, + }, + MetricFamily { + name: Some("valid:metric".to_owned()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![Metric { + gauge: Some(Gauge { value: Some(1.0) }), + ..Default::default() + }], + unit: None, + }, + ]; + + assert_eq!( + encode_to_text(&families), + "# TYPE valid:metric gauge\nvalid:metric 1.0\n# EOF\n" + ); + } + + #[test] + fn invalid_duplicate_and_reserved_row_labels_skip_only_their_rows() { + let families = [ + MetricFamily { + name: Some("row_gauge".to_owned()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![ + Metric { + label: vec![label("id", "valid")], + gauge: Some(Gauge { value: Some(1.0) }), + ..Default::default() + }, + Metric { + label: vec![label("bad name", "invalid")], + gauge: Some(Gauge { value: Some(99.0) }), + ..Default::default() + }, + Metric { + label: vec![label("dup", "a"), label("dup", "b")], + gauge: Some(Gauge { value: Some(98.0) }), + ..Default::default() + }, + ], + unit: None, + }, + MetricFamily { + name: Some("row_histogram".to_owned()), + help: None, + r#type: Some(MetricType::Histogram as i32), + metric: vec![ + Metric { + histogram: Some(Histogram { + sample_count: Some(1), + sample_sum: Some(2.0), + ..Default::default() + }), + ..Default::default() + }, + Metric { + label: vec![label("le", "1")], + histogram: Some(Histogram { + sample_count: Some(99), + sample_sum: Some(99.0), + ..Default::default() + }), + ..Default::default() + }, + ], + unit: None, + }, + MetricFamily { + name: Some("row_summary".to_owned()), + help: None, + r#type: Some(MetricType::Summary as i32), + metric: vec![ + Metric { + summary: Some(Default::default()), + ..Default::default() + }, + Metric { + label: vec![label("quantile", "0.5")], + summary: Some(Default::default()), + ..Default::default() + }, + ], + unit: None, + }, + ]; + + let output = encode_to_text(&families); + assert!(output.contains("row_gauge{id=\"valid\"} 1.0\n")); + assert!(!output.contains("99.0")); + assert!(!output.contains("98.0")); + assert_eq!(output.matches("row_histogram_sum").count(), 1); + assert_eq!(output.matches("row_summary_sum").count(), 1); + assert!(output.ends_with("# EOF\n")); + } + + #[test] + fn invalid_exemplar_labels_drop_only_the_exemplar() { + let families = [ + MetricFamily { + name: Some("exemplar_counter".to_owned()), + help: None, + r#type: Some(MetricType::Counter as i32), + metric: vec![ + Metric { + label: vec![label("id", "invalid")], + counter: Some(Counter { + value: Some(1.0), + exemplar: Some(Exemplar { + label: vec![label("trace:id", "bad")], + value: Some(2.0), + timestamp: None, + }), + created_timestamp: None, + }), + ..Default::default() + }, + Metric { + label: vec![label("id", "valid")], + counter: Some(Counter { + value: Some(3.0), + exemplar: Some(Exemplar { + label: vec![label("trace_id", "good")], + value: Some(4.0), + timestamp: None, + }), + created_timestamp: None, + }), + ..Default::default() + }, + ], + unit: None, + }, + MetricFamily { + name: Some("exemplar_histogram".to_owned()), + help: None, + r#type: Some(MetricType::Histogram as i32), + metric: vec![Metric { + histogram: Some(Histogram { + sample_count: Some(1), + sample_sum: Some(1.0), + bucket: vec![Bucket { + cumulative_count: Some(1), + upper_bound: Some(1.0), + exemplar: Some(Exemplar { + label: vec![label("dup", "a"), label("dup", "b")], + value: Some(5.0), + timestamp: None, + }), + ..Default::default() + }], + ..Default::default() + }), + ..Default::default() + }], + unit: None, + }, + ]; + + let output = encode_to_text(&families); + assert!(output.contains("exemplar_counter{id=\"invalid\"} 1.0\n")); + assert!(output.contains("exemplar_counter{id=\"valid\"} 3.0 # {trace_id=\"good\"} 4.0\n")); + assert!(output.contains("exemplar_histogram_bucket{le=\"1.0\"} 1\n")); + assert!(!output.contains("trace:id")); + assert!(!output.contains("{dup=")); + } } diff --git a/foundations-metrics/src/labels/serializer.rs b/foundations-metrics/src/labels/serializer.rs index cf3954e6..6615e8f2 100644 --- a/foundations-metrics/src/labels/serializer.rs +++ b/foundations-metrics/src/labels/serializer.rs @@ -5,6 +5,7 @@ use serde::Serialize; use serde::ser::{Impossible, SerializeStruct, Serializer}; use super::LabelError; +use crate::validation::{LABEL_NAME_GRAMMAR, is_valid_label_name}; // Adapted from prometools' `serde::top::TopSerializer` // (https://github.com/nox/prometools, licensed MIT OR Apache-2.0). @@ -396,16 +397,9 @@ impl Serializer for LabelValueSerializer { // Adapted from prometools' `serde::top::check_key` // (https://github.com/nox/prometools, licensed MIT OR Apache-2.0). fn validate_label_name(name: &str) -> Result<(), LabelError> { - let mut chars = name.chars(); - let valid = chars - .next() - .is_some_and(|character| character.is_ascii_alphabetic() || matches!(character, '_' | ':')) - && chars - .all(|character| character.is_ascii_alphanumeric() || matches!(character, '_' | ':')); - - valid.then_some(()).ok_or_else(|| { + is_valid_label_name(name).then_some(()).ok_or_else(|| { LabelError::new(format!( - "invalid metric label name {name:?}: expected [a-zA-Z_:][a-zA-Z0-9_:]*" + "invalid metric label name {name:?}: expected {LABEL_NAME_GRAMMAR}" )) }) } @@ -490,6 +484,21 @@ mod tests { assert!(to_label_pairs(&Invalid { value: "x" }).is_err()); } + #[test] + fn rejects_colon_label_names_with_the_label_grammar_in_the_error() { + #[derive(Serialize)] + struct Invalid { + #[serde(rename = "trace:id")] + value: &'static str, + } + + let error = to_label_pairs(&Invalid { value: "x" }).unwrap_err(); + assert_eq!( + error.to_string(), + "invalid metric label name \"trace:id\": expected [a-zA-Z_][a-zA-Z0-9_]*" + ); + } + #[test] fn rejects_compound_label_values() { #[derive(Serialize)] diff --git a/foundations-metrics/src/lib.rs b/foundations-metrics/src/lib.rs index 943c8362..affd18b9 100644 --- a/foundations-metrics/src/lib.rs +++ b/foundations-metrics/src/lib.rs @@ -12,6 +12,7 @@ mod encoding; mod labels; pub mod metrics; mod registered; +mod validation; mod value; pub use collect::{CollectionOptions, ServiceNameFormat, collect}; diff --git a/foundations-metrics/src/validation.rs b/foundations-metrics/src/validation.rs new file mode 100644 index 00000000..7e12640e --- /dev/null +++ b/foundations-metrics/src/validation.rs @@ -0,0 +1,332 @@ +use std::borrow::Cow; + +use foundations_metrics_registry::proto::{Exemplar, LabelPair, Metric, MetricFamily, MetricType}; + +use crate::diagnostics::report_collect_error; + +pub(crate) const METRIC_NAME_GRAMMAR: &str = "[a-zA-Z_:][a-zA-Z0-9_:]*"; +pub(crate) const LABEL_NAME_GRAMMAR: &str = "[a-zA-Z_][a-zA-Z0-9_]*"; + +#[derive(Clone, Copy)] +pub(crate) enum ValidationContext { + Collection, + TextEncoding, + ProtobufEncoding, +} + +impl ValidationContext { + fn action(self) -> &'static str { + match self { + Self::Collection => "collecting metrics", + Self::TextEncoding => "encoding OpenMetrics text", + Self::ProtobufEncoding => "encoding Prometheus protobuf", + } + } +} + +pub(crate) fn is_valid_metric_name(name: &str) -> bool { + is_valid_name(name, true) +} + +pub(crate) fn is_valid_label_name(name: &str) -> bool { + is_valid_name(name, false) +} + +fn is_valid_name(name: &str, allow_colon: bool) -> bool { + let mut bytes = name.bytes(); + bytes.next().is_some_and(|byte| { + byte.is_ascii_alphabetic() || byte == b'_' || (allow_colon && byte == b':') + }) && bytes + .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_' || (allow_colon && byte == b':')) +} + +pub(crate) fn sanitize_metric_family( + family: &mut MetricFamily, + context: ValidationContext, +) -> bool { + let Some(name) = family + .name + .as_deref() + .filter(|name| is_valid_metric_name(name)) + else { + report_invalid_family_name(context, family.name.as_deref()); + return false; + }; + + sanitize_rows( + &mut family.metric, + family + .r#type + .and_then(|value| MetricType::try_from(value).ok()), + name, + context, + ); + true +} + +pub(crate) fn sanitized_metric_family<'a>( + family: &'a MetricFamily, + context: ValidationContext, +) -> Option> { + let Some(name) = family + .name + .as_deref() + .filter(|name| is_valid_metric_name(name)) + else { + report_invalid_family_name(context, family.name.as_deref()); + return None; + }; + let metric_type = family + .r#type + .and_then(|value| MetricType::try_from(value).ok()); + + if rows_are_valid(&family.metric, metric_type) { + return Some(Cow::Borrowed(family)); + } + + let mut sanitized = family.clone(); + sanitize_rows(&mut sanitized.metric, metric_type, name, context); + Some(Cow::Owned(sanitized)) +} + +fn report_invalid_family_name(context: ValidationContext, name: Option<&str>) { + report_collect_error(format_args!( + "non-fatal error while {}: skipped metric family with invalid name {name:?}; expected {METRIC_NAME_GRAMMAR}", + context.action() + )); +} + +fn rows_are_valid(metrics: &[Metric], metric_type: Option) -> bool { + let reserved_label = reserved_row_label(metric_type); + metrics.iter().all(|metric| { + find_label_issue(&metric.label, reserved_label).is_none() && exemplars_are_valid(metric) + }) +} + +fn sanitize_rows( + metrics: &mut Vec, + metric_type: Option, + family_name: &str, + context: ValidationContext, +) { + let reserved_label = reserved_row_label(metric_type); + metrics.retain_mut(|metric| { + if let Some(issue) = find_label_issue(&metric.label, reserved_label) { + report_row_drop(context, family_name, issue); + return false; + } + + sanitize_exemplars(metric, family_name, context); + true + }); +} + +fn reserved_row_label(metric_type: Option) -> Option<&'static str> { + match metric_type { + Some(MetricType::Histogram | MetricType::GaugeHistogram) => Some("le"), + Some(MetricType::Summary) => Some("quantile"), + _ => None, + } +} + +#[derive(Clone, Copy)] +enum LabelIssue<'a> { + Invalid(Option<&'a str>), + Duplicate(&'a str), + Reserved(&'a str), +} + +fn find_label_issue<'a>( + labels: &'a [LabelPair], + reserved_label: Option<&str>, +) -> Option> { + for (index, label) in labels.iter().enumerate() { + let Some(name) = label + .name + .as_deref() + .filter(|name| is_valid_label_name(name)) + else { + return Some(LabelIssue::Invalid(label.name.as_deref())); + }; + + if labels[..index] + .iter() + .any(|previous| previous.name.as_deref() == Some(name)) + { + return Some(LabelIssue::Duplicate(name)); + } + if reserved_label == Some(name) { + return Some(LabelIssue::Reserved(name)); + } + } + + None +} + +fn report_row_drop(context: ValidationContext, family_name: &str, issue: LabelIssue<'_>) { + match issue { + LabelIssue::Invalid(name) => report_collect_error(format_args!( + "non-fatal error while {}: skipped row in metric family {family_name:?} with invalid label name {name:?}; expected {LABEL_NAME_GRAMMAR}", + context.action() + )), + LabelIssue::Duplicate(name) => report_collect_error(format_args!( + "non-fatal error while {}: skipped row in metric family {family_name:?} with duplicate label name {name:?}", + context.action() + )), + LabelIssue::Reserved(name) => report_collect_error(format_args!( + "non-fatal error while {}: skipped row in metric family {family_name:?}; label name {name:?} is reserved for this metric type", + context.action() + )), + } +} + +fn exemplars_are_valid(metric: &Metric) -> bool { + metric + .counter + .as_ref() + .and_then(|counter| counter.exemplar.as_ref()) + .is_none_or(exemplar_is_valid) + && metric.histogram.as_ref().is_none_or(|histogram| { + histogram + .bucket + .iter() + .all(|bucket| bucket.exemplar.as_ref().is_none_or(exemplar_is_valid)) + && histogram.exemplars.iter().all(exemplar_is_valid) + }) +} + +fn exemplar_is_valid(exemplar: &Exemplar) -> bool { + find_label_issue(&exemplar.label, None).is_none() +} + +fn sanitize_exemplars(metric: &mut Metric, family_name: &str, context: ValidationContext) { + if let Some(counter) = &mut metric.counter { + sanitize_exemplar_slot(&mut counter.exemplar, "counter", family_name, context); + } + + if let Some(histogram) = &mut metric.histogram { + for bucket in &mut histogram.bucket { + sanitize_exemplar_slot( + &mut bucket.exemplar, + "classic histogram bucket", + family_name, + context, + ); + } + histogram.exemplars.retain(|exemplar| { + if let Some(issue) = find_label_issue(&exemplar.label, None) { + report_exemplar_drop(context, family_name, "native histogram", issue); + false + } else { + true + } + }); + } +} + +fn sanitize_exemplar_slot( + exemplar: &mut Option, + kind: &str, + family_name: &str, + context: ValidationContext, +) { + let Some(issue) = exemplar + .as_ref() + .and_then(|exemplar| find_label_issue(&exemplar.label, None)) + else { + return; + }; + + report_exemplar_drop(context, family_name, kind, issue); + *exemplar = None; +} + +fn report_exemplar_drop( + context: ValidationContext, + family_name: &str, + kind: &str, + issue: LabelIssue<'_>, +) { + match issue { + LabelIssue::Invalid(name) => report_collect_error(format_args!( + "non-fatal error while {}: dropped {kind} exemplar in metric family {family_name:?} with invalid label name {name:?}; expected {LABEL_NAME_GRAMMAR}", + context.action() + )), + LabelIssue::Duplicate(name) => report_collect_error(format_args!( + "non-fatal error while {}: dropped {kind} exemplar in metric family {family_name:?} with duplicate label name {name:?}", + context.action() + )), + LabelIssue::Reserved(_) => unreachable!("exemplar labels have no reserved names"), + } +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn exhaustively_checks_ascii_metric_name_characters() { + for byte in 0_u8..=127 { + let character = char::from(byte); + let expected_first = byte.is_ascii_alphabetic() || matches!(byte, b'_' | b':'); + let expected_continuation = byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b':'); + + assert_eq!( + is_valid_metric_name(&character.to_string()), + expected_first, + "metric first character {byte:#04x}" + ); + assert_eq!( + is_valid_metric_name(&format!("a{character}")), + expected_continuation, + "metric continuation character {byte:#04x}" + ); + } + } + + #[test] + fn exhaustively_checks_ascii_label_name_characters() { + for byte in 0_u8..=127 { + let character = char::from(byte); + let expected_first = byte.is_ascii_alphabetic() || byte == b'_'; + let expected_continuation = byte.is_ascii_alphanumeric() || byte == b'_'; + + assert_eq!( + is_valid_label_name(&character.to_string()), + expected_first, + "label first character {byte:#04x}" + ); + assert_eq!( + is_valid_label_name(&format!("a{character}")), + expected_continuation, + "label continuation character {byte:#04x}" + ); + } + } + + #[test] + fn rejects_empty_non_ascii_and_injection_names() { + for name in [ + "", + "é", + "aλ", + "metric name", + "metric\nname", + "metric#name", + "metric\"name", + ] { + assert!(!is_valid_metric_name(name), "metric name {name:?}"); + assert!(!is_valid_label_name(name), "label name {name:?}"); + } + } + + #[test] + fn metric_names_allow_colons_but_label_names_do_not() { + for name in [":", ":metric", "metric:name"] { + assert!(is_valid_metric_name(name)); + assert!(!is_valid_label_name(name)); + } + assert!(is_valid_metric_name("_metric")); + assert!(is_valid_label_name("_label")); + } +} From 32b5ebf5fd45afa5f9e4eed90bcec304d07d9dc5 Mon Sep 17 00:00:00 2001 From: Ethan Olchik Date: Thu, 23 Jul 2026 16:12:56 +0100 Subject: [PATCH 2/6] refactor(metrics): add support for UTF-8 characters in metric & label names Legacy metric and label names must match Prometheus' name grammars. Names outside these grammars cause Prometheus to reject the scrape, and the grammars exclude non-ASCII characters. Foundations now quotes label names and uses OpenMetrics' quoted metric-name syntax when required. This allows UTF-8 and other nonstandard names without affecting legacy compatibility. --- foundations-metrics/src/collect.rs | 62 ++++--- foundations-metrics/src/encoding/mod.rs | 66 +++++-- foundations-metrics/src/encoding/text.rs | 178 ++++++++++++++++--- foundations-metrics/src/labels/serializer.rs | 23 ++- foundations-metrics/src/lib.rs | 2 +- foundations-metrics/src/validation.rs | 101 ++--------- 6 files changed, 264 insertions(+), 168 deletions(-) diff --git a/foundations-metrics/src/collect.rs b/foundations-metrics/src/collect.rs index f26c16bf..0bec3e74 100644 --- a/foundations-metrics/src/collect.rs +++ b/foundations-metrics/src/collect.rs @@ -3,7 +3,7 @@ use foundations_metrics_registry::{iter, proto::LabelPair}; use crate::MetricFamily; use crate::diagnostics::report_collect_error; use crate::validation::{ - LABEL_NAME_GRAMMAR, ValidationContext, is_valid_label_name, sanitize_metric_family, + NAME_REQUIREMENT, ValidationContext, is_valid_name, sanitize_metric_family, }; /// Options that control which registered metrics are collected and how the @@ -34,10 +34,10 @@ pub enum ServiceNameFormat<'a> { pub fn collect(options: CollectionOptions) -> Vec { if options.service_name.is_some() && let ServiceNameFormat::LabelWithName(label_name) = options.service_name_format - && !is_valid_label_name(label_name) + && !is_valid_name(label_name) { report_collect_error(format_args!( - "non-fatal error while collecting metrics: invalid configured service label name {label_name:?}; expected {LABEL_NAME_GRAMMAR}; skipped all metric families" + "non-fatal error while collecting metrics: invalid configured service label name {label_name:?}; expected {NAME_REQUIREMENT}; skipped all metric families" )); return Vec::new(); } @@ -221,9 +221,9 @@ mod tests { } #[test] - fn rejects_invalid_final_service_prefixed_family_names() { + fn keeps_nonstandard_service_prefixed_family_names() { register_test_metric( - "collect_invalid_prefix_metric", + "collect_nonstandard_prefix_metric", RegistrationMetadata::default(), ); @@ -233,15 +233,15 @@ mod tests { service_name_format: ServiceNameFormat::MetricPrefix, }); - assert!(!families.iter().any(|family| { - family.name.as_deref() == Some("invalid-service_collect_invalid_prefix_metric") + assert!(families.iter().any(|family| { + family.name.as_deref() == Some("invalid-service_collect_nonstandard_prefix_metric") })); } #[test] - fn invalid_service_label_name_rejects_the_whole_collection() { + fn keeps_nonstandard_service_label_names() { register_test_metric( - "collect_invalid_service_label_metric", + "collect_nonstandard_service_label_metric", RegistrationMetadata::default(), ); @@ -251,7 +251,16 @@ mod tests { service_name_format: ServiceNameFormat::LabelWithName("service:name"), }); - assert!(families.is_empty()); + let family = families + .iter() + .find(|family| { + family.name.as_deref() == Some("collect_nonstandard_service_label_metric") + }) + .expect("metric with a nonstandard service label name should remain"); + assert_eq!( + family.metric[0].label[0], + label("service:name", "test_service") + ); } #[test] @@ -324,7 +333,7 @@ mod tests { } #[test] - fn collection_skips_invalid_duplicate_and_reserved_row_labels() { + fn collection_keeps_nonstandard_names_and_skips_duplicate_and_reserved_labels() { register_test_family(MetricFamily { name: Some("collect_row_validation_gauge".to_owned()), help: None, @@ -336,7 +345,7 @@ mod tests { ..Default::default() }, Metric { - label: vec![label("bad\nname", "invalid")], + label: vec![label("bad\nname", "nonstandard")], gauge: Some(Gauge { value: Some(2.0) }), ..Default::default() }, @@ -371,20 +380,20 @@ mod tests { service_name_format: ServiceNameFormat::MetricPrefix, }); - for name in [ - "collect_row_validation_gauge", - "collect_row_validation_histogram", + for (name, expected_rows) in [ + ("collect_row_validation_gauge", 2), + ("collect_row_validation_histogram", 1), ] { let family = families .iter() .find(|family| family.name.as_deref() == Some(name)) .expect("valid family should remain"); - assert_eq!(family.metric.len(), 1, "family {name}"); + assert_eq!(family.metric.len(), expected_rows, "family {name}"); } } #[test] - fn collection_drops_only_invalid_exemplars() { + fn collection_keeps_nonstandard_exemplar_names_and_drops_duplicates() { register_test_family(MetricFamily { name: Some("collect_counter_exemplar_validation".to_owned()), help: None, @@ -442,13 +451,18 @@ mod tests { .iter() .find(|family| family.name.as_deref() == Some("collect_counter_exemplar_validation")) .expect("counter family should remain"); - assert!( + assert_eq!( counter.metric[0] .counter .as_ref() .unwrap() .exemplar - .is_none() + .as_ref() + .unwrap() + .label[0] + .name + .as_deref(), + Some("trace:id") ); let histogram = families @@ -457,10 +471,14 @@ mod tests { .expect("histogram family should remain"); let histogram = histogram.metric[0].histogram.as_ref().unwrap(); assert!(histogram.bucket[0].exemplar.is_none()); - assert_eq!(histogram.exemplars.len(), 1); + assert_eq!(histogram.exemplars.len(), 2); assert_eq!( - histogram.exemplars[0].label[0].name.as_deref(), - Some("trace_id") + histogram + .exemplars + .iter() + .map(|exemplar| exemplar.label[0].name.as_deref().unwrap()) + .collect::>(), + ["bad name", "trace_id"] ); } } diff --git a/foundations-metrics/src/encoding/mod.rs b/foundations-metrics/src/encoding/mod.rs index 4fa50864..dbd5c12d 100644 --- a/foundations-metrics/src/encoding/mod.rs +++ b/foundations-metrics/src/encoding/mod.rs @@ -5,7 +5,7 @@ use prost::Message; use crate::MetricFamily; use crate::validation::{ValidationContext, sanitized_metric_family}; -pub use text::encode_to_text; +pub use text::{OPENMETRICS_CONTENT_TYPE, encode_to_text}; /// Encodes metric families as length-delimited Prometheus protobuf messages. pub fn encode_to_protobuf(families: &[MetricFamily]) -> Vec { @@ -48,13 +48,13 @@ mod tests { } #[test] - fn fully_valid_protobuf_output_is_unchanged() { + fn utf8_protobuf_output_is_unchanged() { let families = [MetricFamily { - name: Some("valid:counter".to_owned()), + name: Some("valid counter λ".to_owned()), help: Some("Valid counter.".to_owned()), r#type: Some(MetricType::Counter as i32), metric: vec![Metric { - label: vec![label("_label", "value")], + label: vec![label("_label.name λ", "value")], counter: Some(Counter { value: Some(1.0), exemplar: Some(Exemplar::default()), @@ -83,8 +83,18 @@ mod tests { } #[test] - fn protobuf_defensively_omits_invalid_families_and_rows_and_strips_exemplars() { + fn protobuf_keeps_nonstandard_names_and_omits_empty_duplicate_and_reserved_names() { let families = [ + MetricFamily { + name: Some(String::new()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![Metric { + gauge: Some(Gauge { value: Some(100.0) }), + ..Default::default() + }], + unit: None, + }, MetricFamily { name: Some("bad\nfamily".to_owned()), help: None, @@ -105,7 +115,7 @@ mod tests { counter: Some(Counter { value: Some(1.0), exemplar: Some(Exemplar { - label: vec![label("trace:id", "bad")], + label: vec![label("trace:id", "nonstandard")], ..Default::default() }), created_timestamp: None, @@ -113,7 +123,7 @@ mod tests { ..Default::default() }, Metric { - label: vec![label("bad name", "dropped")], + label: vec![label("bad name", "kept_nonstandard")], counter: Some(Counter { value: Some(2.0), ..Default::default() @@ -147,7 +157,7 @@ mod tests { }], exemplars: vec![ Exemplar { - label: vec![label("bad#name", "bad")], + label: vec![label("bad#name", "nonstandard")], ..Default::default() }, Exemplar { @@ -185,29 +195,47 @@ mod tests { .iter() .filter_map(|family| family.name.as_deref()) .collect::>(), - ["protobuf_counter", "protobuf_histogram", "protobuf_sibling",] + [ + "bad\nfamily", + "protobuf_counter", + "protobuf_histogram", + "protobuf_sibling", + ] ); - assert_eq!(decoded[0].metric.len(), 1); - assert!( - decoded[0].metric[0] + assert_eq!(decoded[1].metric.len(), 2); + assert_eq!( + decoded[1].metric[0] .counter .as_ref() .unwrap() .exemplar - .is_none() + .as_ref() + .unwrap() + .label[0] + .name + .as_deref(), + Some("trace:id") + ); + assert_eq!( + decoded[1].metric[1].label[0].name.as_deref(), + Some("bad name") ); - assert_eq!(decoded[1].metric.len(), 1); - let histogram = decoded[1].metric[0].histogram.as_ref().unwrap(); + assert_eq!(decoded[2].metric.len(), 1); + let histogram = decoded[2].metric[0].histogram.as_ref().unwrap(); assert!(histogram.bucket[0].exemplar.is_none()); - assert_eq!(histogram.exemplars.len(), 1); + assert_eq!(histogram.exemplars.len(), 2); assert_eq!( - histogram.exemplars[0].label[0].name.as_deref(), - Some("trace_id") + histogram + .exemplars + .iter() + .map(|exemplar| exemplar.label[0].name.as_deref().unwrap()) + .collect::>(), + ["bad#name", "trace_id"] ); - assert_eq!(decoded[2].metric.len(), 1); + assert_eq!(decoded[3].metric.len(), 1); } #[test] diff --git a/foundations-metrics/src/encoding/text.rs b/foundations-metrics/src/encoding/text.rs index 71f92c6c..481e0693 100644 --- a/foundations-metrics/src/encoding/text.rs +++ b/foundations-metrics/src/encoding/text.rs @@ -7,7 +7,15 @@ use foundations_metrics_registry::proto::{ use crate::diagnostics::report_collect_error; use crate::validation::{ValidationContext, sanitized_metric_family}; -/// Encodes metric families as OpenMetrics text. +/// Content type for the UTF-8 OpenMetrics text emitted by [`encode_to_text`]. +pub const OPENMETRICS_CONTENT_TYPE: &str = + "application/openmetrics-text; version=1.0.0; charset=utf-8; escaping=allow-utf-8"; + +/// Encodes metric families as UTF-8 OpenMetrics text. +/// +/// Label names are always quoted. Metric names outside the legacy Prometheus +/// grammar use the quoted metric-name form. Serve the output with +/// [`OPENMETRICS_CONTENT_TYPE`] so scrapers retain UTF-8 names. pub fn encode_to_text(families: &[MetricFamily]) -> String { let mut output = String::new(); @@ -48,21 +56,21 @@ fn encode_family(output: &mut String, family: &MetricFamily) { // than written with a blank value. if let Some(help) = family.help.as_deref().filter(|help| !help.is_empty()) { output.push_str("# HELP "); - output.push_str(name); + write_metadata_name(output, name); output.push(' '); write_escaped(output, help); output.push('\n'); } output.push_str("# TYPE "); - output.push_str(name); + write_metadata_name(output, name); output.push(' '); output.push_str(metric_type_name); output.push('\n'); if let Some(unit) = &family.unit { output.push_str("# UNIT "); - output.push_str(name); + write_metadata_name(output, name); output.push(' '); write_escaped(output, unit); output.push('\n'); @@ -280,9 +288,7 @@ fn write_sample( value: SampleValue, exemplar: Option<&Exemplar>, ) { - output.push_str(name); - output.push_str(suffix); - write_labels(output, &metric.label, additional_label); + write_sample_name_and_labels(output, name, suffix, &metric.label, additional_label); output.push(' '); match value { SampleValue::Float(value) => write_float(output, value), @@ -323,7 +329,7 @@ fn write_labels(output: &mut String, labels: &[LabelPair], additional_label: Opt let mut separator = ""; for label in labels { output.push_str(separator); - output.push_str(label.name.as_deref().unwrap_or_default()); + write_quoted(output, label.name.as_deref().unwrap_or_default()); output.push_str("=\""); write_escaped(output, label.value.as_deref().unwrap_or_default()); output.push('"'); @@ -332,7 +338,46 @@ fn write_labels(output: &mut String, labels: &[LabelPair], additional_label: Opt if let Some((name, value)) = additional_label { output.push_str(separator); + write_quoted(output, name); + output.push_str("=\""); + write_float(output, value); + output.push('"'); + } + + output.push('}'); +} + +fn write_sample_name_and_labels( + output: &mut String, + name: &str, + suffix: &str, + labels: &[LabelPair], + additional_label: Option<(&str, f64)>, +) { + if is_legacy_metric_name(name) { output.push_str(name); + output.push_str(suffix); + write_labels(output, labels, additional_label); + return; + } + + output.push('{'); + output.push('"'); + write_escaped(output, name); + output.push_str(suffix); + output.push('"'); + + for label in labels { + output.push(','); + write_quoted(output, label.name.as_deref().unwrap_or_default()); + output.push_str("=\""); + write_escaped(output, label.value.as_deref().unwrap_or_default()); + output.push('"'); + } + + if let Some((name, value)) = additional_label { + output.push(','); + write_quoted(output, name); output.push_str("=\""); write_float(output, value); output.push('"'); @@ -341,6 +386,28 @@ fn write_labels(output: &mut String, labels: &[LabelPair], additional_label: Opt output.push('}'); } +fn write_metadata_name(output: &mut String, name: &str) { + if is_legacy_metric_name(name) { + output.push_str(name); + } else { + write_quoted(output, name); + } +} + +fn is_legacy_metric_name(name: &str) -> bool { + let mut bytes = name.bytes(); + bytes + .next() + .is_some_and(|byte| byte.is_ascii_alphabetic() || matches!(byte, b'_' | b':')) + && bytes.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b':')) +} + +fn write_quoted(output: &mut String, value: &str) { + output.push('"'); + write_escaped(output, value); + output.push('"'); +} + fn write_float(output: &mut String, value: f64) { if value.is_nan() { output.push_str("NaN"); @@ -448,7 +515,7 @@ requests 1.0\n\ encode_to_text(&families), "# HELP requests A \\\"quoted\\\" help\\\\line\\nnext\n\ # TYPE requests counter\n\ -requests{kind=\"a\\\"b\\\\c\\nd\"} 1.0 1.5 # {trace_id=\"abc\"} 2.0\n\ +requests{\"kind\"=\"a\\\"b\\\\c\\nd\"} 1.0 1.5 # {\"trace_id\"=\"abc\"} 2.0\n\ # EOF\n" ); } @@ -491,10 +558,10 @@ requests{kind=\"a\\\"b\\\\c\\nd\"} 1.0 1.5 # {trace_id=\"abc\"} 2.0\n\ "# HELP request_duration_seconds Request duration.\n\ # TYPE request_duration_seconds histogram\n\ # UNIT request_duration_seconds seconds\n\ -request_duration_seconds_sum{route=\"/test\"} 4.5\n\ -request_duration_seconds_count{route=\"/test\"} 3\n\ -request_duration_seconds_bucket{route=\"/test\",le=\"1.0\"} 1\n\ -request_duration_seconds_bucket{route=\"/test\",le=\"+Inf\"} 3\n\ +request_duration_seconds_sum{\"route\"=\"/test\"} 4.5\n\ +request_duration_seconds_count{\"route\"=\"/test\"} 3\n\ +request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"1.0\"} 1\n\ +request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"+Inf\"} 3\n\ # EOF\n" ); } @@ -522,7 +589,7 @@ request_duration_seconds_bucket{route=\"/test\",le=\"+Inf\"} 3\n\ }]; let output = encode_to_text(&families); - assert!(output.contains("values_bucket{le=\"+Inf\"} 2\n")); + assert!(output.contains("values_bucket{\"le\"=\"+Inf\"} 2\n")); } #[test] @@ -629,19 +696,63 @@ temperature -Inf\n\ encode_to_text(&families), "# HELP build_info Build information.\n\ # TYPE build_info gauge\n\ -build_info{version=\"1.2.3\"} 1.0\n\ +build_info{\"version\"=\"1.2.3\"} 1.0\n\ +# EOF\n" + ); + } + + #[test] + fn appends_histogram_suffixes_inside_quoted_metric_names() { + let families = [MetricFamily { + name: Some("request.耗时".to_owned()), + help: None, + r#type: Some(MetricType::Histogram as i32), + metric: vec![Metric { + histogram: Some(Histogram { + sample_count: Some(1), + sample_sum: Some(0.5), + ..Default::default() + }), + ..Default::default() + }], + unit: None, + }]; + + assert_eq!( + encode_to_text(&families), + "# TYPE \"request.耗时\" histogram\n\ +{\"request.耗时_sum\"} 0.5\n\ +{\"request.耗时_count\"} 1\n\ +{\"request.耗时_bucket\",\"le\"=\"+Inf\"} 1\n\ # EOF\n" ); } #[test] - fn invalid_family_names_cannot_inject_metadata_and_valid_siblings_remain() { + fn skips_empty_metric_names() { + let families = [MetricFamily { + name: Some(String::new()), + help: None, + r#type: Some(MetricType::Gauge as i32), + metric: vec![Metric { + gauge: Some(Gauge { value: Some(1.0) }), + ..Default::default() + }], + unit: None, + }]; + + assert_eq!(encode_to_text(&families), "# EOF\n"); + } + + #[test] + fn quotes_and_escapes_utf8_metric_and_label_names() { let families = [ MetricFamily { name: Some("bad\n# HELP injected metadata".to_owned()), - help: Some("should not be written".to_owned()), + help: Some("Escaped help.".to_owned()), r#type: Some(MetricType::Gauge as i32), metric: vec![Metric { + label: vec![label("路由.name\n", "值")], gauge: Some(Gauge { value: Some(99.0) }), ..Default::default() }], @@ -661,12 +772,17 @@ build_info{version=\"1.2.3\"} 1.0\n\ assert_eq!( encode_to_text(&families), - "# TYPE valid:metric gauge\nvalid:metric 1.0\n# EOF\n" + "# HELP \"bad\\n# HELP injected metadata\" Escaped help.\n\ +# TYPE \"bad\\n# HELP injected metadata\" gauge\n\ +{\"bad\\n# HELP injected metadata\",\"路由.name\\n\"=\"值\"} 99.0\n\ +# TYPE valid:metric gauge\n\ +valid:metric 1.0\n\ +# EOF\n" ); } #[test] - fn invalid_duplicate_and_reserved_row_labels_skip_only_their_rows() { + fn nonstandard_labels_are_kept_while_duplicate_and_reserved_labels_are_dropped() { let families = [ MetricFamily { name: Some("row_gauge".to_owned()), @@ -679,7 +795,7 @@ build_info{version=\"1.2.3\"} 1.0\n\ ..Default::default() }, Metric { - label: vec![label("bad name", "invalid")], + label: vec![label("bad name", "nonstandard")], gauge: Some(Gauge { value: Some(99.0) }), ..Default::default() }, @@ -736,8 +852,8 @@ build_info{version=\"1.2.3\"} 1.0\n\ ]; let output = encode_to_text(&families); - assert!(output.contains("row_gauge{id=\"valid\"} 1.0\n")); - assert!(!output.contains("99.0")); + assert!(output.contains("row_gauge{\"id\"=\"valid\"} 1.0\n")); + assert!(output.contains("row_gauge{\"bad name\"=\"nonstandard\"} 99.0\n")); assert!(!output.contains("98.0")); assert_eq!(output.matches("row_histogram_sum").count(), 1); assert_eq!(output.matches("row_summary_sum").count(), 1); @@ -745,7 +861,7 @@ build_info{version=\"1.2.3\"} 1.0\n\ } #[test] - fn invalid_exemplar_labels_drop_only_the_exemplar() { + fn nonstandard_exemplar_labels_are_kept_while_duplicates_are_dropped() { let families = [ MetricFamily { name: Some("exemplar_counter".to_owned()), @@ -753,7 +869,7 @@ build_info{version=\"1.2.3\"} 1.0\n\ r#type: Some(MetricType::Counter as i32), metric: vec![ Metric { - label: vec![label("id", "invalid")], + label: vec![label("id", "nonstandard")], counter: Some(Counter { value: Some(1.0), exemplar: Some(Exemplar { @@ -808,10 +924,14 @@ build_info{version=\"1.2.3\"} 1.0\n\ ]; let output = encode_to_text(&families); - assert!(output.contains("exemplar_counter{id=\"invalid\"} 1.0\n")); - assert!(output.contains("exemplar_counter{id=\"valid\"} 3.0 # {trace_id=\"good\"} 4.0\n")); - assert!(output.contains("exemplar_histogram_bucket{le=\"1.0\"} 1\n")); - assert!(!output.contains("trace:id")); - assert!(!output.contains("{dup=")); + assert!(output.contains( + "exemplar_counter{\"id\"=\"nonstandard\"} 1.0 # {\"trace:id\"=\"bad\"} 2.0\n" + )); + assert!( + output + .contains("exemplar_counter{\"id\"=\"valid\"} 3.0 # {\"trace_id\"=\"good\"} 4.0\n") + ); + assert!(output.contains("exemplar_histogram_bucket{\"le\"=\"1.0\"} 1\n")); + assert!(!output.contains("\"dup\"=")); } } diff --git a/foundations-metrics/src/labels/serializer.rs b/foundations-metrics/src/labels/serializer.rs index 6615e8f2..780aa058 100644 --- a/foundations-metrics/src/labels/serializer.rs +++ b/foundations-metrics/src/labels/serializer.rs @@ -5,7 +5,7 @@ use serde::Serialize; use serde::ser::{Impossible, SerializeStruct, Serializer}; use super::LabelError; -use crate::validation::{LABEL_NAME_GRAMMAR, is_valid_label_name}; +use crate::validation::{NAME_REQUIREMENT, is_valid_name}; // Adapted from prometools' `serde::top::TopSerializer` // (https://github.com/nox/prometools, licensed MIT OR Apache-2.0). @@ -397,9 +397,9 @@ impl Serializer for LabelValueSerializer { // Adapted from prometools' `serde::top::check_key` // (https://github.com/nox/prometools, licensed MIT OR Apache-2.0). fn validate_label_name(name: &str) -> Result<(), LabelError> { - is_valid_label_name(name).then_some(()).ok_or_else(|| { + is_valid_name(name).then_some(()).ok_or_else(|| { LabelError::new(format!( - "invalid metric label name {name:?}: expected {LABEL_NAME_GRAMMAR}" + "invalid metric label name {name:?}: expected {NAME_REQUIREMENT}" )) }) } @@ -474,10 +474,10 @@ mod tests { } #[test] - fn rejects_invalid_label_names() { + fn rejects_empty_label_names() { #[derive(Serialize)] struct Invalid { - #[serde(rename = "not-valid")] + #[serde(rename = "")] value: &'static str, } @@ -485,18 +485,15 @@ mod tests { } #[test] - fn rejects_colon_label_names_with_the_label_grammar_in_the_error() { + fn serializes_utf8_label_names() { #[derive(Serialize)] - struct Invalid { - #[serde(rename = "trace:id")] + struct Labels { + #[serde(rename = "trace.id λ\n\"")] value: &'static str, } - let error = to_label_pairs(&Invalid { value: "x" }).unwrap_err(); - assert_eq!( - error.to_string(), - "invalid metric label name \"trace:id\": expected [a-zA-Z_][a-zA-Z0-9_]*" - ); + let labels = to_label_pairs(&Labels { value: "x" }).unwrap(); + assert_eq!(labels[0].name.as_deref(), Some("trace.id λ\n\"")); } #[test] diff --git a/foundations-metrics/src/lib.rs b/foundations-metrics/src/lib.rs index affd18b9..0b50e154 100644 --- a/foundations-metrics/src/lib.rs +++ b/foundations-metrics/src/lib.rs @@ -17,7 +17,7 @@ mod value; pub use collect::{CollectionOptions, ServiceNameFormat, collect}; pub use diagnostics::{CollectErrorHookAlreadySet, set_collect_error_hook}; -pub use encoding::{encode_to_protobuf, encode_to_text}; +pub use encoding::{OPENMETRICS_CONTENT_TYPE, encode_to_protobuf, encode_to_text}; pub use foundations_metrics_registry::{ EncodeMetric, IntoMetrics, MetricFamily, RegistrationMetadata, register, }; diff --git a/foundations-metrics/src/validation.rs b/foundations-metrics/src/validation.rs index 7e12640e..4cb8da7c 100644 --- a/foundations-metrics/src/validation.rs +++ b/foundations-metrics/src/validation.rs @@ -4,8 +4,7 @@ use foundations_metrics_registry::proto::{Exemplar, LabelPair, Metric, MetricFam use crate::diagnostics::report_collect_error; -pub(crate) const METRIC_NAME_GRAMMAR: &str = "[a-zA-Z_:][a-zA-Z0-9_:]*"; -pub(crate) const LABEL_NAME_GRAMMAR: &str = "[a-zA-Z_][a-zA-Z0-9_]*"; +pub(crate) const NAME_REQUIREMENT: &str = "a non-empty UTF-8 string without NUL bytes"; #[derive(Clone, Copy)] pub(crate) enum ValidationContext { @@ -24,31 +23,15 @@ impl ValidationContext { } } -pub(crate) fn is_valid_metric_name(name: &str) -> bool { - is_valid_name(name, true) -} - -pub(crate) fn is_valid_label_name(name: &str) -> bool { - is_valid_name(name, false) -} - -fn is_valid_name(name: &str, allow_colon: bool) -> bool { - let mut bytes = name.bytes(); - bytes.next().is_some_and(|byte| { - byte.is_ascii_alphabetic() || byte == b'_' || (allow_colon && byte == b':') - }) && bytes - .all(|byte| byte.is_ascii_alphanumeric() || byte == b'_' || (allow_colon && byte == b':')) +pub(crate) fn is_valid_name(name: &str) -> bool { + !name.is_empty() && !name.contains('\0') } pub(crate) fn sanitize_metric_family( family: &mut MetricFamily, context: ValidationContext, ) -> bool { - let Some(name) = family - .name - .as_deref() - .filter(|name| is_valid_metric_name(name)) - else { + let Some(name) = family.name.as_deref().filter(|name| is_valid_name(name)) else { report_invalid_family_name(context, family.name.as_deref()); return false; }; @@ -68,11 +51,7 @@ pub(crate) fn sanitized_metric_family<'a>( family: &'a MetricFamily, context: ValidationContext, ) -> Option> { - let Some(name) = family - .name - .as_deref() - .filter(|name| is_valid_metric_name(name)) - else { + let Some(name) = family.name.as_deref().filter(|name| is_valid_name(name)) else { report_invalid_family_name(context, family.name.as_deref()); return None; }; @@ -91,7 +70,7 @@ pub(crate) fn sanitized_metric_family<'a>( fn report_invalid_family_name(context: ValidationContext, name: Option<&str>) { report_collect_error(format_args!( - "non-fatal error while {}: skipped metric family with invalid name {name:?}; expected {METRIC_NAME_GRAMMAR}", + "non-fatal error while {}: skipped metric family with invalid name {name:?}; expected {NAME_REQUIREMENT}", context.action() )); } @@ -141,11 +120,7 @@ fn find_label_issue<'a>( reserved_label: Option<&str>, ) -> Option> { for (index, label) in labels.iter().enumerate() { - let Some(name) = label - .name - .as_deref() - .filter(|name| is_valid_label_name(name)) - else { + let Some(name) = label.name.as_deref().filter(|name| is_valid_name(name)) else { return Some(LabelIssue::Invalid(label.name.as_deref())); }; @@ -166,7 +141,7 @@ fn find_label_issue<'a>( fn report_row_drop(context: ValidationContext, family_name: &str, issue: LabelIssue<'_>) { match issue { LabelIssue::Invalid(name) => report_collect_error(format_args!( - "non-fatal error while {}: skipped row in metric family {family_name:?} with invalid label name {name:?}; expected {LABEL_NAME_GRAMMAR}", + "non-fatal error while {}: skipped row in metric family {family_name:?} with invalid label name {name:?}; expected {NAME_REQUIREMENT}", context.action() )), LabelIssue::Duplicate(name) => report_collect_error(format_args!( @@ -249,7 +224,7 @@ fn report_exemplar_drop( ) { match issue { LabelIssue::Invalid(name) => report_collect_error(format_args!( - "non-fatal error while {}: dropped {kind} exemplar in metric family {family_name:?} with invalid label name {name:?}; expected {LABEL_NAME_GRAMMAR}", + "non-fatal error while {}: dropped {kind} exemplar in metric family {family_name:?} with invalid label name {name:?}; expected {NAME_REQUIREMENT}", context.action() )), LabelIssue::Duplicate(name) => report_collect_error(format_args!( @@ -265,68 +240,26 @@ mod tests { use super::*; #[test] - fn exhaustively_checks_ascii_metric_name_characters() { - for byte in 0_u8..=127 { - let character = char::from(byte); - let expected_first = byte.is_ascii_alphabetic() || matches!(byte, b'_' | b':'); - let expected_continuation = byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b':'); - - assert_eq!( - is_valid_metric_name(&character.to_string()), - expected_first, - "metric first character {byte:#04x}" - ); - assert_eq!( - is_valid_metric_name(&format!("a{character}")), - expected_continuation, - "metric continuation character {byte:#04x}" - ); - } - } - - #[test] - fn exhaustively_checks_ascii_label_name_characters() { - for byte in 0_u8..=127 { - let character = char::from(byte); - let expected_first = byte.is_ascii_alphabetic() || byte == b'_'; - let expected_continuation = byte.is_ascii_alphanumeric() || byte == b'_'; - - assert_eq!( - is_valid_label_name(&character.to_string()), - expected_first, - "label first character {byte:#04x}" - ); - assert_eq!( - is_valid_label_name(&format!("a{character}")), - expected_continuation, - "label continuation character {byte:#04x}" - ); - } - } - - #[test] - fn rejects_empty_non_ascii_and_injection_names() { + fn accepts_non_empty_utf8_metric_and_label_names() { for name in [ - "", "é", "aλ", "metric name", "metric\nname", "metric#name", "metric\"name", + "指标.名称", ] { - assert!(!is_valid_metric_name(name), "metric name {name:?}"); - assert!(!is_valid_label_name(name), "label name {name:?}"); + assert!(is_valid_name(name), "metric name {name:?}"); + assert!(is_valid_name(name), "label name {name:?}"); } } #[test] - fn metric_names_allow_colons_but_label_names_do_not() { - for name in [":", ":metric", "metric:name"] { - assert!(is_valid_metric_name(name)); - assert!(!is_valid_label_name(name)); + fn rejects_empty_and_nul_names() { + for name in ["", "nul\0name"] { + assert!(!is_valid_name(name)); + assert!(!is_valid_name(name)); } - assert!(is_valid_metric_name("_metric")); - assert!(is_valid_label_name("_label")); } } From a9e8a8579ebb2bc8ec572d6bdaed81239bed338e Mon Sep 17 00:00:00 2001 From: Ethan Olchik Date: Thu, 23 Jul 2026 20:35:08 +0100 Subject: [PATCH 3/6] fix(validation): remove duplicate assertions --- foundations-metrics/src/validation.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/foundations-metrics/src/validation.rs b/foundations-metrics/src/validation.rs index 4cb8da7c..a3c43dd4 100644 --- a/foundations-metrics/src/validation.rs +++ b/foundations-metrics/src/validation.rs @@ -251,7 +251,6 @@ mod tests { "指标.名称", ] { assert!(is_valid_name(name), "metric name {name:?}"); - assert!(is_valid_name(name), "label name {name:?}"); } } @@ -259,7 +258,6 @@ mod tests { fn rejects_empty_and_nul_names() { for name in ["", "nul\0name"] { assert!(!is_valid_name(name)); - assert!(!is_valid_name(name)); } } } From fa9c305a8e380624d2773a15fea13a50a23f1499 Mon Sep 17 00:00:00 2001 From: Ethan Olchik Date: Fri, 24 Jul 2026 15:42:04 +0100 Subject: [PATCH 4/6] test(metrics): cover validated summary and gauge histogram output --- foundations-metrics/src/encoding/text.rs | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/foundations-metrics/src/encoding/text.rs b/foundations-metrics/src/encoding/text.rs index 481e0693..c5df05a1 100644 --- a/foundations-metrics/src/encoding/text.rs +++ b/foundations-metrics/src/encoding/text.rs @@ -637,14 +637,14 @@ request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"+Inf\"} 3\n\ assert_eq!( encode_to_text(&families), "# TYPE request_size summary\n\ -request_size{quantile=\"0.5\"} 3.0\n\ +request_size{\"quantile\"=\"0.5\"} 3.0\n\ request_size_sum 6.0\n\ request_size_count 2\n\ # TYPE queue_depth gaugehistogram\n\ queue_depth_gsum 8.0\n\ queue_depth_gcount 3\n\ -queue_depth_bucket{le=\"1.0\"} 1\n\ -queue_depth_bucket{le=\"+Inf\"} 3\n\ +queue_depth_bucket{\"le\"=\"1.0\"} 1\n\ +queue_depth_bucket{\"le\"=\"+Inf\"} 3\n\ # EOF\n" ); } @@ -849,6 +849,23 @@ valid:metric 1.0\n\ ], unit: None, }, + MetricFamily { + name: Some("row_gauge_histogram".to_owned()), + help: None, + r#type: Some(MetricType::GaugeHistogram as i32), + metric: vec![ + Metric { + histogram: Some(Histogram::default()), + ..Default::default() + }, + Metric { + label: vec![label("le", "1")], + histogram: Some(Histogram::default()), + ..Default::default() + }, + ], + unit: None, + }, ]; let output = encode_to_text(&families); @@ -857,6 +874,7 @@ valid:metric 1.0\n\ assert!(!output.contains("98.0")); assert_eq!(output.matches("row_histogram_sum").count(), 1); assert_eq!(output.matches("row_summary_sum").count(), 1); + assert_eq!(output.matches("row_gauge_histogram_gsum").count(), 1); assert!(output.ends_with("# EOF\n")); } From c2280fb2b7fa891625ed6da24f8def0f227dd132 Mon Sep 17 00:00:00 2001 From: Ethan Olchik Date: Mon, 27 Jul 2026 16:52:42 +0100 Subject: [PATCH 5/6] fix(metrics): quote label names only when they need UTF-8 syntax --- foundations-metrics/src/encoding/text.rs | 100 ++++++++++++++++++----- 1 file changed, 78 insertions(+), 22 deletions(-) diff --git a/foundations-metrics/src/encoding/text.rs b/foundations-metrics/src/encoding/text.rs index c5df05a1..c3f75b2e 100644 --- a/foundations-metrics/src/encoding/text.rs +++ b/foundations-metrics/src/encoding/text.rs @@ -329,7 +329,7 @@ fn write_labels(output: &mut String, labels: &[LabelPair], additional_label: Opt let mut separator = ""; for label in labels { output.push_str(separator); - write_quoted(output, label.name.as_deref().unwrap_or_default()); + write_label_name(output, label.name.as_deref().unwrap_or_default()); output.push_str("=\""); write_escaped(output, label.value.as_deref().unwrap_or_default()); output.push('"'); @@ -338,7 +338,7 @@ fn write_labels(output: &mut String, labels: &[LabelPair], additional_label: Opt if let Some((name, value)) = additional_label { output.push_str(separator); - write_quoted(output, name); + write_label_name(output, name); output.push_str("=\""); write_float(output, value); output.push('"'); @@ -369,7 +369,7 @@ fn write_sample_name_and_labels( for label in labels { output.push(','); - write_quoted(output, label.name.as_deref().unwrap_or_default()); + write_label_name(output, label.name.as_deref().unwrap_or_default()); output.push_str("=\""); write_escaped(output, label.value.as_deref().unwrap_or_default()); output.push('"'); @@ -377,7 +377,7 @@ fn write_sample_name_and_labels( if let Some((name, value)) = additional_label { output.push(','); - write_quoted(output, name); + write_label_name(output, name); output.push_str("=\""); write_float(output, value); output.push('"'); @@ -402,6 +402,29 @@ fn is_legacy_metric_name(name: &str) -> bool { && bytes.all(|byte| byte.is_ascii_alphanumeric() || matches!(byte, b'_' | b':')) } +/// Whether `name` matches the legacy label name grammar, which unlike metric +/// names does not permit colons. +fn is_legacy_label_name(name: &str) -> bool { + let mut bytes = name.bytes(); + bytes + .next() + .is_some_and(|byte| byte.is_ascii_alphabetic() || byte == b'_') + && bytes.all(|byte| byte.is_ascii_alphanumeric() || byte == b'_') +} + +/// Writes a label name, quoting it only when it needs UTF-8 syntax. +/// +/// Legacy-compatible names are emitted unquoted so that output stays byte +/// identical to the classic Prometheus text format, which collectors that +/// predate UTF-8 name support require. +fn write_label_name(output: &mut String, name: &str) { + if is_legacy_label_name(name) { + output.push_str(name); + } else { + write_quoted(output, name); + } +} + fn write_quoted(output: &mut String, value: &str) { output.push('"'); write_escaped(output, value); @@ -458,6 +481,40 @@ mod tests { } } + #[test] + fn quotes_only_label_names_that_need_utf8_syntax() { + let families = [MetricFamily { + name: Some("requests".to_owned()), + help: None, + r#type: Some(MetricType::Counter as i32), + metric: vec![Metric { + label: vec![ + // Legacy-compatible names stay unquoted, so output remains + // byte identical to the classic Prometheus text format. + label("route", "/test"), + label("_internal9", "yes"), + // Colons are valid in metric names but not in label names. + label("trace:id", "abc"), + label("label.name", "dotted"), + label("indicateur_\u{8017}\u{65f6}", "utf8"), + ], + counter: Some(Counter { + value: Some(1.0), + ..Default::default() + }), + ..Default::default() + }], + unit: None, + }]; + + assert_eq!( + encode_to_text(&families), + "# TYPE requests counter\n\ +requests{route=\"/test\",_internal9=\"yes\",\"trace:id\"=\"abc\",\"label.name\"=\"dotted\",\"indicateur_\u{8017}\u{65f6}\"=\"utf8\"} 1.0\n\ +# EOF\n" + ); + } + #[test] fn omits_the_help_line_when_there_is_no_help_text() { let families = [MetricFamily { @@ -515,7 +572,7 @@ requests 1.0\n\ encode_to_text(&families), "# HELP requests A \\\"quoted\\\" help\\\\line\\nnext\n\ # TYPE requests counter\n\ -requests{\"kind\"=\"a\\\"b\\\\c\\nd\"} 1.0 1.5 # {\"trace_id\"=\"abc\"} 2.0\n\ +requests{kind=\"a\\\"b\\\\c\\nd\"} 1.0 1.5 # {trace_id=\"abc\"} 2.0\n\ # EOF\n" ); } @@ -558,10 +615,10 @@ requests{\"kind\"=\"a\\\"b\\\\c\\nd\"} 1.0 1.5 # {\"trace_id\"=\"abc\"} 2.0\n\ "# HELP request_duration_seconds Request duration.\n\ # TYPE request_duration_seconds histogram\n\ # UNIT request_duration_seconds seconds\n\ -request_duration_seconds_sum{\"route\"=\"/test\"} 4.5\n\ -request_duration_seconds_count{\"route\"=\"/test\"} 3\n\ -request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"1.0\"} 1\n\ -request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"+Inf\"} 3\n\ +request_duration_seconds_sum{route=\"/test\"} 4.5\n\ +request_duration_seconds_count{route=\"/test\"} 3\n\ +request_duration_seconds_bucket{route=\"/test\",le=\"1.0\"} 1\n\ +request_duration_seconds_bucket{route=\"/test\",le=\"+Inf\"} 3\n\ # EOF\n" ); } @@ -589,7 +646,7 @@ request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"+Inf\"} 3\n\ }]; let output = encode_to_text(&families); - assert!(output.contains("values_bucket{\"le\"=\"+Inf\"} 2\n")); + assert!(output.contains("values_bucket{le=\"+Inf\"} 2\n")); } #[test] @@ -637,14 +694,14 @@ request_duration_seconds_bucket{\"route\"=\"/test\",\"le\"=\"+Inf\"} 3\n\ assert_eq!( encode_to_text(&families), "# TYPE request_size summary\n\ -request_size{\"quantile\"=\"0.5\"} 3.0\n\ +request_size{quantile=\"0.5\"} 3.0\n\ request_size_sum 6.0\n\ request_size_count 2\n\ # TYPE queue_depth gaugehistogram\n\ queue_depth_gsum 8.0\n\ queue_depth_gcount 3\n\ -queue_depth_bucket{\"le\"=\"1.0\"} 1\n\ -queue_depth_bucket{\"le\"=\"+Inf\"} 3\n\ +queue_depth_bucket{le=\"1.0\"} 1\n\ +queue_depth_bucket{le=\"+Inf\"} 3\n\ # EOF\n" ); } @@ -696,7 +753,7 @@ temperature -Inf\n\ encode_to_text(&families), "# HELP build_info Build information.\n\ # TYPE build_info gauge\n\ -build_info{\"version\"=\"1.2.3\"} 1.0\n\ +build_info{version=\"1.2.3\"} 1.0\n\ # EOF\n" ); } @@ -723,7 +780,7 @@ build_info{\"version\"=\"1.2.3\"} 1.0\n\ "# TYPE \"request.耗时\" histogram\n\ {\"request.耗时_sum\"} 0.5\n\ {\"request.耗时_count\"} 1\n\ -{\"request.耗时_bucket\",\"le\"=\"+Inf\"} 1\n\ +{\"request.耗时_bucket\",le=\"+Inf\"} 1\n\ # EOF\n" ); } @@ -869,7 +926,7 @@ valid:metric 1.0\n\ ]; let output = encode_to_text(&families); - assert!(output.contains("row_gauge{\"id\"=\"valid\"} 1.0\n")); + assert!(output.contains("row_gauge{id=\"valid\"} 1.0\n")); assert!(output.contains("row_gauge{\"bad name\"=\"nonstandard\"} 99.0\n")); assert!(!output.contains("98.0")); assert_eq!(output.matches("row_histogram_sum").count(), 1); @@ -942,14 +999,13 @@ valid:metric 1.0\n\ ]; let output = encode_to_text(&families); - assert!(output.contains( - "exemplar_counter{\"id\"=\"nonstandard\"} 1.0 # {\"trace:id\"=\"bad\"} 2.0\n" - )); assert!( - output - .contains("exemplar_counter{\"id\"=\"valid\"} 3.0 # {\"trace_id\"=\"good\"} 4.0\n") + output.contains( + "exemplar_counter{id=\"nonstandard\"} 1.0 # {\"trace:id\"=\"bad\"} 2.0\n" + ) ); - assert!(output.contains("exemplar_histogram_bucket{\"le\"=\"1.0\"} 1\n")); + assert!(output.contains("exemplar_counter{id=\"valid\"} 3.0 # {trace_id=\"good\"} 4.0\n")); + assert!(output.contains("exemplar_histogram_bucket{le=\"1.0\"} 1\n")); assert!(!output.contains("\"dup\"=")); } } From 843e002f3b4ea1d276b26764bb47bb46c6cef7d5 Mon Sep 17 00:00:00 2001 From: Ethan Olchik Date: Fri, 31 Jul 2026 13:07:27 +0100 Subject: [PATCH 6/6] refactor(metrics): extract service name application into separate functions. Also replace retain_mut for filter_map --- foundations-metrics/src/collect.rs | 107 +++++++++++++++++------------ 1 file changed, 63 insertions(+), 44 deletions(-) diff --git a/foundations-metrics/src/collect.rs b/foundations-metrics/src/collect.rs index 0bec3e74..a3c55e6f 100644 --- a/foundations-metrics/src/collect.rs +++ b/foundations-metrics/src/collect.rs @@ -54,57 +54,76 @@ pub fn collect(options: CollectionOptions) -> Vec { let mut families = registered.metric().encode(); if let Some(service_name) = options.service_name { - match options.service_name_format { - ServiceNameFormat::MetricPrefix if !metadata.unprefixed => { - for family in &mut families { - if let Some(name) = &mut family.name { - name.insert(0, '_'); - name.insert_str(0, service_name); - } - } - } - ServiceNameFormat::LabelWithName(label_name) => { - let service_label = LabelPair { - name: Some(label_name.to_owned()), - value: Some(service_name.to_owned()), - }; - - for family in &mut families { - let family_name = family.name.as_deref().unwrap_or_default(); - family.metric.retain_mut(|metric| { - let mut has_same_value = false; - for label in &metric.label { - if label.name.as_deref() != Some(label_name) { - continue; - } - - if label.value.as_deref() != Some(service_name) { - report_collect_error(format_args!( - "non-fatal error while collecting metrics: skipped row in metric family {family_name:?}; service label {label_name:?} already has a different value" - )); - return false; - } - has_same_value = true; - } - - if !has_same_value { - metric.label.insert(0, service_label.clone()); - } - true - }); - } - } - ServiceNameFormat::MetricPrefix => {} - } + apply_service_name( + &mut families, + service_name, + options.service_name_format, + metadata.unprefixed, + ); } - families.retain_mut(|family| sanitize_metric_family(family, ValidationContext::Collection)); - collected.extend(families); + collected.extend(families.into_iter().filter_map(|mut family| { + sanitize_metric_family(&mut family, ValidationContext::Collection).then_some(family) + })); } collected } +fn apply_service_name( + families: &mut [MetricFamily], + service_name: &str, + format: ServiceNameFormat<'_>, + unprefixed: bool, +) { + match format { + ServiceNameFormat::MetricPrefix if !unprefixed => { + for family in families { + if let Some(name) = &mut family.name { + name.insert(0, '_'); + name.insert_str(0, service_name); + } + } + } + ServiceNameFormat::LabelWithName(label_name) => { + apply_service_label(families, label_name, service_name); + } + ServiceNameFormat::MetricPrefix => {} + } +} + +fn apply_service_label(families: &mut [MetricFamily], label_name: &str, service_name: &str) { + let service_label = LabelPair { + name: Some(label_name.to_owned()), + value: Some(service_name.to_owned()), + }; + + for family in families { + let family_name = family.name.as_deref().unwrap_or_default(); + family.metric.retain_mut(|metric| { + let mut has_same_value = false; + for label in &metric.label { + if label.name.as_deref() != Some(label_name) { + continue; + } + + if label.value.as_deref() != Some(service_name) { + report_collect_error(format_args!( + "non-fatal error while collecting metrics: skipped row in metric family {family_name:?}; service label {label_name:?} already has a different value" + )); + return false; + } + has_same_value = true; + } + + if !has_same_value { + metric.label.insert(0, service_label.clone()); + } + true + }); + } +} + #[cfg(test)] mod tests { use foundations_metrics_registry::proto::{