From 4e264b14338c21cab3d242807d7159928722e306 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 23 Aug 2026 19:29:43 +0330 Subject: [PATCH 1/2] test(sdk): assert histograms omit min/max when recording is disabled The aggregators seed min/max with +inf/-inf. Assert those sentinels never reach a data point when record_min_max is disabled: the SDK fields are None, the OTLP fields are absent, to_json stays valid JSON, and the delta-to-cumulative merge does not resurrect them. Both histogram aggregations and both temporalities are covered, along with the record_min_max=True path to guard against over-correcting. These tests fail against the current implementation. --- .../metrics/test_histogram_record_min_max.py | 163 ++++++++++++++++++ 1 file changed, 163 insertions(+) create mode 100644 opentelemetry-sdk/tests/metrics/test_histogram_record_min_max.py diff --git a/opentelemetry-sdk/tests/metrics/test_histogram_record_min_max.py b/opentelemetry-sdk/tests/metrics/test_histogram_record_min_max.py new file mode 100644 index 0000000000..3989dbd7c4 --- /dev/null +++ b/opentelemetry-sdk/tests/metrics/test_histogram_record_min_max.py @@ -0,0 +1,163 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +"""With record_min_max disabled the min/max fields must be absent, not sentinels. + +The aggregators seed min/max with +inf/-inf. If those sentinels reach a data +point they are exported as present OTLP fields, producing a histogram whose +minimum is +Infinity and maximum is -Infinity, and `to_json` emits the literals +`Infinity` / `-Infinity`, which are not valid JSON. +""" + +import json +import unittest + +from opentelemetry.exporter.otlp.proto.common.metrics_encoder import ( + encode_metrics, +) +from opentelemetry.sdk.metrics import Histogram as HistogramInstrument +from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics.export import ( + AggregationTemporality, + InMemoryMetricReader, +) +from opentelemetry.sdk.metrics.view import ( + ExplicitBucketHistogramAggregation, + ExponentialBucketHistogramAggregation, + View, +) + + +def _reject_non_json_constants(constant): + raise ValueError(f"{constant!r} is not valid JSON") + + +class _HistogramCase: + """Drive one histogram aggregation through a reader.""" + + def __init__(self, aggregation, temporality=AggregationTemporality.CUMULATIVE): + self.reader = InMemoryMetricReader( + preferred_temporality={HistogramInstrument: temporality}, + ) + self.provider = MeterProvider( + metric_readers=[self.reader], + views=[View(instrument_name="hist", aggregation=aggregation)], + ) + self.histogram = self.provider.get_meter(__name__).create_histogram("hist") + + def collect(self): + data = self.reader.get_metrics_data() + return data.resource_metrics[0].scope_metrics[0].metrics[0].data.data_points[0] + + def collect_encoded(self): + data = self.reader.get_metrics_data() + metric = encode_metrics(data).resource_metrics[0].scope_metrics[0].metrics[0] + return getattr(metric, metric.WhichOneof("data")).data_points[0] + + def shutdown(self): + self.provider.shutdown() + + +class TestRecordMinMaxDisabled(unittest.TestCase): + AGGREGATIONS = { + "explicit": ExplicitBucketHistogramAggregation, + "exponential": ExponentialBucketHistogramAggregation, + } + + def test_data_point_leaves_min_and_max_unset(self): + for name, aggregation in self.AGGREGATIONS.items(): + with self.subTest(aggregation=name): + case = _HistogramCase(aggregation(record_min_max=False)) + case.histogram.record(5) + case.histogram.record(50) + point = case.collect() + self.assertIsNone(point.min) + self.assertIsNone(point.max) + self.assertEqual(point.sum, 55) + self.assertEqual(point.count, 2) + case.shutdown() + + def test_otlp_fields_are_absent(self): + for name, aggregation in self.AGGREGATIONS.items(): + with self.subTest(aggregation=name): + case = _HistogramCase(aggregation(record_min_max=False)) + case.histogram.record(5) + point = case.collect_encoded() + self.assertFalse(point.HasField("min")) + self.assertFalse(point.HasField("max")) + case.shutdown() + + def test_to_json_is_valid_json(self): + for name, aggregation in self.AGGREGATIONS.items(): + with self.subTest(aggregation=name): + case = _HistogramCase(aggregation(record_min_max=False)) + case.histogram.record(5) + payload = case.collect().to_json() + json.loads(payload, parse_constant=_reject_non_json_constants) + case.shutdown() + + def test_cumulative_collections_stay_unset(self): + """The delta-to-cumulative merge must not resurrect the sentinels.""" + for name, aggregation in self.AGGREGATIONS.items(): + with self.subTest(aggregation=name): + case = _HistogramCase(aggregation(record_min_max=False)) + case.histogram.record(5) + case.collect() + case.histogram.record(50) + point = case.collect() + self.assertIsNone(point.min) + self.assertIsNone(point.max) + self.assertEqual(point.sum, 55) + case.shutdown() + + def test_delta_temporality_leaves_min_and_max_unset(self): + case = _HistogramCase( + ExplicitBucketHistogramAggregation(record_min_max=False), + temporality=AggregationTemporality.DELTA, + ) + case.histogram.record(5) + point = case.collect() + self.assertIsNone(point.min) + self.assertIsNone(point.max) + case.shutdown() + + +class TestRecordMinMaxEnabled(unittest.TestCase): + """The default must keep working exactly as before.""" + + AGGREGATIONS = { + "explicit": ExplicitBucketHistogramAggregation, + "exponential": ExponentialBucketHistogramAggregation, + } + + def test_min_and_max_are_recorded(self): + for name, aggregation in self.AGGREGATIONS.items(): + with self.subTest(aggregation=name): + case = _HistogramCase(aggregation(record_min_max=True)) + case.histogram.record(5) + case.histogram.record(50) + point = case.collect() + self.assertEqual(point.min, 5) + self.assertEqual(point.max, 50) + case.shutdown() + + def test_cumulative_min_and_max_span_collections(self): + for name, aggregation in self.AGGREGATIONS.items(): + with self.subTest(aggregation=name): + case = _HistogramCase(aggregation(record_min_max=True)) + case.histogram.record(50) + case.collect() + case.histogram.record(5) + point = case.collect() + self.assertEqual(point.min, 5) + self.assertEqual(point.max, 50) + case.shutdown() + + def test_otlp_fields_are_present(self): + case = _HistogramCase(ExplicitBucketHistogramAggregation(record_min_max=True)) + case.histogram.record(5) + point = case.collect_encoded() + self.assertTrue(point.HasField("min")) + self.assertTrue(point.HasField("max")) + self.assertEqual(point.min, 5) + case.shutdown() From d912058b0329984a6891a749424c374993f56c75 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 23 Aug 2026 19:29:43 +0330 Subject: [PATCH 2/2] fix(sdk): leave histogram min/max unset when recording is disabled Both histogram aggregations seed _min/_max with +inf/-inf and never consulted record_min_max in collect(). With min/max recording disabled the sentinels reached the data point, and the OTLP encoder set both optional fields as present -- so backends received a histogram with min=+Infinity and max=-Infinity, violating the min <= max invariant. to_json emitted the bare literals Infinity and -Infinity, which are not valid JSON. Report None instead, in both aggregations and in both delta-to-cumulative merge branches, and widen the data point fields to float | None. Protobuf treats None as "field not set", so no encoder change is required. --- .changelog/5566.fixed | 1 + .../sdk/metrics/_internal/aggregation.py | 24 ++++++++++++------- .../sdk/metrics/_internal/point.py | 10 ++++---- 3 files changed, 23 insertions(+), 12 deletions(-) create mode 100644 .changelog/5566.fixed diff --git a/.changelog/5566.fixed b/.changelog/5566.fixed new file mode 100644 index 0000000000..fa6d679ebe --- /dev/null +++ b/.changelog/5566.fixed @@ -0,0 +1 @@ +`opentelemetry-sdk`: leave histogram `min`/`max` unset when `record_min_max` is disabled, instead of exporting the `+Inf`/`-Inf` sentinels diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index c0f7cf22d4..541c8f20a8 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -495,8 +495,8 @@ def collect( with self._lock: value = self._value sum_ = self._sum - min_ = self._min - max_ = self._max + min_ = self._min if self._record_min_max else None + max_ = self._max if self._record_min_max else None self._value = None self._sum = 0 @@ -539,8 +539,12 @@ def collect( previous_value_element, ) in zip(value, self._previous_value) ] - self._previous_min = min(min_, self._previous_min) - self._previous_max = max(max_, self._previous_max) + if self._record_min_max: + self._previous_min = min(min_, self._previous_min) + self._previous_max = max(max_, self._previous_max) + else: + self._previous_min = None + self._previous_max = None self._previous_sum = sum_ + self._previous_sum return HistogramDataPoint( @@ -764,8 +768,8 @@ def collect( value_positive = self._value_positive value_negative = self._value_negative sum_ = self._sum - min_ = self._min - max_ = self._max + min_ = self._min if self._record_min_max else None + max_ = self._max if self._record_min_max else None count = self._count zero_count = self._zero_count scale = self._scale @@ -924,8 +928,12 @@ def collect( collection_aggregation_temporality, ) - self._previous_min = min(min_, self._previous_min) - self._previous_max = max(max_, self._previous_max) + if self._record_min_max: + self._previous_min = min(min_, self._previous_min) + self._previous_max = max(max_, self._previous_max) + else: + self._previous_min = None + self._previous_max = None self._previous_sum = sum_ + self._previous_sum self._previous_count = count + self._previous_count self._previous_zero_count = zero_count + self._previous_zero_count diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/point.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/point.py index bb375fd7cf..87b3e0b782 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/point.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/point.py @@ -3,6 +3,8 @@ # pylint: disable=unused-import +from __future__ import annotations + from collections.abc import Sequence from dataclasses import asdict, dataclass, field from json import dumps, loads @@ -44,8 +46,8 @@ class HistogramDataPoint: sum: int | float bucket_counts: Sequence[int] explicit_bounds: Sequence[float] - min: float - max: float + min: float | None + max: float | None exemplars: Sequence[Exemplar] = field(default_factory=list) def to_json(self, indent: int | None = 4) -> str: @@ -75,8 +77,8 @@ class ExponentialHistogramDataPoint: positive: Buckets negative: Buckets flags: int - min: float - max: float + min: float | None + max: float | None exemplars: Sequence[Exemplar] = field(default_factory=list) def to_json(self, indent: int | None = 4) -> str: