fix(metrics): size exponential histogram exemplar reservoirs - #2392
LouisDeconinck wants to merge 2 commits into
Conversation
Assisted-by: Codex
|
|
There was a problem hiding this comment.
🟢 Approval recommended
The change directly implements the spec-required default sizing and includes a focused regression test covering key boundary cases.
Pull request overview
This PR fixes the default exemplar reservoir sizing for exponential histograms in metrics_sdk to comply with the OpenTelemetry spec requirement of min(20, max_buckets) rather than using the reservoir’s CPU-count default.
Changes:
- Create the default
SimpleFixedSizeExemplarReservoironly after validatingmax_size, explicitly settingmax_size: [20, @size].min. - Remove the shared
DEFAULT_RESERVOIRconstant so the default reservoir sizing can be derived from the histogram’s validated bucket limit. - Add a regression test covering
max_sizebelow 20, equal to 20, and above 20.
File summaries
| File | Description |
|---|---|
| metrics_sdk/lib/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram.rb | Constructs the default exemplar reservoir with an explicit size capped at 20 based on validated histogram max_size. |
| metrics_sdk/test/opentelemetry/sdk/metrics/aggregation/exponential_bucket_histogram_test.rb | Adds a regression test asserting the default exemplar reservoir size follows the min(20, max_size) rule. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Hi @LouisDeconinck, thank you for your contribution! Could you sign the CLA? |
|
|
||
| # if no reservoir pass from instrument, then use this empty reservoir to avoid no method found error | ||
| DEFAULT_RESERVOIR = Metrics::Exemplar::SimpleFixedSizeExemplarReservoir.new | ||
| private_constant :DEFAULT_RESERVOIR |
There was a problem hiding this comment.
Why to move out of private contstant?
There was a problem hiding this comment.
The default reservoir now needs a per-instance size - SimpleFixedSizeExemplarReservoir.new(max_size: [20, @SiZe].min) - which depends on the histogram's max_size , so it can't be a shared class constant. This also avoids all default-constructed histograms sharing a single mutable reservoir instance.
It would be good to at least somehow verify locally code works before submitting PR. |
|
@kaylareopelle CLA was signed. |
|
Verified locally in Docker (ruby:3.4): |
Fixes #2366.
The default exemplar reservoir for exponential histograms used the reservoir's CPU-count default instead of the specification's
min(20, max_size)sizing rule.This creates the default reservoir after validating
max_size, with an explicit size capped at 20. Custom reservoirs are unchanged. The regression test covers smaller-than-20, equal-to-20, and larger-than-20 histograms.Validation:
git diff --checkAI assistance is disclosed in the commit trailer:
Assisted-by: Codex.