diff --git a/.changelog/5845.fixed b/.changelog/5845.fixed new file mode 100644 index 0000000000..3132d675ed --- /dev/null +++ b/.changelog/5845.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: validate `explicit_bucket_boundaries_advisory` for finite, strictly increasing values in `create_histogram`, instead of only at the first `record()` call diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py index 2a43dc299c..f6c9696267 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/__init__.py @@ -1,6 +1,7 @@ # Copyright The OpenTelemetry Authors # SPDX-License-Identifier: Apache-2.0 +import math import os import weakref from atexit import register, unregister @@ -224,12 +225,23 @@ def create_histogram( ) except (KeyError, TypeError): invalid_advisory = True + if not invalid_advisory: + # Mirrors the check _ExplicitBucketHistogramAggregation.__init__ raises on. + # Catching it here means a bad advisory is dropped with a warning when the + # histogram is created, instead of raising uncaught out of every record() + # call once the aggregation is actually built. + for idx, bound in enumerate(explicit_bucket_boundaries_advisory): + if not math.isfinite(bound) or (idx and explicit_bucket_boundaries_advisory[idx - 1] >= bound): + invalid_advisory = True + break else: invalid_advisory = True if invalid_advisory: explicit_bucket_boundaries_advisory = None - _logger.warning("explicit_bucket_boundaries_advisory must be a sequence of numbers") + _logger.warning( + "explicit_bucket_boundaries_advisory must be a sequence of finite, strictly increasing numbers" + ) with self._instrument_registration_lock: status = self._register_instrument( diff --git a/opentelemetry-sdk/tests/metrics/test_metrics.py b/opentelemetry-sdk/tests/metrics/test_metrics.py index 47db7bc648..d5cbd267b6 100644 --- a/opentelemetry-sdk/tests/metrics/test_metrics.py +++ b/opentelemetry-sdk/tests/metrics/test_metrics.py @@ -690,16 +690,42 @@ def test_create_histogram_advisory_validation(self): advisories = [ {"explicit_bucket_boundaries_advisory": "hello"}, {"explicit_bucket_boundaries_advisory": ["1"]}, + # Right type, but not strictly increasing / not finite: these pass the + # type check but previously weren't caught until the first record() call. + {"explicit_bucket_boundaries_advisory": [100, 10, 50]}, + {"explicit_bucket_boundaries_advisory": [1, 1]}, + {"explicit_bucket_boundaries_advisory": [1.0, float("nan")]}, ] - for advisory in advisories: + for idx, advisory in enumerate(advisories): with self.subTest(advisory=advisory): - with self.assertLogs(level=WARNING): + # A distinct name per case, so the assertion below can't accidentally + # be satisfied by the SDK's unrelated "duplicate instrument name" + # warning instead of the advisory-validation warning under test. + with self.assertLogs(level=WARNING) as cm: self.meter.create_histogram( - "name", + f"histogram-{idx}", unit="unit", description="description", **advisory, ) + self.assertTrue(any("explicit_bucket_boundaries_advisory" in line for line in cm.output)) + + def test_histogram_bad_advisory_does_not_crash_record(self): + # create_histogram only type-checked the advisory, so an unsorted (but + # correctly-typed) advisory used to sail through with no warning and then + # raise ValueError out of every single record() call once the real + # aggregation was built on first use. self.meter's measurement_consumer is + # a Mock, so this needs a real MeterProvider + reader to actually reach + # the aggregation construction that record() triggers. + reader = InMemoryMetricReader() + meter = MeterProvider(metric_readers=[reader]).get_meter("test_meter") + with self.assertLogs(level=WARNING): + histogram = meter.create_histogram( + "name", + explicit_bucket_boundaries_advisory=[100, 10, 50], + ) + histogram.record(5) + histogram.record(5) def test_create_observable_gauge(self): observable_gauge = self.meter.create_observable_gauge(