diff --git a/.changelog/5340.fixed b/.changelog/5340.fixed new file mode 100644 index 0000000000..eb1ba3b8a0 --- /dev/null +++ b/.changelog/5340.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: raise `ValueError` when `ExplicitBucketHistogramAggregation` boundaries are not strictly increasing or finite diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index dea213f428..9c12e1a3cd 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -9,7 +9,7 @@ from enum import IntEnum from functools import partial from logging import getLogger -from math import inf +from math import inf, isnan from threading import Lock from typing import ( Generic, @@ -464,6 +464,12 @@ def __init__( boundaries = ( _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES ) + if boundaries: + for i, x in enumerate(boundaries): + if not math.isfinite(x): + raise ValueError(f"invalid boundary: {x!r}") + if i and boundaries[i - 1] >= x: + raise ValueError("boundaries must be strictly increasing") super().__init__( attributes, reservoir_builder=partial( diff --git a/opentelemetry-sdk/tests/metrics/test_aggregation.py b/opentelemetry-sdk/tests/metrics/test_aggregation.py index 1f11393cee..12a42b3177 100644 --- a/opentelemetry-sdk/tests/metrics/test_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/test_aggregation.py @@ -477,6 +477,66 @@ def test_create_aggregation_on_instrument_without_boundaries(self): ) self.assertIsInstance(result, _ExplicitBucketHistogramAggregation) + def test_unsorted_boundaries_raise(self): + with self.assertRaises(ValueError): + _ExplicitBucketHistogramAggregation( + Mock(), + AggregationTemporality.DELTA, + 0, + _default_reservoir_factory( + _ExplicitBucketHistogramAggregation + ), + boundaries=[100, 10, 50], + ) + + def test_duplicate_boundaries_raise(self): + with self.assertRaises(ValueError): + _ExplicitBucketHistogramAggregation( + Mock(), + AggregationTemporality.DELTA, + 0, + _default_reservoir_factory( + _ExplicitBucketHistogramAggregation + ), + boundaries=[10, 50, 50, 100], + ) + + def test_nan_boundary_raises(self): + with self.assertRaises(ValueError): + _ExplicitBucketHistogramAggregation( + Mock(), + AggregationTemporality.DELTA, + 0, + _default_reservoir_factory( + _ExplicitBucketHistogramAggregation + ), + boundaries=[10, float("nan"), 100], + ) + + def test_inf_boundary_raises(self): + with self.assertRaises(ValueError): + _ExplicitBucketHistogramAggregation( + Mock(), + AggregationTemporality.DELTA, + 0, + _default_reservoir_factory( + _ExplicitBucketHistogramAggregation + ), + boundaries=[10, 50, float("inf")], + ) + + def test_negative_inf_boundary_raises(self): + with self.assertRaises(ValueError): + _ExplicitBucketHistogramAggregation( + Mock(), + AggregationTemporality.DELTA, + 0, + _default_reservoir_factory( + _ExplicitBucketHistogramAggregation + ), + boundaries=[float("-inf"), 50, 100], + ) + class TestAggregationFactory(TestCase): def test_sum_factory(self):