[dlowzzxx]

Description

Related to #5157

Prevent MetricReaderStorage.collect from using unassigned or stale metric data when it encounters an unsupported aggregation. Drop and unsupported aggregations are skipped, while supported metrics are still collected

Reject unsupported custom aggregation views during initialization, log a warning and use the default view instead. The tests cover both fallback and collection with supported and unsupported matches

Also use addCleanup to shut down the periodic exporting reader in the two affected preference tests. The changelog remains .changelog/5622.fixed

Type of change

• Bug fix

Verification

Refreshed with upstream main and pushed at 58107d0224cb391129144e27355a249e94409bcb on October 14

Focused metric storage tests pass, 20 tests. The SDK metrics suite passes 332 tests plus 44 subtests, with 11 platform skips on Windows. Two shutdown warnings are emitted after the successful suite summary

Fresh CI identified Pylint findings for renamed override parameters and an extra collection branch. The override now uses the base parameter names, and the redundant drop-specific branch was removed because the final fallback already skips it

Pylint 3.3.4 passes on both changed files with only the Windows checkout line-ending warning disabled. Ruff 0.16.0 lint and formatting, and whitespace checks pass. Full local tox lint is not a passing result because of checkout-wide CRLF warnings and os.fork findings in untouched platform-specific files

Full precommit, typecheck and the official dependency matrix were not rerun locally. The latest GitHub check rollup is SUCCESS across 579 contexts. Human re-review remains required

Checklist

• I am a human and not a bot
• I have run tox -e precommit
• I have run tox -e typecheck
• I have run the relevant tests and linting tox environments

The refresh used OpenAI Codex assistance, disclosed in commit trailers

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

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

[opentelemetry-pr-dashboard]

Pull request dashboard status

Waiting on the author · refreshed 2026-10-17 02:50 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]

please sign the CLA

[ocelotl]

how does this PR fix #5157?

[dlowzzxx]

@ocelotl Thank you for reviewing!

Regarding how this PR addresses #5157:

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

1. \AssertionError: 2 != 1\ in \TestMetricReaderStorage.test_creates_view_instrument_matches\
2. \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:

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

[dlowzzxx]

/dashboard route:reviewers

[opentelemetry-pr-dashboard]

@dlowzzxx, this pull request was routed to reviewers.

[ocelotl]

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:

1. 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
2. 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]

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.

[dlowzzxx]

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.

[ocelotl]

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.

[dlowzzxx]

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]

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

[dlowzzxx]

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]

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

[dlowzzxx]

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

[ocelotl]

Same here

[dlowzzxx]

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.

[dlowzzxx]

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

I have refactored this PR according to your feedback:

1. 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.

2. 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.

3. 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.

4. Changelog Fragment (.changelog/5622.fixed):

   • Simplified to focus strictly on user-facing SDK changes, removing references to test cleanups.

5. PR Title:

   • Updated title to fix(sdk): handle unmapped aggregations defensively in MetricReaderStorage.collect.

/dashboard route:reviewers

[dlowzzxx]

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

1. 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.

2. 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.

3. 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.

4. 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`.

5. Inline Threads & PR Title:

   • Replied to all inline review threads and updated the PR title.

Latest commit: b232b18

/dashboard route:reviewers

[ocelotl]

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

[dlowzzxx]

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

/dashboard route:reviewers

[ocelotl]

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.

[dlowzzxx]

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

[dlowzzxx]

@ocelotl Thank you for the review and guidance!

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

1. 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.
2. 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.
3. 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

[dlowzzxx]

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

[opentelemetry-pr-dashboard]

Hi @dlowzzxx — just a friendly reminder that this pull request is waiting on you. The dashboard status comment has the open items and is kept current.

• Replying is enough to hand it off — answer, explain why no change is needed, or ask a follow-up. The dashboard routes it onward once nothing on the list is waiting on you.
• To hand it back for any other reason, including the dashboard getting this wrong, comment /dashboard route:reviewers.