Repository navigation
fix(metrics): reset the exponent mapping between DELTA collections - #5717
inchang-ing wants to merge 5 commits into
Conversation
Pull request dashboard statusWaiting on the author · refreshed 2026-10-18 03:14 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
…etry#5716) Match the unittest-style assertions used by the other exponential histogram tests and add the .changelog fragment required by CI.
|
Done in 1a6912a — switched to unittest-style |
|
Hi maintainers, gentle ping on #5717 — fix(metrics): reset the exponent mapping between DELTA collections. It's a small, self-contained fix with passing tests. Would appreciate a review when you have a moment. Thanks for maintaining open-telemetry/opentelemetry-python! |
… add exhaustive tests - Move the mapping reset to the top of the DELTA/DELTA branch of collect(), grouped with the delta bookkeeping instead of sitting between the empty check and the data point construction. It stays under the lock because aggregate() reads and rewrites self._mapping under the same lock, and it stays inside the delta branch because a cumulative collection needs the mapping to survive across collections. - Make the regression tests more exhaustive: the reset now also survives an interval without records, happens on every collection (alternating wide and narrow intervals), and buckets values with the maximum-scale mapping (the reported bucket offset matches LogarithmMapping(20)).
github-project-automation
Bot
moved this from Ready for review
to Approved PRs
in Python PR digest
There was a problem hiding this comment.
🟢 Approval recommended
The isolated fix has targeted regression coverage and no unresolved review findings.
0 open findings
What changed in this PR
Fixes #5716 by allowing DELTA exponential histograms to regain precision after a wide-range interval.
Changes:
- Resets the exponent mapping between DELTA collections without changing cumulative behavior.
- Adds regression tests for scale recovery and repeated downscaling.
- Adds a changelog entry.
| File | Description |
|---|---|
| opentelemetry-sdk/tests/metrics/exponential_histogram/test_delta_scale_reset.py | Tests scale recovery and repeated downscaling. |
| opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/aggregation.py | Resets the mapping after capturing a DELTA interval. |
| .changelog/5717.fixed | Documents the fix. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # to the maximum (the next interval downscales as needed). | ||
| # Otherwise a single wide interval would pin every later | ||
| # point to its lowest scale (issue #5716). | ||
| self._mapping = self._new_mapping(self._max_scale) |
There was a problem hiding this comment.
Nit: we should move this out of the critical section.
There was a problem hiding this comment.
Done in e3f0e76 — moved it to the top of the DELTA/DELTA branch, grouped with the delta bookkeeping (`previous_collection_start_nano`) instead of sitting between the empty check and the data point construction.
It does stay under `self._lock` though: `aggregate()` reads and rewrites `self._mapping` under the same lock (the rescale path), so an out-of-lock reset could lose a concurrent rescale or leave `index_base` computed with the old mapping. It also stays inside the delta branch because a cumulative collection needs the mapping to survive across collections — hoisting it into the shared reset block above would corrupt CUMULATIVE aggregation. The full metrics suite (335 tests, including the CUMULATIVE integration tests) passes with this placement.
There was a problem hiding this comment.
Please remove the unnecessary comments.
Also, I wasn't talking about the assignment to _mapping. I was referring to creation of the new mapping. Regardless, we can probably keep it where it is.
Description
Fixes #5716.
With DELTA temporality, the exponential histogram aggregation lowered its scale when an interval's values spanned a wide range but never raised it again:
collectreset the buckets and_scalebut not the exponentmapping. Every later delta point was therefore recorded — and reported — at the lowest scale any earlier interval needed, even when it held a single value.The spec says that when either range holds not more than one value, the scale SHOULD be the maximum; a delta point covers only its own interval, so the mapping must not leak across collections.
Fix
In the DELTA→DELTA branch of
_ExponentialBucketHistogramAggregation.collect, resetself._mappingto the maximum scale after the interval is captured. The next interval starts fresh and downscales again as needed. The CUMULATIVE branch is untouched (a cumulative point legitimately spans its whole lifetime, so its mapping keeps lowering).Validation
opentelemetry-sdk/tests/metrics/exponential_histogram/test_delta_scale_reset.py), mirroring the issue's reproduction withInMemoryMetricReader:test_exponential_bucket_histogram_aggregation.py: 22 tests + 5 subtests) passes with the change; CUMULATIVE temporality behavior verified unchanged.