Skip to content

sdk/metric/exemplar: skip Offer on a zero-sized FixedSizeReservoir#8243

Closed
SAY-5 wants to merge 1 commit into
open-telemetry:mainfrom
SAY-5:fix/fixed-size-reservoir-zero-8232
Closed

sdk/metric/exemplar: skip Offer on a zero-sized FixedSizeReservoir#8243
SAY-5 wants to merge 1 commit into
open-telemetry:mainfrom
SAY-5:fix/fixed-size-reservoir-zero-8232

Conversation

@SAY-5
Copy link
Copy Markdown

@SAY-5 SAY-5 commented Apr 20, 2026

Summary

NewFixedSizeReservoir already clamps k to 0 when a caller passes a negative value, but FixedSizeReservoir.Offer does not defend against r.k == 0. After a single call, count reaches next and the random-index branch runs:

idx := rand.IntN(int(r.k)) // rand.IntN(0) panics

which panics with invalid argument to IntN. The SDK doesn't expose this path today, but callers that hand-roll a reservoir (or reach it through a custom ReservoirProvider) can — exactly the case Copilot flagged on #8230 and tracked in this issue.

Fixes #8232.

Change

Short-circuit Offer when r.k == 0:

if r.k == 0 {
    return
}

Collect is already safe on an empty storage, so a zero-sized reservoir now silently drops every exemplar instead of panicking.

Testing

  • go test ./sdk/metric/exemplar/... passes.
  • Added a local Offer regression against a reservoir built with NewFixedSizeReservoir(0) and then called Offer 100 times; previously it panicked on the second call, now it runs cleanly and Collect returns an empty slice.

NewFixedSizeReservoir already clamps k to 0 when the caller passes
a negative value, but FixedSizeReservoir.Offer doesn't defend
against k == 0: after the first call, count reaches next and the
random-index branch runs rand.IntN(int(r.k)) = rand.IntN(0), which
panics with 'invalid argument to IntN'. The SDK path doesn't expose
this today, but callers that hand-roll a reservoir (or reach it
through a custom ReservoirProvider) can, as Copilot noted on
open-telemetry#8230 / open-telemetry#8232.

Short-circuit Offer when r.k == 0. Collect is already safe on an
empty storage, so a zero-sized reservoir now silently drops every
exemplar instead of panicking.

Signed-off-by: SAY-5 <SAY-5@users.noreply.github.com>

Fixes open-telemetry#8232
@dashpole
Copy link
Copy Markdown
Collaborator

Needs a unit test, and the giant comment probably isn't necessary. A one-line comment should be sufficient.

@dashpole dashpole added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Apr 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.7%. Comparing base (5bd5702) to head (6465356).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #8243   +/-   ##
=====================================
  Coverage   82.6%   82.7%           
=====================================
  Files        310     310           
  Lines      24648   24650    +2     
=====================================
+ Hits       20381   20386    +5     
+ Misses      3890    3887    -3     
  Partials     377     377           
Files with missing lines Coverage Δ
sdk/metric/exemplar/fixed_size_reservoir.go 89.4% <100.0%> (+0.3%) ⬆️

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

@pellared pellared left a comment

Choose a reason for hiding this comment

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

@SAY-5
Copy link
Copy Markdown
Author

SAY-5 commented Apr 25, 2026

@pellared sorry for the duplicate. To answer your questions directly:

@SAY-5 SAY-5 closed this Apr 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Gracefully handle FixedSizeReservoir of size 0

3 participants