diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md index 9c298a2a03..c63ebd6a22 100644 --- a/opentelemetry-sdk/CHANGELOG.md +++ b/opentelemetry-sdk/CHANGELOG.md @@ -32,6 +32,13 @@ - These methods are intended for log appenders. Keep the clone of the provider handle, instead of depending on above methods. + - **Bug Fix:** Validates the `with_boundaries` bucket boundaries used in + Histograms. The boundaries provided by the user must not contain `f64::NAN`, + `f64::INFINITY` or `f64::NEG_INFINITY` and must be sorted in strictly + increasing order, and contain no duplicates. Instruments will not record + measurements if the boundaries are invalid. + [#2351](https://github.com/open-telemetry/opentelemetry-rust/pull/2351) + ## 0.27.0 Released 2024-Nov-11 diff --git a/opentelemetry-sdk/src/metrics/meter.rs b/opentelemetry-sdk/src/metrics/meter.rs index 3c3ad37352..e644623e34 100644 --- a/opentelemetry-sdk/src/metrics/meter.rs +++ b/opentelemetry-sdk/src/metrics/meter.rs @@ -394,6 +394,23 @@ impl SdkMeter { return Histogram::new(Arc::new(NoopSyncInstrument::new())); } + if let Some(ref boundaries) = builder.boundaries { + let validation_result = validate_bucket_boundaries(boundaries); + if let Err(err) = validation_result { + // TODO: Include the buckets too in the error message. + // TODO: This validation is not done when Views are used to + // provide boundaries, and that should be fixed. + otel_error!( + name: "InstrumentCreationFailed", + meter_name = self.scope.name(), + instrument_name = builder.name.as_ref(), + message = "Measurements from this Histogram will be ignored.", + reason = format!("{}", err) + ); + return Histogram::new(Arc::new(NoopSyncInstrument::new())); + } + } + match resolver .lookup( InstrumentKind::Histogram, @@ -533,6 +550,28 @@ fn validate_instrument_config(name: &str, unit: &Option>) -> M validate_instrument_name(name).and_then(|_| validate_instrument_unit(unit)) } +fn validate_bucket_boundaries(boundaries: &[f64]) -> MetricResult<()> { + // Validate boundaries do not contain f64::NAN, f64::INFINITY, or f64::NEG_INFINITY + for boundary in boundaries { + if boundary.is_nan() || boundary.is_infinite() { + return Err(MetricError::InvalidInstrumentConfiguration( + "Bucket boundaries must not contain NaN, +Inf, or -Inf", + )); + } + } + + // validate that buckets are sorted and non-duplicate + for i in 1..boundaries.len() { + if boundaries[i] <= boundaries[i - 1] { + return Err(MetricError::InvalidInstrumentConfiguration( + "Bucket boundaries must be sorted and non-duplicate", + )); + } + } + + Ok(()) +} + fn validate_instrument_name(name: &str) -> MetricResult<()> { if name.is_empty() { return Err(MetricError::InvalidInstrumentConfiguration( diff --git a/opentelemetry-sdk/src/metrics/mod.rs b/opentelemetry-sdk/src/metrics/mod.rs index 22aaba95e5..3d2d8af74e 100644 --- a/opentelemetry-sdk/src/metrics/mod.rs +++ b/opentelemetry-sdk/src/metrics/mod.rs @@ -178,6 +178,29 @@ mod tests { // As instrument name is invalid, no metrics should be exported test_context.check_no_metrics(); } + + let invalid_bucket_boundaries = vec![ + vec![1.0, 1.0], // duplicate boundaries + vec![1.0, 2.0, 3.0, 2.0], // duplicate non consequent boundaries + vec![1.0, 2.0, 3.0, 4.0, 2.5], // unsorted boundaries + vec![1.0, 2.0, 3.0, f64::INFINITY, 4.0], // boundaries with positive infinity + vec![1.0, 2.0, 3.0, f64::NAN], // boundaries with NaNs + vec![f64::NEG_INFINITY, 2.0, 3.0], // boundaries with negative infinity + ]; + for bucket_boundaries in invalid_bucket_boundaries { + let test_context = TestContext::new(Temporality::Cumulative); + let histogram = test_context + .meter() + .f64_histogram("test") + .with_boundaries(bucket_boundaries) + .build(); + histogram.record(1.9, &[]); + test_context.flush_metrics(); + + // As bucket boundaires provided via advisory params are invalid, no + // metrics should be exported + test_context.check_no_metrics(); + } } #[tokio::test(flavor = "multi_thread", worker_threads = 1)] diff --git a/opentelemetry/src/metrics/instruments/mod.rs b/opentelemetry/src/metrics/instruments/mod.rs index ca6de9ff95..cb06e21b9f 100644 --- a/opentelemetry/src/metrics/instruments/mod.rs +++ b/opentelemetry/src/metrics/instruments/mod.rs @@ -87,7 +87,26 @@ impl<'a, T> HistogramBuilder<'a, T> { /// /// Setting boundaries is optional. By default, the boundaries are set to: /// - /// `[0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 750.0, 1000.0, 2500.0, 5000.0, 7500.0, 10000.0]` + /// `[0.0, 5.0, 10.0, 25.0, 50.0, 75.0, 100.0, 250.0, 500.0, 750.0, 1000.0, + /// 2500.0, 5000.0, 7500.0, 10000.0]` + /// + /// # Notes + /// - Boundaries must not contain `f64::NAN`, `f64::INFINITY` or + /// `f64::NEG_INFINITY` + /// - Values must be in strictly increasing order (e.g., each value must be + /// greater than the previous). + /// - Boundaries must not contain duplicate values. + /// + /// If invalid boundaries are provided, the instrument will not report + /// measurements. + /// Providing an empty `vec![]` means no bucket information will be + /// calculated. + /// + /// # Warning + /// Using more buckets can improve the accuracy of percentile calculations in backends. + /// However, this comes at a cost, including increased memory, CPU, and network usage. + /// Choose the number of buckets carefully, considering your application's performance + /// and resource requirements. pub fn with_boundaries(mut self, boundaries: Vec) -> Self { self.boundaries = Some(boundaries); self