Disable exemplar reservoir for asynchronous instruments by default#8286
Disable exemplar reservoir for asynchronous instruments by default#8286dashpole wants to merge 5 commits intoopen-telemetry:mainfrom
Conversation
…sed filter is used
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8286 +/- ##
=====================================
Coverage 82.9% 82.9%
=====================================
Files 314 314
Lines 24985 24989 +4
=====================================
+ Hits 20730 20736 +6
+ Misses 3882 3880 -2
Partials 373 373
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Disables exemplar reservoir allocation for asynchronous instruments when the default exemplar.TraceBasedFilter is configured (since async observations are recorded without a measurement context, trace-based filtering cannot ever sample), reducing per-attribute-set memory/alloc overhead in sdk/metric.
Changes:
- Pass instrument kind into
reservoirFuncand returnaggregate.DropReservoirfor async instruments whenexemplar.TraceBasedFilteris used. - Expand unit tests to cover AlwaysOn/AlwaysOff/TraceBased across sync vs async instrument kinds.
- Add a benchmark covering async instruments with AlwaysOn vs TraceBased exemplar filters, and document the optimization in the changelog.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/metric/pipeline.go | Passes InstrumentKind into exemplar reservoir factory selection. |
| sdk/metric/exemplar.go | Drops exemplar reservoirs for async instruments under TraceBasedFilter. |
| sdk/metric/exemplar_test.go | Adds table-driven coverage for the updated reservoir selection logic. |
| sdk/metric/benchmark_test.go | Adds benchmark to quantify async attribute-set overhead with different filters. |
| CHANGELOG.md | Notes the user-visible optimization. |
|
@dashpole I went through the changes across reservoirFunc, the updated tests, and the new benchmark. The additional check in reservoirFunc to return DropReservoir for async instrument kinds (ObservableCounter, ObservableUpDownCounter, ObservableGauge) when using the TraceBased filter makes sense, since async observations don’t carry context and therefore cannot produce exemplars. I also verified from the test updates that:
The benchmark results for the async TraceBased case clearly show reduced allocations and memory usage, which aligns well with avoiding unnecessary reservoir creation. Overall, the change looks consistent with the intended behavior and provides a meaningful optimization without affecting existing functionality. |
Fixes #8285
Asynchronous instruments do not accept context, so the default TraceBased exemplar filter can never record an exemplar. Use the DropReservoir when the TraceBased exemplar filter is used, similar to #8211.
Benchmarks
Since callbacks are made as part of collect, there is a significant baseline overhead from collection included in the benchmark.