Skip to content

fix: off-by-one bug for FixedSizeReservoir#8309

Draft
dashpole wants to merge 1 commit into
open-telemetry:mainfrom
dashpole:fix_algl
Draft

fix: off-by-one bug for FixedSizeReservoir#8309
dashpole wants to merge 1 commit into
open-telemetry:mainfrom
dashpole:fix_algl

Conversation

@dashpole
Copy link
Copy Markdown
Collaborator

@dashpole dashpole commented May 6, 2026

While working on #8306, I found an off-by-one error.

Reset was setting r.next to k, and then calling advance(), which meant r.next was always at least k+1.

The first k exemplars are unconditionally stored during the "fill" phase. Count is incremented at the end of the Offer call, so the k+1th exemplar was never stored because r.next is k when that Offer call is being evaluated.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes an off-by-one error in the FixedSizeReservoir (Algorithm L) reset initialization that previously made the first measurement after the reservoir filled (the k+1th offer) impossible to sample, and adds a regression test plus a changelog entry.

Changes:

  • Fix FixedSizeReservoir.reset() initialization of r.next so the first post-fill replacement opportunity can occur at the correct count.
  • Add a regression test that would deterministically fail under the prior behavior (second offered item never sampled for k=1).
  • Document the fix in CHANGELOG.md.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
sdk/metric/exemplar/fixed_size_reservoir.go Adjusts reset() to initialize r.next as cap-1 so the subsequent advance() produces the correct first eligible replacement count.
sdk/metric/exemplar/fixed_size_reservoir_test.go Adds a regression test ensuring the k+1th offer can be sampled (verifies the off-by-one is fixed).
CHANGELOG.md Adds a “Fixed” entry describing the off-by-one bug and its user-visible impact.

@codecov
Copy link
Copy Markdown

codecov Bot commented May 6, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.9%. Comparing base (4086701) to head (74e40d7).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #8309   +/-   ##
=====================================
  Coverage   82.9%   82.9%           
=====================================
  Files        314     314           
  Lines      24985   24985           
=====================================
+ Hits       20730   20733    +3     
+ Misses      3882    3880    -2     
+ Partials     373     372    -1     
Files with missing lines Coverage Δ
sdk/metric/exemplar/fixed_size_reservoir.go 94.7% <100.0%> (ø)

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

@dashpole
Copy link
Copy Markdown
Collaborator Author

dashpole commented May 6, 2026

Test failure is because this needs #8295

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants