Skip to content

opentelemetry-sdk: fill every exemplar reservoir bucket before sampling - #5462

Open
henry3260 wants to merge 1 commit into
open-telemetry:mainfrom
henry3260:fix/exemplar-reservoir-off-by-one
Open

opentelemetry-sdk: fill every exemplar reservoir bucket before sampling#5462
henry3260 wants to merge 1 commit into
open-telemetry:mainfrom
henry3260:fix/exemplar-reservoir-off-by-one

Conversation

@henry3260

@henry3260 henry3260 commented Jul 27, 2026

Copy link
Copy Markdown

Description

SimpleFixedSizeExemplarReservoir implements Algorithm R reservoir sampling, which
has two phases: the first size measurements fill each bucket directly, and every
measurement after that replaces a random bucket with probability size / seen.

_find_bucket_index incremented _measurements_seen before comparing it against
_size, so at the point of the check the counter meant "measurements seen including
this one" rather than "measurements seen before this one". The deterministic fill
phase therefore ran size - 1 times instead of size.

self._measurements_seen += 1
if self._measurements_seen < self._size:   # off by one
    return self._measurements_seen - 1
  1. The uniform sampling guarantee no longer held. Algorithm R's correctness relies
    on the first size measurements always being retained. Losing one of them breaks the
    invariant that every measurement seen so far is retained with equal probability.

size=1 was unaffected (1 < 1 is false, and randrange(0, 1) is always 0).

The fix moves the increment after the comparison so the counter carries the meaning the
algorithm expects at the point of the check. No public API or configuration changes.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • New regression testtest_fills_every_bucket_before_sampling in
    opentelemetry-sdk/tests/metrics/test_exemplarreservoir.py. It offers exactly size
    measurements for size in (1, 2, 4, 10) and asserts that all size buckets are
    filled with exactly those measurements. The test fails on the current code
    (AssertionError: 9 != 10 for size=10) and passes with the fix.

Does This PR Require a Contrib Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

SimpleFixedSizeExemplarReservoir incremented _measurements_seen before
comparing it against _size, so the deterministic fill phase of the
reservoir sampling algorithm ran size - 1 times instead of size.

The size-th measurement therefore entered the random replacement phase
early. Because randrange(0, size) always yields an index below _size,
that measurement overwrote an already-filled bucket with probability
(size - 1) / size and left the last bucket empty. The reservoir never
reached its configured capacity, and the uniformity guarantee of the
algorithm, which depends on the first size measurements always being
retained, no longer held.

Increment the counter after the comparison so that it means "number of
measurements seen before this one" at the point of the check.
@henry3260
henry3260 requested a review from a team as a code owner July 27, 2026 13:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

1 participant