fix(sdk): handle unmapped aggregations defensively in MetricReaderStorage.collect by dlowzzxx · Pull Request #5622 · open-telemetry/opentelemetry-python

dlowzzxx · 2026-09-04T16:30:45Z

Description

Related to #5157

Context

In opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/metric_reader_storage.py, MetricReaderStorage.collect() iterates through view instrument matches and assigns data depending on the aggregation instance. When an unmapped or custom aggregation was encountered, no branch was taken, leaving data unassigned and resulting in UnboundLocalError: cannot access local variable 'data' where it is not associated with a value when constructing Metric(...).

Changes

MetricReaderStorage: Added an else: fallback in the aggregation isinstance chain in collect(). Deduplicates warnings using self._unsupported_aggregation_warned so the warning is logged only once per (instrument, type(_aggregation)) pair, avoiding log/stderr flooding during periodic export cycles.
Periodic Exporting Metric Reader Tests: Replaced try/finally with self.addCleanup(pmr.shutdown) in test_exporter_temporality_preference and test_exporter_aggregation_preference.
Tests: Added test_collect_skips_unsupported_aggregation to test_metric_reader_storage.py (avoiding module-level mock of _ViewInstrumentMatch by directly populating _instrument_view_instrument_matches) and asserted that unsupported aggregations are skipped and repeat collections do not re-emit the warning.
Changelog Fragment: Added .changelog/5622.fixed.

…aggregations (open-telemetry#5157)

linux-foundation-easycla · 2026-09-04T16:30:54Z

The committers listed above are authorized under a signed CLA.

✅ login: dlowzzxx / name: dlowzzxx (1362fa4, 1a2c139, 2ec9a74)

…te double shutdown warning

opentelemetry-pr-dashboard · 2026-09-04T17:28:06Z

Pull request dashboard status

Waiting on the author · refreshed 2026-09-13 22:53 UTC

Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):

Top-level threads: 1

Status above doesn't look right?

Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
Should this be with reviewers? Comment /dashboard route:reviewers to route it to them.
Anything wrong — including the routing? Report it with what you expected; it helps us improve the dashboard.

ocelotl · 2026-09-04T18:45:29Z

please sign the CLA

ocelotl · 2026-09-04T19:03:21Z

how does this PR fix #5157?

dlowzzxx · 2026-09-04T21:23:51Z

@ocelotl Thank you for reviewing!

Regarding how this PR addresses #5157:

The traceback captured in #5157 shows two symptoms occurring together:

\AssertionError: 2 != 1\ in \TestMetricReaderStorage.test_creates_view_instrument_matches\
\PytestUnhandledThreadExceptionWarning: Exception in thread OtelPeriodicExportingMetricReader: UnboundLocalError: cannot access local variable 'data' where it is not associated with a value\ in \metric_reader_storage.py:219.

These two failures share a single root cause:

Tests in \ est_periodic_exporting_metric_reader.py\ spawned \PeriodicExportingMetricReader\ instances whose daemon background ticker threads (_ticker) were not explicitly joined/shut down in test teardown.
While \TestMetricReaderStorage\ was executing, a leaked background ticker thread concurrently invoked \collect()\ -> \storage.consume_measurement(). This triggered _ViewInstrumentMatch\ a second time, causing \AssertionError: 2 != 1. Furthermore, because the mock aggregations in that test did not produce a matching aggregation case in \MetricReaderStorage.collect(), the concurrent call referenced an unassigned \data\ variable, raising \UnboundLocalError.

This PR fixes both:

Adds explicit
eader.shutdown()\ in \inally\ blocks across \ est_periodic_exporting_metric_reader.py, preventing the background ticker thread leak.
Adds defensive fallback in \MetricReaderStorage.collect()\ so unmapped aggregations do not crash with \UnboundLocalError.

dlowzzxx · 2026-09-06T12:22:10Z

/dashboard route:reviewers

opentelemetry-pr-dashboard · 2026-09-06T13:20:10Z

@dlowzzxx, this pull request was routed to reviewers.

ocelotl · 2026-09-09T00:02:02Z

Ok, I looked into this, and I think this PR is valuable, but it still does not fix #5157.

You are right that there is a leaked ticker thread, but it does not cause the issue here in the way this PR is intended to fix it:

@patch("...metric_reader_storage._ViewInstrumentMatch") rebinds a module attribute process wide, so any MeterProvider that builds a view instrument match during that window gets a Mock, and every construction anywhere appends to the same MockViewInstrumentMatch.call_args_list that the test is counting. That causes the 2 != 1. The Mock also gets stored, its type does not match the ones we check for, and that is what raises UnboundLocalError on a later tick.

#5157 is 2 separate problems, I am adding their fixes here:

Tests in test_metric_reader_storage.py assert on process-global mock state instead of on the storage under test #5638 / Assert on the storage under test, not on process-global mock state, in test_metric_reader_storage.py #5639, tests asserting on process global mock state
Nothing detects a test that leaks a PeriodicExportingMetricReader ticker thread, and four tests leak one today #5640 / Fail any test that leaks a PeriodicExportingMetricReader ticker thread, and fix the four that do #5641, no detector for leaked ticker threads

I'll comment in this PR diff with the parts of this PR I find valuable.

ocelotl · 2026-09-09T00:04:52Z

                            data_points=data_points,
                            aggregation_temporality=aggregation_temporality,
                        )
+                    else:


This is a genuine bug fix, Aggregation is a public API so we can expect to handle this case.

A user with a custom Aggregation would get a warning with every export, so we probably want to reconsider this particular approach.

Addressed: warnings are now deduplicated via self._unsupported_aggregation_warned using (instrument, type(view_instrument_match._aggregation)) as the key, ensuring unmapped or custom aggregations only log a warning once per instrument.

I would suggest another approach: take look here.

I think it is better to put the check this PR attempts to introduce there as well. If we use the current approach from this PR the warning shows once and only if the process starts after its logging is set up, if not the warning is lost and the user never realizes their data is gone. Here we can just keep the else/continue.

Addressed: moved the unsupported aggregation check to _check_view_instrument_compatibility (returning False so the incompatible view is not applied, matching #5461) and kept the clean else: continue fallback in MetricReaderStorage.collect().

ocelotl · 2026-09-09T00:05:45Z

@@ -0,0 +1 @@
+`opentelemetry-sdk`: log a warning and skip unmapped aggregations in `MetricReaderStorage.collect` to prevent `UnboundLocalError`, and shut down periodic metric readers in tests to prevent background daemon thread leaks.


This should be updated, test case changes are not something we want to tell users about.

Updated: simplified the changelog fragment to describe only user-facing SDK behavior (skip unmapped aggregations in MetricReaderStorage.collect and log a warning once per instrument to prevent UnboundLocalError).

ocelotl · 2026-09-09T00:24:24Z

@@ -245,11 +245,14 @@ def test_exporter_temporality_preference(self):
            },
        )
        pmr = PeriodicExportingMetricReader(exporter)


Consider self.addCleanup(pmr.shutdown) as in #5641

Updated to self.addCleanup(pmr.shutdown) as in #5641.

ocelotl · 2026-09-09T00:24:39Z

@@ -258,11 +261,14 @@ def test_exporter_aggregation_preference(self):
            },
        )
        pmr = PeriodicExportingMetricReader(exporter)


Updated to self.addCleanup(pmr.shutdown) as in #5641.

ocelotl

This PR contains valuable changes but needs refactoring, its changes are also not a fix for the target issue.

… review comments

dlowzzxx · 2026-09-09T06:27:40Z

@ocelotl Thank you for the detailed review and clarifying the root cause for #5157!

I have refactored this PR according to your feedback:

Warning Deduplication (MetricReaderStorage.collect):
- Added self._unsupported_aggregation_warned: set[tuple[_Instrument, type[_Aggregation]]] = set() in MetricReaderStorage.__init__.
- In MetricReaderStorage.collect(), the warning is now emitted only once per (instrument, type(view_instrument_match._aggregation)) pair, preventing log/stderr flooding during periodic export cycles.
Test Refactoring (test_metric_reader_storage.py):
- Removed @patch("opentelemetry.sdk.metrics._internal.metric_reader_storage._ViewInstrumentMatch") to eliminate process-wide class patching.
- Directly populated storage._instrument_view_instrument_matches with the test matches.
- Added assertions to verify that repeated calls to storage.collect() do not re-emit the warning.
Cleanup Pattern (test_periodic_exporting_metric_reader.py):
- Replaced try/finally: pmr.shutdown() with self.addCleanup(pmr.shutdown) in test_exporter_temporality_preference and test_exporter_aggregation_preference.
Changelog Fragment (.changelog/5622.fixed):
- Simplified to focus strictly on user-facing SDK changes, removing references to test cleanups.
PR Title:
- Updated title to fix(sdk): handle unmapped aggregations defensively in MetricReaderStorage.collect.

/dashboard route:reviewers

dlowzzxx · 2026-09-09T06:37:57Z

@ocelotl All review feedback and inline threads have been addressed and updated:

Warning Deduplication (MetricReaderStorage.collect):
- Added self._unsupported_aggregation_warned: set[tuple[_Instrument, type[_Aggregation]]] = set() in MetricReaderStorage.__init__.
- In MetricReaderStorage.collect(), deduplicated warnings using (instrument, type(view_instrument_match._aggregation)) as the key, ensuring unmapped or custom aggregations log a warning at most once per instrument rather than flooding on periodic export cycles.
Test Refactoring (test_metric_reader_storage.py):
- Removed process-wide @patch("..._ViewInstrumentMatch").
- Populated storage._instrument_view_instrument_matches directly with test matches.
- Scoped assertLogs and assertNoLogs to "opentelemetry.sdk.metrics._internal.metric_reader_storage" to prevent ambient thread log pollution.
- Verified that repeated storage.collect() calls do not re-emit the warning, that internal state is recorded, and that subsequent new instruments with unsupported aggregations still emit a warning once.
Cleanup Pattern (test_periodic_exporting_metric_reader.py):
- Replaced try/finally: pmr.shutdown() with self.addCleanup(pmr.shutdown) in test_exporter_temporality_preference and test_exporter_aggregation_preference to match Fail any test that leaks a PeriodicExportingMetricReader ticker thread, and fix the four that do #5641.
- Reverted extraneous modifications in test_metric_reader_metrics, keeping the diff strictly isolated to the two preference tests.
Changelog Fragment (.changelog/5622.fixed):
- Simplified to focus strictly on user-facing SDK behavior: `opentelemetry-sdk`: skip unmapped aggregations in `MetricReaderStorage.collect` and log a warning once per instrument to prevent `UnboundLocalError`.
Inline Threads & PR Title:
- Replied to all inline review threads and updated the PR title.

Latest commit: b232b18

/dashboard route:reviewers

ocelotl · 2026-09-09T17:02:44Z

thanks! please resolve the conflicts ✌️ so I can continue reviewing and approving

…y-python into fix/metrics-test-thread-leak-collector-fallback-5157

dlowzzxx · 2026-09-09T17:52:23Z

@ocelotl Conflicts with main have been resolved cleanly! All tests pass. Ready for your review and approval ✌️

/dashboard route:reviewers

ocelotl · 2026-09-11T17:09:58Z

+                        warn_key = (instrument, type(view_instrument_match._aggregation))
+                        if warn_key not in self._unsupported_aggregation_warned:
+                            self._unsupported_aggregation_warned.add(warn_key)
+                            _logger.warning(


This would render something like this

Unsupported aggregation <opentelemetry.sdk.metrics._internal.aggregation._SumAggregation object at 0x79db26bccc20> for instrument <opentelemetry.sdk.metrics._internal.instrument._Counter object at 0x79db26bccad0>

Better to use type(view_instrument_match._aggregation).__name__ and instrument.name.

Addressed: updated warning formatting to use ype(view._aggregation).name and instrument.name.

…rument_compatibility (open-telemetry#5622)

dlowzzxx · 2026-09-11T20:55:15Z

@ocelotl Thank you for the review and guidance!

I have updated the implementation to align with PR #5461:

View Compatibility Check: Moved the unsupported aggregation check to _check_view_instrument_compatibility. When view._aggregation is an Aggregation instance that is not one of the SDK-supported aggregations (DefaultAggregation, DropAggregation, ExplicitBucketHistogramAggregation, ExponentialBucketHistogramAggregation, LastValueAggregation, SumAggregation), a warning is logged:
"Unsupported aggregation %s for instrument %s"
using type(view._aggregation).__name__ and instrument.name, and the method returns False so the incompatible view is not applied.
Defensive Collector Fallback: In MetricReaderStorage.collect(), retained the simple else: continue branch to cleanly guard against UnboundLocalError without duplicate logging during collection. Removed self._unsupported_aggregation_warned.
Tests: Added test_unsupported_aggregation_view_not_applied to assert view rejection and warning logging, and verified test_collect_skips_unsupported_aggregation. All 324 metrics tests pass locally, with clean ruff linting and formatting.

Commit: d7ada04

/dashboard route:reviewers

…ad-leak-collector-fallback-5157

…ng main

dlowzzxx · 2026-09-13T20:07:47Z

Rebased on latest main, updated the _LastValueAggregation constructor arguments following the merge of #5637, and verified all tests pass. Ready for your review!

fix(metrics): shutdown periodic readers in tests and handle unmapped …

2ec9a74

…aggregations (open-telemetry#5157)

dlowzzxx requested a review from a team as a code owner September 4, 2026 16:30

otelbot-python Bot added this to Python PR digest Sep 4, 2026

fix(metrics): rename changelog fragment to PR number 5622 and elimina…

1a2c139

…te double shutdown warning

opentelemetry-pr-dashboard Bot mentioned this pull request Sep 4, 2026

Pull Request Dashboard #5550

Open

chore: retrigger easycla check

1362fa4

ocelotl mentioned this pull request Sep 8, 2026

Fail any test that leaks a PeriodicExportingMetricReader ticker thread, and fix the four that do #5641

Open

ocelotl reviewed Sep 9, 2026

View reviewed changes

ocelotl suggested changes Sep 9, 2026

View reviewed changes

github-project-automation Bot moved this to Reviewed PRs that need fixes in Python PR digest Sep 9, 2026

fix(metrics): deduplicate unsupported aggregation warning and address…

8828d34

… review comments

dlowzzxx changed the title ~~fix(metrics): shutdown periodic readers in tests and handle unmapped aggregations (#5157)~~ fix(sdk): handle unmapped aggregations defensively in MetricReaderStorage.collect Sep 9, 2026

test(metrics): clean up reader metrics test and scope log assertions

b232b18

Merge branch 'main' of https://github.com/open-telemetry/opentelemetr…

35ad30d

…y-python into fix/metrics-test-thread-leak-collector-fallback-5157

ocelotl reviewed Sep 11, 2026

View reviewed changes

ocelotl suggested changes Sep 11, 2026

View reviewed changes

fix(sdk): validate view aggregation compatibility in _check_view_inst…

d7ada04

…rument_compatibility (open-telemetry#5622)

dlowzzxx added 2 commits September 13, 2026 22:06

Merge remote-tracking branch 'origin/main' into fix/metrics-test-thre…

8214967

…ad-leak-collector-fallback-5157

test(metrics): update _LastValueAggregation instantiation after mergi…

822e57e

…ng main

		@@ -0,0 +1 @@
		`opentelemetry-sdk`: log a warning and skip unmapped aggregations in `MetricReaderStorage.collect` to prevent `UnboundLocalError`, and shut down periodic metric readers in tests to prevent background daemon thread leaks.

Uh oh!

Conversation

dlowzzxx commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Context

Changes

Uh oh!

linux-foundation-easycla Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

opentelemetry-pr-dashboard Bot commented Sep 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request dashboard status

Uh oh!

ocelotl commented Sep 4, 2026

Uh oh!

ocelotl commented Sep 4, 2026

Uh oh!

dlowzzxx commented Sep 4, 2026

Uh oh!

dlowzzxx commented Sep 6, 2026

Uh oh!

opentelemetry-pr-dashboard Bot commented Sep 6, 2026

Uh oh!

ocelotl commented Sep 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

dlowzzxx commented Sep 9, 2026

Uh oh!

dlowzzxx commented Sep 9, 2026

Uh oh!

ocelotl commented Sep 9, 2026

Uh oh!

dlowzzxx commented Sep 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlowzzxx commented Sep 11, 2026

Uh oh!

dlowzzxx commented Sep 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dlowzzxx commented Sep 4, 2026 •

edited

Loading

linux-foundation-easycla Bot commented Sep 4, 2026 •

edited

Loading

opentelemetry-pr-dashboard Bot commented Sep 4, 2026 •

edited

Loading