From 3410c6854144bfa9aa1caa3d93021e28e1f7b5e7 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 1 Jul 2026 14:00:34 -0600 Subject: [PATCH 1/4] opentelemetry-sdk: add record_min_max option to exponential histogram aggregation The spec defines RecordMinMax as a configuration parameter for both the explicit bucket and base2 exponential bucket histogram aggregations, but only the explicit bucket variant exposed it. Exponential histograms always tracked min/max unconditionally, with no way to opt out. --- .changelog/3322.added | 1 + .../sdk/_configuration/_meter_provider.py | 7 ++ .../sdk/metrics/_internal/aggregation.py | 23 ++++++- .../_configuration/test_meter_provider.py | 17 +++++ ...xponential_bucket_histogram_aggregation.py | 67 ++++++++++++++++++- 5 files changed, 112 insertions(+), 3 deletions(-) create mode 100644 .changelog/3322.added diff --git a/.changelog/3322.added b/.changelog/3322.added new file mode 100644 index 0000000000..d8af02e550 --- /dev/null +++ b/.changelog/3322.added @@ -0,0 +1 @@ +`opentelemetry-sdk`: add `record_min_max` option to `ExponentialBucketHistogramAggregation`, matching the option already available on `ExplicitBucketHistogramAggregation` and required by the specification diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py b/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py index 8fffaf9d63..66cb2f1a09 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py @@ -203,6 +203,13 @@ def _create_aggregation(config: AggregationConfig) -> Aggregation: kwargs["max_scale"] = ( config.base2_exponential_bucket_histogram.max_scale ) + if ( + config.base2_exponential_bucket_histogram.record_min_max + is not None + ): + kwargs["record_min_max"] = ( + config.base2_exponential_bucket_histogram.record_min_max + ) return ExponentialBucketHistogramAggregation(**kwargs) if config.last_value is not None: return LastValueAggregation() diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index dea213f428..7cebf654c2 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -623,6 +623,7 @@ def __init__( # https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exponential-bucket-histogram-aggregation) max_size: int = 160, max_scale: int = 20, + record_min_max: bool = True, ): # max_size is the maximum capacity of the positive and negative # buckets. @@ -671,6 +672,7 @@ def __init__( self._start_time_unix_nano = start_time_unix_nano self._max_size = max_size self._max_scale = max_scale + self._record_min_max = record_min_max self._value_positive = None self._value_negative = None @@ -709,8 +711,9 @@ def aggregate( self._sum += measurement_value - self._min = min(self._min, measurement_value) - self._max = max(self._max, measurement_value) + if self._record_min_max: + self._min = min(self._min, measurement_value) + self._max = max(self._max, measurement_value) self._count += 1 @@ -1307,13 +1310,28 @@ def _create_aggregation( class ExponentialBucketHistogramAggregation(Aggregation): + """This aggregation informs the SDK to collect: + + - Count of Measurement values falling using a base-2 exponential formula. + - Arithmetic sum of Measurement values in population. This SHOULD NOT be collected when used with instruments that record negative measurements, e.g. UpDownCounter or ObservableGauge. + - Min (optional) Measurement value in population. + - Max (optional) Measurement value in population. + + Args: + max_size: Maximum number of buckets in each of the positive and negative ranges, not counting the special zero bucket. + max_scale: Maximum scale factor. + record_min_max: Whether to record min and max. + """ + def __init__( self, max_size: int = 160, max_scale: int = 20, + record_min_max: bool = True, ): self._max_size = max_size self._max_scale = max_scale + self._record_min_max = record_min_max def _create_aggregation( self, @@ -1339,6 +1357,7 @@ def _create_aggregation( start_time_unix_nano, max_size=self._max_size, max_scale=self._max_scale, + record_min_max=self._record_min_max, ) diff --git a/opentelemetry-sdk/tests/_configuration/test_meter_provider.py b/opentelemetry-sdk/tests/_configuration/test_meter_provider.py index adc8a63f56..8d7d8044bd 100644 --- a/opentelemetry-sdk/tests/_configuration/test_meter_provider.py +++ b/opentelemetry-sdk/tests/_configuration/test_meter_provider.py @@ -934,6 +934,23 @@ def test_stream_aggregation_base2_exponential_with_params(self): view._aggregation, ExponentialBucketHistogramAggregation ) + def test_stream_aggregation_base2_exponential_record_min_max(self): + view = self._get_view( + self._make_view_config( + stream_kwargs={ + "aggregation": AggregationConfig( + base2_exponential_bucket_histogram=Base2Config( + record_min_max=False + ) + ) + } + ) + ) + self.assertIsInstance( + view._aggregation, ExponentialBucketHistogramAggregation + ) + self.assertFalse(view._aggregation._record_min_max) + def test_stream_aggregation_last_value(self): view = self._get_view( self._make_view_config( diff --git a/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py b/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py index fea16a7bc8..6a5807d9f7 100644 --- a/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py @@ -7,7 +7,7 @@ from inspect import currentframe from itertools import permutations from logging import WARNING -from math import ldexp +from math import inf, ldexp from random import Random, randrange from sys import float_info, maxsize from time import time_ns @@ -112,6 +112,71 @@ def test_create_aggregation(self, mock_logarithm_mapping): mock_logarithm_mapping.assert_called_with(100) + def test_create_aggregation_record_min_max(self): + exponential_bucket_histogram_aggregation = ( + ExponentialBucketHistogramAggregation() + )._create_aggregation(Mock(), Mock(), Mock(), Mock()) + + self.assertTrue( + exponential_bucket_histogram_aggregation._record_min_max + ) + + exponential_bucket_histogram_aggregation = ( + ExponentialBucketHistogramAggregation(record_min_max=False) + )._create_aggregation(Mock(), Mock(), Mock(), Mock()) + + self.assertFalse( + exponential_bucket_histogram_aggregation._record_min_max + ) + + def test_min_max(self): + """ + `record_min_max` indicates the aggregator to record the minimum and + maximum value in the population + """ + + now = time_ns() + ctx = Context() + + exponential_histogram_aggregation = ( + _ExponentialBucketHistogramAggregation( + Mock(), + _default_reservoir_factory( + _ExponentialBucketHistogramAggregation + ), + AggregationTemporality.CUMULATIVE, + 0, + ) + ) + + for value in [2, 4, 1, 9999]: + exponential_histogram_aggregation.aggregate( + Measurement(value, now, Mock(), ctx) + ) + + self.assertEqual(exponential_histogram_aggregation._min, 1) + self.assertEqual(exponential_histogram_aggregation._max, 9999) + + exponential_histogram_aggregation = ( + _ExponentialBucketHistogramAggregation( + Mock(), + _default_reservoir_factory( + _ExponentialBucketHistogramAggregation + ), + AggregationTemporality.CUMULATIVE, + 0, + record_min_max=False, + ) + ) + + for value in [2, 4, 1, 9999]: + exponential_histogram_aggregation.aggregate( + Measurement(value, now, Mock(), ctx) + ) + + self.assertEqual(exponential_histogram_aggregation._min, inf) + self.assertEqual(exponential_histogram_aggregation._max, -inf) + def assertInEpsilon(self, first, second, epsilon): self.assertLessEqual(first, (second * (1 + epsilon))) self.assertGreaterEqual(first, (second * (1 - epsilon))) From b6077c682f439a6f5abe2764067ec405538b7f75 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Wed, 1 Jul 2026 14:17:19 -0600 Subject: [PATCH 2/4] changelog: rename fragment to match PR 5377 --- .changelog/{3322.added => 5377.added} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename .changelog/{3322.added => 5377.added} (100%) diff --git a/.changelog/3322.added b/.changelog/5377.added similarity index 100% rename from .changelog/3322.added rename to .changelog/5377.added From 56aa292c8721e2c3cf68074056bae7047fb6331a Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Fri, 3 Jul 2026 17:52:25 -0600 Subject: [PATCH 3/4] test: parameterize record_min_max tests with subTest per review feedback Addresses PR review nits on #5377: cover explicit True/False and the default case for record_min_max in a single parameterized test instead of duplicating setup per case. --- .../_configuration/test_meter_provider.py | 34 ++++--- ...xponential_bucket_histogram_aggregation.py | 96 +++++++++---------- 2 files changed, 65 insertions(+), 65 deletions(-) diff --git a/opentelemetry-sdk/tests/_configuration/test_meter_provider.py b/opentelemetry-sdk/tests/_configuration/test_meter_provider.py index 8d7d8044bd..9f77c904b5 100644 --- a/opentelemetry-sdk/tests/_configuration/test_meter_provider.py +++ b/opentelemetry-sdk/tests/_configuration/test_meter_provider.py @@ -935,21 +935,27 @@ def test_stream_aggregation_base2_exponential_with_params(self): ) def test_stream_aggregation_base2_exponential_record_min_max(self): - view = self._get_view( - self._make_view_config( - stream_kwargs={ - "aggregation": AggregationConfig( - base2_exponential_bucket_histogram=Base2Config( - record_min_max=False - ) + for record_min_max, expected in [ + (True, True), + (False, False), + (None, True), + ]: + with self.subTest(record_min_max=record_min_max): + view = self._get_view( + self._make_view_config( + stream_kwargs={ + "aggregation": AggregationConfig( + base2_exponential_bucket_histogram=Base2Config( + record_min_max=record_min_max + ) + ) + } ) - } - ) - ) - self.assertIsInstance( - view._aggregation, ExponentialBucketHistogramAggregation - ) - self.assertFalse(view._aggregation._record_min_max) + ) + self.assertIsInstance( + view._aggregation, ExponentialBucketHistogramAggregation + ) + self.assertEqual(view._aggregation._record_min_max, expected) def test_stream_aggregation_last_value(self): view = self._get_view( diff --git a/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py b/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py index 6a5807d9f7..88d6923db7 100644 --- a/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py +++ b/opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py @@ -113,21 +113,25 @@ def test_create_aggregation(self, mock_logarithm_mapping): mock_logarithm_mapping.assert_called_with(100) def test_create_aggregation_record_min_max(self): - exponential_bucket_histogram_aggregation = ( - ExponentialBucketHistogramAggregation() - )._create_aggregation(Mock(), Mock(), Mock(), Mock()) - - self.assertTrue( - exponential_bucket_histogram_aggregation._record_min_max - ) - - exponential_bucket_histogram_aggregation = ( - ExponentialBucketHistogramAggregation(record_min_max=False) - )._create_aggregation(Mock(), Mock(), Mock(), Mock()) + for record_min_max, expected in [ + (None, True), + (True, True), + (False, False), + ]: + with self.subTest(record_min_max=record_min_max): + kwargs = ( + {} + if record_min_max is None + else {"record_min_max": record_min_max} + ) + exponential_bucket_histogram_aggregation = ( + ExponentialBucketHistogramAggregation(**kwargs) + )._create_aggregation(Mock(), Mock(), Mock(), Mock()) - self.assertFalse( - exponential_bucket_histogram_aggregation._record_min_max - ) + self.assertEqual( + exponential_bucket_histogram_aggregation._record_min_max, + expected, + ) def test_min_max(self): """ @@ -136,46 +140,36 @@ def test_min_max(self): """ now = time_ns() - ctx = Context() - - exponential_histogram_aggregation = ( - _ExponentialBucketHistogramAggregation( - Mock(), - _default_reservoir_factory( - _ExponentialBucketHistogramAggregation - ), - AggregationTemporality.CUMULATIVE, - 0, - ) - ) - - for value in [2, 4, 1, 9999]: - exponential_histogram_aggregation.aggregate( - Measurement(value, now, Mock(), ctx) - ) - - self.assertEqual(exponential_histogram_aggregation._min, 1) - self.assertEqual(exponential_histogram_aggregation._max, 9999) - exponential_histogram_aggregation = ( - _ExponentialBucketHistogramAggregation( - Mock(), - _default_reservoir_factory( - _ExponentialBucketHistogramAggregation - ), - AggregationTemporality.CUMULATIVE, - 0, - record_min_max=False, - ) - ) + for record_min_max, expected_min, expected_max in [ + (True, 1, 9999), + (False, inf, -inf), + ]: + with self.subTest(record_min_max=record_min_max): + ctx = Context() + exponential_histogram_aggregation = ( + _ExponentialBucketHistogramAggregation( + Mock(), + _default_reservoir_factory( + _ExponentialBucketHistogramAggregation + ), + AggregationTemporality.CUMULATIVE, + 0, + record_min_max=record_min_max, + ) + ) - for value in [2, 4, 1, 9999]: - exponential_histogram_aggregation.aggregate( - Measurement(value, now, Mock(), ctx) - ) + for value in [2, 4, 1, 9999]: + exponential_histogram_aggregation.aggregate( + Measurement(value, now, Mock(), ctx) + ) - self.assertEqual(exponential_histogram_aggregation._min, inf) - self.assertEqual(exponential_histogram_aggregation._max, -inf) + self.assertEqual( + exponential_histogram_aggregation._min, expected_min + ) + self.assertEqual( + exponential_histogram_aggregation._max, expected_max + ) def assertInEpsilon(self, first, second, epsilon): self.assertLessEqual(first, (second * (1 + epsilon))) From 74f652b11e7024cfb49c4ec8aa6f7027728c4a72 Mon Sep 17 00:00:00 2001 From: Diego Hurtado Date: Fri, 3 Jul 2026 18:05:18 -0600 Subject: [PATCH 4/4] fix(ci): split test_meter_provider.py to satisfy pylint max-module-lines Root cause: the module reached 1018 lines, above pylint's max-module-lines=1000 (C0302), causing the lint job (and the alls-green check gate) to fail. Move the self-contained TestExemplarFilter class into its own test file with only the imports it needs; no test logic changed. --- .gitignore | 2 + .../_configuration/test_meter_provider.py | 41 ------------- .../test_meter_provider_exemplar_filter.py | 58 +++++++++++++++++++ 3 files changed, 60 insertions(+), 41 deletions(-) create mode 100644 opentelemetry-sdk/tests/_configuration/test_meter_provider_exemplar_filter.py diff --git a/.gitignore b/.gitignore index 07c7b9aa6e..fcc3f5df27 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,8 @@ coverage.xml .cache htmlcov +.agent/ + # Translations *.mo diff --git a/opentelemetry-sdk/tests/_configuration/test_meter_provider.py b/opentelemetry-sdk/tests/_configuration/test_meter_provider.py index 9f77c904b5..9780f967e0 100644 --- a/opentelemetry-sdk/tests/_configuration/test_meter_provider.py +++ b/opentelemetry-sdk/tests/_configuration/test_meter_provider.py @@ -23,9 +23,6 @@ from opentelemetry.sdk._configuration.models import ( ConsoleMetricExporter as ConsoleMetricExporterConfig, ) -from opentelemetry.sdk._configuration.models import ( - ExemplarFilter as ExemplarFilterConfig, -) from opentelemetry.sdk._configuration.models import ( ExperimentalOtlpFileMetricExporter as ExperimentalOtlpFileMetricExporterConfig, ) @@ -71,8 +68,6 @@ View as ViewConfig, ) from opentelemetry.sdk.metrics import ( - AlwaysOffExemplarFilter, - AlwaysOnExemplarFilter, Counter, Histogram, MeterProvider, @@ -980,39 +975,3 @@ def test_stream_aggregation_default(self): ) ) self.assertIsInstance(view._aggregation, DefaultAggregation) - - -class TestExemplarFilter(unittest.TestCase): - @staticmethod - def _make_config(exemplar_filter): - return MeterProviderConfig(readers=[], exemplar_filter=exemplar_filter) - - def test_always_on(self): - provider = create_meter_provider( - self._make_config(ExemplarFilterConfig.always_on) - ) - self.assertIsInstance( - provider._sdk_config.exemplar_filter, AlwaysOnExemplarFilter - ) - - def test_always_off(self): - provider = create_meter_provider( - self._make_config(ExemplarFilterConfig.always_off) - ) - self.assertIsInstance( - provider._sdk_config.exemplar_filter, AlwaysOffExemplarFilter - ) - - def test_trace_based(self): - provider = create_meter_provider( - self._make_config(ExemplarFilterConfig.trace_based) - ) - self.assertIsInstance( - provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter - ) - - def test_absent_defaults_to_trace_based(self): - provider = create_meter_provider(MeterProviderConfig(readers=[])) - self.assertIsInstance( - provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter - ) diff --git a/opentelemetry-sdk/tests/_configuration/test_meter_provider_exemplar_filter.py b/opentelemetry-sdk/tests/_configuration/test_meter_provider_exemplar_filter.py new file mode 100644 index 0000000000..ac5a617f47 --- /dev/null +++ b/opentelemetry-sdk/tests/_configuration/test_meter_provider_exemplar_filter.py @@ -0,0 +1,58 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +# Tests access private members of SDK classes to assert correct configuration. +# pylint: disable=protected-access + +import unittest + +from opentelemetry.sdk._configuration._meter_provider import ( + create_meter_provider, +) +from opentelemetry.sdk._configuration.models import ( + ExemplarFilter as ExemplarFilterConfig, +) +from opentelemetry.sdk._configuration.models import ( + MeterProvider as MeterProviderConfig, +) +from opentelemetry.sdk.metrics import ( + AlwaysOffExemplarFilter, + AlwaysOnExemplarFilter, + TraceBasedExemplarFilter, +) + + +class TestExemplarFilter(unittest.TestCase): + @staticmethod + def _make_config(exemplar_filter): + return MeterProviderConfig(readers=[], exemplar_filter=exemplar_filter) + + def test_always_on(self): + provider = create_meter_provider( + self._make_config(ExemplarFilterConfig.always_on) + ) + self.assertIsInstance( + provider._sdk_config.exemplar_filter, AlwaysOnExemplarFilter + ) + + def test_always_off(self): + provider = create_meter_provider( + self._make_config(ExemplarFilterConfig.always_off) + ) + self.assertIsInstance( + provider._sdk_config.exemplar_filter, AlwaysOffExemplarFilter + ) + + def test_trace_based(self): + provider = create_meter_provider( + self._make_config(ExemplarFilterConfig.trace_based) + ) + self.assertIsInstance( + provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter + ) + + def test_absent_defaults_to_trace_based(self): + provider = create_meter_provider(MeterProviderConfig(readers=[])) + self.assertIsInstance( + provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter + )