Skip to content

Recommend using a time-unbiased reservoir sampling algorithm for histograms#4678

Merged
carlosalberto merged 7 commits intoopen-telemetry:mainfrom
dashpole:time_based_histogram
Oct 28, 2025
Merged

Recommend using a time-unbiased reservoir sampling algorithm for histograms#4678
carlosalberto merged 7 commits intoopen-telemetry:mainfrom
dashpole:time_based_histogram

Conversation

@dashpole
Copy link
Copy Markdown
Contributor

@dashpole dashpole commented Oct 6, 2025 •
edited
Loading

Fixes #4675

Changes

Change the recommended algorithm for histogram reservoirs to be time-unbiased.

I've left the previous algorithm as an option to ensure this change is backwards-compatible.

Go prototype: open-telemetry/opentelemetry-go#7458

  • Links to the prototypes (when adding or changing features)
  • CHANGELOG.md file updated for non-trivial changes

@dashpole dashpole force-pushed the time_based_histogram branch from de93eba to 80f9b71 Compare October 6, 2025 14:00
@dashpole dashpole force-pushed the time_based_histogram branch from 80f9b71 to 106246d Compare October 6, 2025 14:02
@dashpole dashpole mentioned this pull request Oct 6, 2025
Closed
@dashpole dashpole marked this pull request as ready for review October 6, 2025 16:05
@dashpole dashpole requested review from a team as code owners October 6, 2025 16:05
MrAlias
MrAlias approved these changes Oct 6, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

Looks good overall.

Comment thread specification/metrics/sdk.md Outdated
Comment thread specification/metrics/sdk.md Outdated
@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Oct 7, 2025

There is a concern from @tigrannajaryan that this is a breaking change. @jsuereth says there is language that allows us to make this change in the spec

@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Oct 7, 2025 •
edited
Loading

Feedback from @austinlparker: We should be careful about how we communicate this to users. Is there a way to make this opt-in for users?

@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Oct 7, 2025

@carlosalberto requests that we leave this open for comment for a while.

@dashpole
Copy link
Copy Markdown
Contributor Author

dashpole commented Oct 7, 2025

From https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/metrics/sdk.md#exemplar-defaults

Exemplar default reservoirs MAY change in a minor version bump. No guarantees are made on the shape or statistical properties of returned exemplars.

jmacd
jmacd approved these changes Oct 7, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@jmacd jmacd left a comment

Choose a reason for hiding this comment

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

Nice. This was a goal of mine in the original version, glad to see it improve.
I believe this is Jeffrey Vitter's "Algorithm R", for the record.

@jmacd
Copy link
Copy Markdown
Contributor

jmacd commented Oct 7, 2025

I might call this "time-unbiased" or "temporal uniform", not time weighted.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Oct 17, 2025

This PR was marked stale due to lack of activity. It will be closed in 7 days.

@github-actions github-actions Bot added the Stale label Oct 17, 2025
@dashpole dashpole removed the Stale label Oct 17, 2025
@carlosalberto
Copy link
Copy Markdown
Contributor

carlosalberto commented Oct 21, 2025

I think we are ready to merge this.

@dashpole dashpole changed the title Recommend using a time-weighted reservoir sampling algorithm for histograms Recommend using a time-unbiased reservoir sampling algorithm for histograms Oct 27, 2025
@carlosalberto carlosalberto added this pull request to the merge queue Oct 28, 2025
Merged via the queue into open-telemetry:main with commit c77fd07 Oct 28, 2025
7 checks passed
@carlosalberto carlosalberto mentioned this pull request Nov 11, 2025
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Nov 17, 2025
@carlosalberto @arminru
Release 1.51.0 (#4730)
5e5f8fd 
### Metrics

- `AlignedHistogramBucketExemplarReservoir` SHOULD use a time-weighted
algorithm.

([#4678](#4678))

### Profiles

- Document the profiles signal.

([#4685](#4685))

### Common

- Extend the set of attribute value types to support more complex data
structures.

([#4651](#4651))

---------

Co-authored-by: Armin Ruech <[email protected]>
@dashpole dashpole mentioned this pull request Apr 20, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jsuereth jsuereth jsuereth approved these changes

@lmolkova lmolkova lmolkova approved these changes

@jmacd jmacd jmacd approved these changes

@MrAlias MrAlias MrAlias approved these changes

Assignees

No one assigned

Labels

changelog.opentelemetry.io

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

AlignedHistogramBucketExemplarReservoir should use time-weighted sampling

7 participants

@dashpole @jmacd @carlosalberto @jsuereth @lmolkova @MrAlias @arminru