Skip to content

Commit e6f2809

Browse files
ocelotlherin049
ocelotl
and
herin049
authored
Add record_min_max option to exponential histogram aggregation (#5377)
* 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: rename fragment to match PR 5377 * 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. * 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. --------- Co-authored-by: Lukas Hering <40302054+herin049@users.noreply.github.com>
1 parent 063809f commit e6f2809

7 files changed

Lines changed: 172 additions & 44 deletions

File tree

‎.changelog/5377.added‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1 @@
1+
`opentelemetry-sdk`: add `record_min_max` option to `ExponentialBucketHistogramAggregation`, matching the option already available on `ExplicitBucketHistogramAggregation` and required by the specification

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,8 @@ coverage.xml
3636
.cache
3737
htmlcov
3838

39+
.agent/
40+
3941
# Translations
4042
*.mo
4143

‎opentelemetry-sdk/src/opentelemetry/sdk/_configuration/_meter_provider.py‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -206,6 +206,13 @@ def _create_aggregation(config: AggregationConfig) -> Aggregation:
206206
kwargs["max_scale"] = (
207207
config.base2_exponential_bucket_histogram.max_scale
208208
)
209+
if (
210+
config.base2_exponential_bucket_histogram.record_min_max
211+
is not None
212+
):
213+
kwargs["record_min_max"] = (
214+
config.base2_exponential_bucket_histogram.record_min_max
215+
)
209216
return ExponentialBucketHistogramAggregation(**kwargs)
210217
if config.last_value is not None:
211218
return LastValueAggregation()

‎opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py‎

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -629,6 +629,7 @@ def __init__(
629629
# https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exponential-bucket-histogram-aggregation)
630630
max_size: int = 160,
631631
max_scale: int = 20,
632+
record_min_max: bool = True,
632633
):
633634
# max_size is the maximum capacity of the positive and negative
634635
# buckets.
@@ -677,6 +678,7 @@ def __init__(
677678
self._start_time_unix_nano = start_time_unix_nano
678679
self._max_size = max_size
679680
self._max_scale = max_scale
681+
self._record_min_max = record_min_max
680682

681683
self._value_positive = None
682684
self._value_negative = None
@@ -715,8 +717,9 @@ def aggregate(
715717

716718
self._sum += measurement_value
717719

718-
self._min = min(self._min, measurement_value)
719-
self._max = max(self._max, measurement_value)
720+
if self._record_min_max:
721+
self._min = min(self._min, measurement_value)
722+
self._max = max(self._max, measurement_value)
720723

721724
self._count += 1
722725

@@ -1313,13 +1316,28 @@ def _create_aggregation(
13131316

13141317

13151318
class ExponentialBucketHistogramAggregation(Aggregation):
1319+
"""This aggregation informs the SDK to collect:
1320+
1321+
- Count of Measurement values falling using a base-2 exponential formula.
1322+
- 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.
1323+
- Min (optional) Measurement value in population.
1324+
- Max (optional) Measurement value in population.
1325+
1326+
Args:
1327+
max_size: Maximum number of buckets in each of the positive and negative ranges, not counting the special zero bucket.
1328+
max_scale: Maximum scale factor.
1329+
record_min_max: Whether to record min and max.
1330+
"""
1331+
13161332
def __init__(
13171333
self,
13181334
max_size: int = 160,
13191335
max_scale: int = 20,
1336+
record_min_max: bool = True,
13201337
):
13211338
self._max_size = max_size
13221339
self._max_scale = max_scale
1340+
self._record_min_max = record_min_max
13231341

13241342
def _create_aggregation(
13251343
self,
@@ -1345,6 +1363,7 @@ def _create_aggregation(
13451363
start_time_unix_nano,
13461364
max_size=self._max_size,
13471365
max_scale=self._max_scale,
1366+
record_min_max=self._record_min_max,
13481367
)
13491368

13501369

‎opentelemetry-sdk/tests/_configuration/test_meter_provider.py‎

Lines changed: 23 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -23,9 +23,6 @@
2323
from opentelemetry.sdk._configuration.models import (
2424
ConsoleMetricExporter as ConsoleMetricExporterConfig,
2525
)
26-
from opentelemetry.sdk._configuration.models import (
27-
ExemplarFilter as ExemplarFilterConfig,
28-
)
2926
from opentelemetry.sdk._configuration.models import (
3027
ExperimentalOtlpFileMetricExporter as ExperimentalOtlpFileMetricExporterConfig,
3128
)
@@ -71,8 +68,6 @@
7168
View as ViewConfig,
7269
)
7370
from opentelemetry.sdk.metrics import (
74-
AlwaysOffExemplarFilter,
75-
AlwaysOnExemplarFilter,
7671
Counter,
7772
Histogram,
7873
MeterProvider,
@@ -934,6 +929,29 @@ def test_stream_aggregation_base2_exponential_with_params(self):
934929
view._aggregation, ExponentialBucketHistogramAggregation
935930
)
936931

932+
def test_stream_aggregation_base2_exponential_record_min_max(self):
933+
for record_min_max, expected in [
934+
(True, True),
935+
(False, False),
936+
(None, True),
937+
]:
938+
with self.subTest(record_min_max=record_min_max):
939+
view = self._get_view(
940+
self._make_view_config(
941+
stream_kwargs={
942+
"aggregation": AggregationConfig(
943+
base2_exponential_bucket_histogram=Base2Config(
944+
record_min_max=record_min_max
945+
)
946+
)
947+
}
948+
)
949+
)
950+
self.assertIsInstance(
951+
view._aggregation, ExponentialBucketHistogramAggregation
952+
)
953+
self.assertEqual(view._aggregation._record_min_max, expected)
954+
937955
def test_stream_aggregation_last_value(self):
938956
view = self._get_view(
939957
self._make_view_config(
@@ -957,39 +975,3 @@ def test_stream_aggregation_default(self):
957975
)
958976
)
959977
self.assertIsInstance(view._aggregation, DefaultAggregation)
960-
961-
962-
class TestExemplarFilter(unittest.TestCase):
963-
@staticmethod
964-
def _make_config(exemplar_filter):
965-
return MeterProviderConfig(readers=[], exemplar_filter=exemplar_filter)
966-
967-
def test_always_on(self):
968-
provider = create_meter_provider(
969-
self._make_config(ExemplarFilterConfig.always_on)
970-
)
971-
self.assertIsInstance(
972-
provider._sdk_config.exemplar_filter, AlwaysOnExemplarFilter
973-
)
974-
975-
def test_always_off(self):
976-
provider = create_meter_provider(
977-
self._make_config(ExemplarFilterConfig.always_off)
978-
)
979-
self.assertIsInstance(
980-
provider._sdk_config.exemplar_filter, AlwaysOffExemplarFilter
981-
)
982-
983-
def test_trace_based(self):
984-
provider = create_meter_provider(
985-
self._make_config(ExemplarFilterConfig.trace_based)
986-
)
987-
self.assertIsInstance(
988-
provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter
989-
)
990-
991-
def test_absent_defaults_to_trace_based(self):
992-
provider = create_meter_provider(MeterProviderConfig(readers=[]))
993-
self.assertIsInstance(
994-
provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter
995-
)

‎opentelemetry-sdk/tests/_configuration/test_meter_provider_exemplar_filter.py‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
# Copyright The OpenTelemetry Authors
2+
# SPDX-License-Identifier: Apache-2.0
3+
4+
# Tests access private members of SDK classes to assert correct configuration.
5+
# pylint: disable=protected-access
6+
7+
import unittest
8+
9+
from opentelemetry.sdk._configuration._meter_provider import (
10+
create_meter_provider,
11+
)
12+
from opentelemetry.sdk._configuration.models import (
13+
ExemplarFilter as ExemplarFilterConfig,
14+
)
15+
from opentelemetry.sdk._configuration.models import (
16+
MeterProvider as MeterProviderConfig,
17+
)
18+
from opentelemetry.sdk.metrics import (
19+
AlwaysOffExemplarFilter,
20+
AlwaysOnExemplarFilter,
21+
TraceBasedExemplarFilter,
22+
)
23+
24+
25+
class TestExemplarFilter(unittest.TestCase):
26+
@staticmethod
27+
def _make_config(exemplar_filter):
28+
return MeterProviderConfig(readers=[], exemplar_filter=exemplar_filter)
29+
30+
def test_always_on(self):
31+
provider = create_meter_provider(
32+
self._make_config(ExemplarFilterConfig.always_on)
33+
)
34+
self.assertIsInstance(
35+
provider._sdk_config.exemplar_filter, AlwaysOnExemplarFilter
36+
)
37+
38+
def test_always_off(self):
39+
provider = create_meter_provider(
40+
self._make_config(ExemplarFilterConfig.always_off)
41+
)
42+
self.assertIsInstance(
43+
provider._sdk_config.exemplar_filter, AlwaysOffExemplarFilter
44+
)
45+
46+
def test_trace_based(self):
47+
provider = create_meter_provider(
48+
self._make_config(ExemplarFilterConfig.trace_based)
49+
)
50+
self.assertIsInstance(
51+
provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter
52+
)
53+
54+
def test_absent_defaults_to_trace_based(self):
55+
provider = create_meter_provider(MeterProviderConfig(readers=[]))
56+
self.assertIsInstance(
57+
provider._sdk_config.exemplar_filter, TraceBasedExemplarFilter
58+
)

‎opentelemetry-sdk/tests/metrics/exponential_histogram/test_exponential_bucket_histogram_aggregation.py‎

Lines changed: 60 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
from inspect import currentframe
88
from itertools import permutations
99
from logging import WARNING
10-
from math import ldexp
10+
from math import inf, ldexp
1111
from random import Random, randrange
1212
from sys import float_info, maxsize
1313
from time import time_ns
@@ -112,6 +112,65 @@ def test_create_aggregation(self, mock_logarithm_mapping):
112112

113113
mock_logarithm_mapping.assert_called_with(100)
114114

115+
def test_create_aggregation_record_min_max(self):
116+
for record_min_max, expected in [
117+
(None, True),
118+
(True, True),
119+
(False, False),
120+
]:
121+
with self.subTest(record_min_max=record_min_max):
122+
kwargs = (
123+
{}
124+
if record_min_max is None
125+
else {"record_min_max": record_min_max}
126+
)
127+
exponential_bucket_histogram_aggregation = (
128+
ExponentialBucketHistogramAggregation(**kwargs)
129+
)._create_aggregation(Mock(), Mock(), Mock(), Mock())
130+
131+
self.assertEqual(
132+
exponential_bucket_histogram_aggregation._record_min_max,
133+
expected,
134+
)
135+
136+
def test_min_max(self):
137+
"""
138+
`record_min_max` indicates the aggregator to record the minimum and
139+
maximum value in the population
140+
"""
141+
142+
now = time_ns()
143+
144+
for record_min_max, expected_min, expected_max in [
145+
(True, 1, 9999),
146+
(False, inf, -inf),
147+
]:
148+
with self.subTest(record_min_max=record_min_max):
149+
ctx = Context()
150+
exponential_histogram_aggregation = (
151+
_ExponentialBucketHistogramAggregation(
152+
Mock(),
153+
_default_reservoir_factory(
154+
_ExponentialBucketHistogramAggregation
155+
),
156+
AggregationTemporality.CUMULATIVE,
157+
0,
158+
record_min_max=record_min_max,
159+
)
160+
)
161+
162+
for value in [2, 4, 1, 9999]:
163+
exponential_histogram_aggregation.aggregate(
164+
Measurement(value, now, Mock(), ctx)
165+
)
166+
167+
self.assertEqual(
168+
exponential_histogram_aggregation._min, expected_min
169+
)
170+
self.assertEqual(
171+
exponential_histogram_aggregation._max, expected_max
172+
)
173+
115174
def assertInEpsilon(self, first, second, epsilon):
116175
self.assertLessEqual(first, (second * (1 + epsilon)))
117176
self.assertGreaterEqual(first, (second * (1 - epsilon)))

0 commit comments

Comments
 (0)