From 6f42342a689433b456bf0df9828a76188c9e06fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20S=C3=A9guy?= <1515987+lseguy@users.noreply.github.com> Date: Wed, 2 Apr 2025 10:24:10 +0200 Subject: [PATCH 1/6] opentelemetry-sdk: fix explicit aggregation with multiple histogram explicit buckets advisory --- .../sdk/metrics/_internal/aggregation.py | 11 +++-- ...est_histogram_advisory_explicit_buckets.py | 44 +++++++++++++++++++ 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index 8443d9516cf..1985599b05d 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1387,18 +1387,17 @@ def _create_aggregation( AggregationTemporality.CUMULATIVE ) - if self._boundaries is None: - self._boundaries = ( - instrument._advisory.explicit_bucket_boundaries - or _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES - ) + if self._boundaries: + boundaries = self._boundaries + else: + boundaries = instrument._advisory.explicit_bucket_boundaries return _ExplicitBucketHistogramAggregation( attributes, instrument_aggregation_temporality, start_time_unix_nano, reservoir_factory(_ExplicitBucketHistogramAggregation), - self._boundaries, + boundaries, self._record_min_max, ) diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index 2f46dca87a6..8f063a9b34c 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -164,3 +164,47 @@ def test_explicit_aggregation(self): self.assertEqual( metric.data.data_points[0].explicit_bounds, (1.0, 2.0, 3.0) ) + + def test_explicit_aggregation_multiple_histograms(self): + reader = InMemoryMetricReader( + preferred_aggregation={ + Histogram: ExplicitBucketHistogramAggregation() + } + ) + meter_provider = MeterProvider( + metric_readers=[reader], + ) + meter = meter_provider.get_meter("testmeter") + + histogram1 = meter.create_histogram( + "testhistogram1", + explicit_bucket_boundaries_advisory=[1.0, 2.0, 3.0], + ) + histogram1.record(1, {"label": "value"}) + histogram1.record(2, {"label": "value"}) + histogram1.record(3, {"label": "value"}) + + histogram2 = meter.create_histogram( + "testhistogram2", + explicit_bucket_boundaries_advisory=[4.0, 5.0, 6.0], + ) + histogram2.record(4, {"label": "value"}) + histogram2.record(5, {"label": "value"}) + histogram2.record(6, {"label": "value"}) + + metrics = reader.get_metrics_data() + self.assertEqual(len(metrics.resource_metrics), 1) + self.assertEqual(len(metrics.resource_metrics[0].scope_metrics), 1) + self.assertEqual( + len(metrics.resource_metrics[0].scope_metrics[0].metrics), 2 + ) + metric1 = metrics.resource_metrics[0].scope_metrics[0].metrics[0] + self.assertEqual(metric1.name, "testhistogram1") + self.assertEqual( + metric1.data.data_points[0].explicit_bounds, (1.0, 2.0, 3.0) + ) + metric2 = metrics.resource_metrics[0].scope_metrics[0].metrics[1] + self.assertEqual(metric2.name, "testhistogram2") + self.assertEqual( + metric2.data.data_points[0].explicit_bounds, (4.0, 5.0, 6.0) + ) From 848865e0d65e9b4c6c43c6199c86feaee8d7af00 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20S=C3=A9guy?= <1515987+lseguy@users.noreply.github.com> Date: Wed, 2 Apr 2025 15:19:36 +0200 Subject: [PATCH 2/6] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 07d3121668e..f3307fbc122 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#4494](https://github.com/open-telemetry/opentelemetry-python/pull/4494)) - Improve CI by cancelling stale runs and setting timeouts ([#4498](https://github.com/open-telemetry/opentelemetry-python/pull/4498)) +- Fix ExplicitBucketHistogramAggregation to handle multiple explicit bucket boundaries advisories ([#4521](https://github.com/open-telemetry/opentelemetry-python/pull/4521)) ## Version 1.31.0/0.52b0 (2025-03-12) From 54fa322a602602dd4d9c460add48d225db84f743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20S=C3=A9guy?= <1515987+lseguy@users.noreply.github.com> Date: Wed, 2 Apr 2025 19:04:46 +0200 Subject: [PATCH 3/6] Update CHANGELOG.md MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Emídio Neto <9735060+emdneto@users.noreply.github.com> --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f3307fbc122..1935034aac0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,7 +17,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ([#4494](https://github.com/open-telemetry/opentelemetry-python/pull/4494)) - Improve CI by cancelling stale runs and setting timeouts ([#4498](https://github.com/open-telemetry/opentelemetry-python/pull/4498)) -- Fix ExplicitBucketHistogramAggregation to handle multiple explicit bucket boundaries advisories ([#4521](https://github.com/open-telemetry/opentelemetry-python/pull/4521)) +- Fix ExplicitBucketHistogramAggregation to handle multiple explicit bucket boundaries advisories + ([#4521](https://github.com/open-telemetry/opentelemetry-python/pull/4521)) ## Version 1.31.0/0.52b0 (2025-03-12) From f0bb399e4927c3aee896b2163c35c5d5ed0d2de4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20S=C3=A9guy?= <1515987+lseguy@users.noreply.github.com> Date: Wed, 2 Apr 2025 19:04:55 +0200 Subject: [PATCH 4/6] Update opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py Co-authored-by: Riccardo Magliocchetti --- .../src/opentelemetry/sdk/metrics/_internal/aggregation.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py index 1985599b05d..1779dac0bba 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py @@ -1387,7 +1387,7 @@ def _create_aggregation( AggregationTemporality.CUMULATIVE ) - if self._boundaries: + if self._boundaries is not None: boundaries = self._boundaries else: boundaries = instrument._advisory.explicit_bucket_boundaries From 29c3a0365a58b4603a8ce8b0811dce2b19c9f795 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20S=C3=A9guy?= <1515987+lseguy@users.noreply.github.com> Date: Wed, 2 Apr 2025 19:08:28 +0200 Subject: [PATCH 5/6] Add integration test for default bucket boundaries --- ...est_histogram_advisory_explicit_buckets.py | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index 8f063a9b34c..eeed78602dc 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -15,6 +15,9 @@ from unittest import TestCase from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics._internal.aggregation import ( + _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES, +) from opentelemetry.sdk.metrics._internal.instrument import Histogram from opentelemetry.sdk.metrics.export import InMemoryMetricReader from opentelemetry.sdk.metrics.view import ( @@ -208,3 +211,34 @@ def test_explicit_aggregation_multiple_histograms(self): self.assertEqual( metric2.data.data_points[0].explicit_bounds, (4.0, 5.0, 6.0) ) + + def test_explicit_aggregation_default_boundaries(self): + reader = InMemoryMetricReader( + preferred_aggregation={ + Histogram: ExplicitBucketHistogramAggregation() + } + ) + meter_provider = MeterProvider( + metric_readers=[reader], + ) + meter = meter_provider.get_meter("testmeter") + + histogram1 = meter.create_histogram( + "testhistogram", + ) + histogram1.record(1, {"label": "value"}) + histogram1.record(2, {"label": "value"}) + histogram1.record(3, {"label": "value"}) + + metrics = reader.get_metrics_data() + self.assertEqual(len(metrics.resource_metrics), 1) + self.assertEqual(len(metrics.resource_metrics[0].scope_metrics), 1) + self.assertEqual( + len(metrics.resource_metrics[0].scope_metrics[0].metrics), 1 + ) + metric = metrics.resource_metrics[0].scope_metrics[0].metrics[0] + self.assertEqual(metric.name, "testhistogram") + self.assertEqual( + metric.data.data_points[0].explicit_bounds, + _DEFAULT_EXPLICIT_BUCKET_HISTOGRAM_AGGREGATION_BOUNDARIES, + ) From cd86bebd2235f2062753cb5529d599ca2c38fa4d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Louis=20S=C3=A9guy?= <1515987+lseguy@users.noreply.github.com> Date: Wed, 2 Apr 2025 19:10:55 +0200 Subject: [PATCH 6/6] Renaming variable --- .../test_histogram_advisory_explicit_buckets.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py index eeed78602dc..569d7fd1c2c 100644 --- a/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py +++ b/opentelemetry-sdk/tests/metrics/integration_test/test_histogram_advisory_explicit_buckets.py @@ -223,12 +223,12 @@ def test_explicit_aggregation_default_boundaries(self): ) meter = meter_provider.get_meter("testmeter") - histogram1 = meter.create_histogram( + histogram = meter.create_histogram( "testhistogram", ) - histogram1.record(1, {"label": "value"}) - histogram1.record(2, {"label": "value"}) - histogram1.record(3, {"label": "value"}) + histogram.record(1, {"label": "value"}) + histogram.record(2, {"label": "value"}) + histogram.record(3, {"label": "value"}) metrics = reader.get_metrics_data() self.assertEqual(len(metrics.resource_metrics), 1)