Fix max_scale validation of exponential histogram configuration (open…

…-telemetry#1452) `metrics::Aggregation::validate()` has a bug that limits `max_scale` of a `Base2ExponentialHistogram` to the interval `[10, 20]` instead of the expected `[-10, 20]`.
MaterializeInc · Jan 4, 2024 · 164c13b · 164c13b
1 parent 43cd4ac
commit 164c13b
Show file tree

Hide file tree

Showing 2 changed files with 88 additions and 1 deletion.
diff --git a/opentelemetry-sdk/CHANGELOG.md b/opentelemetry-sdk/CHANGELOG.md
@@ -3,6 +3,7 @@
 ## vNext
 
 - Fix delta aggregation metric reuse. (#1434)
+- Fix `max_scale` validation of exponential histogram configuration. (#1452)
 
 ## v0.21.1
 

diff --git a/opentelemetry-sdk/src/metrics/aggregation.rs b/opentelemetry-sdk/src/metrics/aggregation.rs
@@ -134,7 +134,7 @@ impl Aggregation {
                         max_scale,
                     )));
                 }
-                if *max_scale < -EXPO_MIN_SCALE {
+                if *max_scale < EXPO_MIN_SCALE {
                     return Err(MetricsError::Config(format!(
                         "aggregation: exponential histogram: max scale ({}) is less than -10",
                         max_scale,
@@ -146,3 +146,89 @@ impl Aggregation {
         }
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::metrics::{
+        internal::{EXPO_MAX_SCALE, EXPO_MIN_SCALE},
+        Aggregation,
+    };
+    use opentelemetry::metrics::{MetricsError, Result};
+
+    #[test]
+    fn validate_aggregation() {
+        struct TestCase {
+            name: &'static str,
+            input: Aggregation,
+            check: Box<dyn Fn(Result<()>) -> bool>,
+        }
+        let ok = Box::new(|result: Result<()>| result.is_ok());
+        let config_error = Box::new(|result| matches!(result, Err(MetricsError::Config(_))));
+
+        let test_cases: Vec<TestCase> = vec![
+            TestCase {
+                name: "base2 histogram with maximum max_scale",
+                input: Aggregation::Base2ExponentialHistogram {
+                    max_size: 160,
+                    max_scale: EXPO_MAX_SCALE,
+                    record_min_max: true,
+                },
+                check: ok.clone(),
+            },
+            TestCase {
+                name: "base2 histogram with minimum max_scale",
+                input: Aggregation::Base2ExponentialHistogram {
+                    max_size: 160,
+                    max_scale: EXPO_MIN_SCALE,
+                    record_min_max: true,
+                },
+                check: ok.clone(),
+            },
+            TestCase {
+                name: "base2 histogram with max_scale too small",
+                input: Aggregation::Base2ExponentialHistogram {
+                    max_size: 160,
+                    max_scale: EXPO_MIN_SCALE - 1,
+                    record_min_max: true,
+                },
+                check: config_error.clone(),
+            },
+            TestCase {
+                name: "base2 histogram with max_scale too big",
+                input: Aggregation::Base2ExponentialHistogram {
+                    max_size: 160,
+                    max_scale: EXPO_MAX_SCALE + 1,
+                    record_min_max: true,
+                },
+                check: config_error.clone(),
+            },
+            TestCase {
+                name: "explicit histogram with one boundary",
+                input: Aggregation::ExplicitBucketHistogram {
+                    boundaries: vec![0.0],
+                    record_min_max: true,
+                },
+                check: ok.clone(),
+            },
+            TestCase {
+                name: "explicit histogram with monotonic boundaries",
+                input: Aggregation::ExplicitBucketHistogram {
+                    boundaries: vec![0.0, 2.0, 4.0, 8.0],
+                    record_min_max: true,
+                },
+                check: ok.clone(),
+            },
+            TestCase {
+                name: "explicit histogram with non-monotonic boundaries",
+                input: Aggregation::ExplicitBucketHistogram {
+                    boundaries: vec![2.0, 0.0, 4.0, 8.0],
+                    record_min_max: true,
+                },
+                check: config_error.clone(),
+            },
+        ];
+        for test in test_cases {
+            assert!((test.check)(test.input.validate()), "{}", test.name)
+        }
+    }
+}