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..ec1da4bcd6 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -3,13 +3,13 @@ # pylint: disable=too-many-lines +import math from abc import ABC, abstractmethod from bisect import bisect_left from collections.abc import Callable, Sequence from enum import IntEnum from functools import partial from logging import getLogger -from math import inf from threading import Lock from typing import ( Generic, @@ -464,6 +464,12 @@ def __init__( boundaries = ( _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES ) + if boundaries: + for idx, bound in enumerate(boundaries): + if not math.isfinite(bound): + raise ValueError(f"invalid boundary: {bound!r}") + if idx and boundaries[idx - 1] >= bound: + raise ValueError("boundaries must be strictly increasing") super().__init__( attributes, reservoir_builder=partial( @@ -479,13 +485,13 @@ def __init__( self._record_min_max = record_min_max self._value = None - self._min = inf - self._max = -inf + self._min = math.inf + self._max = -math.inf self._sum = 0 self._previous_value = None - self._previous_min = inf - self._previous_max = -inf + self._previous_min = math.inf + self._previous_max = -math.inf self._previous_sum = 0 self._previous_collection_start_nano = self._start_time_unix_nano @@ -529,8 +535,8 @@ def collect( self._value = None self._sum = 0 - self._min = inf - self._max = -inf + self._min = math.inf + self._max = -math.inf if ( self._instrument_aggregation_temporality @@ -674,8 +680,8 @@ def __init__( self._value_positive = None self._value_negative = None - self._min = inf - self._max = -inf + self._min = math.inf + self._max = -math.inf self._sum = 0 self._count = 0 self._zero_count = 0 @@ -683,8 +689,8 @@ def __init__( self._previous_value_positive = None self._previous_value_negative = None - self._previous_min = inf - self._previous_max = -inf + self._previous_min = math.inf + self._previous_max = -math.inf self._previous_sum = 0 self._previous_count = 0 self._previous_zero_count = 0 @@ -830,8 +836,8 @@ def collect( self._value_positive = None self._value_negative = None self._sum = 0 - self._min = inf - self._max = -inf + self._min = math.inf + self._max = -math.inf self._count = 0 self._zero_count = 0 self._scale = None 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):