Bugfix - add validation for custom buckets provided for Histograms (#2351)

Co-authored-by: Utkarsh Umesan Pillai <[email protected]>

Author: cijothomas

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

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<Cow<'static, str>>) -> 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(

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)]

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<f64>) -> Self {
         self.boundaries = Some(boundaries);
         self