Skip to content

feat: use time-unbiased algorithm for histogram reservoir#8306

Draft
dashpole wants to merge 3 commits into
open-telemetry:mainfrom
dashpole:simple_hist_timeunbiased
Draft

feat: use time-unbiased algorithm for histogram reservoir#8306
dashpole wants to merge 3 commits into
open-telemetry:mainfrom
dashpole:simple_hist_timeunbiased

Conversation

@dashpole
Copy link
Copy Markdown
Collaborator

@dashpole dashpole commented May 5, 2026

Fixes #8236

Factors out algorithm L into a nextTracker, and uses it for both the histogram reservoir and the fixed-size reservoir.

Benchmarks:

goos: linux
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/metric/exemplar
cpu: AMD EPYC 7B12
                           │  main.txt   │         new_embedded_2.txt          │
                           │   sec/op    │    sec/op     vs base               │
FixedSizeReservoirOffer-24   186.0n ± 4%   173.5n ±  3%   -6.75% (p=0.002 n=6)
HistogramReservoirOffer-24   46.39n ± 4%   20.35n ± 11%  -56.14% (p=0.002 n=6)
geomean                      92.91n        59.42n        -36.04%

Gemini helped write this.

I'll wait to mark this ready for review until after #8309

@codecov
Copy link
Copy Markdown

codecov Bot commented May 5, 2026

Codecov Report

❌ Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.9%. Comparing base (4086701) to head (c852a82).

Files with missing lines Patch % Lines
sdk/metric/exemplar/next_tracker.go 93.1% 1 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #8306   +/-   ##
=====================================
  Coverage   82.9%   82.9%           
=====================================
  Files        314     315    +1     
  Lines      24985   25005   +20     
=====================================
+ Hits       20730   20749   +19     
  Misses      3882    3882           
- Partials     373     374    +1     
Files with missing lines Coverage Δ
sdk/metric/exemplar/fixed_size_reservoir.go 100.0% <100.0%> (+5.2%) ⬆️
sdk/metric/exemplar/histogram_reservoir.go 94.8% <100.0%> (+3.9%) ⬆️
sdk/metric/exemplar/storage.go 100.0% <100.0%> (ø)
sdk/metric/exemplar/next_tracker.go 93.1% <93.1%> (ø)

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

This PR updates the SDK metric exemplar reservoirs to use a time-unbiased reservoir sampling approach (Algorithm L) for histogram buckets, while factoring the sampling logic into a shared nextTracker used by both histogram and fixed-size reservoirs.

Changes:

  • Introduces nextTracker implementing Algorithm L and reuses it across reservoirs.
  • Updates FixedSizeReservoir and HistogramReservoir to use nextTracker-based sampling and refactors storage/locking accordingly.
  • Adds a statistical test asserting time-unbiased behavior for the histogram reservoir.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
sdk/metric/exemplar/storage.go Removes the old shared storage wrapper and moves storage logic onto measurement.
sdk/metric/exemplar/next_tracker.go Adds the shared Algorithm L sampling tracker.
sdk/metric/exemplar/histogram_reservoir.go Switches histogram reservoir sampling from “last seen” to Algorithm L per bucket.
sdk/metric/exemplar/histogram_reservoir_test.go Adds a statistical test to validate time-unbiased sampling behavior.
sdk/metric/exemplar/fixed_size_reservoir.go Refactors fixed-size reservoir to use nextTracker and inline storage.
sdk/metric/exemplar/fixed_size_reservoir_test.go Updates tests to reflect the new internal storage layout.
sdk/metric/exemplar/benchmark_test.go Updates benchmark reset logic to use the new tracker.

Comment thread sdk/metric/exemplar/next_tracker.go
Comment thread sdk/metric/exemplar/next_tracker.go Outdated
Comment thread sdk/metric/exemplar/histogram_reservoir.go Outdated
Comment thread sdk/metric/exemplar/fixed_size_reservoir.go
@dashpole dashpole force-pushed the simple_hist_timeunbiased branch from beb31f5 to 0211b4d Compare May 6, 2026 02:30
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.

Histogram Reservoir should use a time-unbiased sampling algorithm

2 participants