diff --git a/.changelog/5377.added b/.changelog/5377.added new file mode 100644 index 00000000000..d8af02e5505 --- /dev/null +++ b/.changelog/5377.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/.gitignore b/.gitignore index 07c7b9aa6e4..fcc3f5df278 100644 --- a/.gitignore +++ b/.gitignore @@ -36,6 +36,8 @@ coverage.xml .cache htmlcov +.agent/ + # Translations *.mo diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py b/opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py index 8fffaf9d63e..66cb2f1a09c 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 dea213f4282..7cebf654c20 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 adc8a63f562..9780f967e08 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, @@ -934,6 +929,29 @@ def test_stream_aggregation_base2_exponential_with_params(self): view._aggregation, ExponentialBucketHistogramAggregation ) + def test_stream_aggregation_base2_exponential_record_min_max(self): + 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.assertEqual(view._aggregation._record_min_max, expected) + def test_stream_aggregation_last_value(self): view = self._get_view( self._make_view_config( @@ -957,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 00000000000..ac5a617f470 --- /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 + ) 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 fea16a7bc87..88d6923db73 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,65 @@ def test_create_aggregation(self, mock_logarithm_mapping): mock_logarithm_mapping.assert_called_with(100) + def test_create_aggregation_record_min_max(self): + 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.assertEqual( + exponential_bucket_histogram_aggregation._record_min_max, + expected, + ) + + def test_min_max(self): + """ + `record_min_max` indicates the aggregator to record the minimum and + maximum value in the population + """ + + now = time_ns() + + 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) + ) + + 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))) self.assertGreaterEqual(first, (second * (1 - epsilon)))