Allow optimizing locking for built-in exemplar reservoirs - #7423
Merged
Conversation
dashpole
requested review from
MrAlias,
XSAM,
dmathieu,
flc1125 and
pellared
as code owners
September 26, 2025 19:02
dashpole
force-pushed
the
change_exemplar_locking
branch
from
September 26, 2025 19:03
a84c8bb to
a1b3abf
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7423 +/- ##
=======================================
- Coverage 85.5% 85.5% -0.1%
=======================================
Files 279 279
Lines 24709 24725 +16
=======================================
+ Hits 21142 21154 +12
- Misses 3188 3192 +4
Partials 379 379
🚀 New features to boost your workflow:
|
Collaborator
Author
|
I think we can ignore the link checker failure. the link works, but the website seems to reject link checkers :) |
dmathieu
approved these changes
Sep 29, 2025
pellared
approved these changes
Sep 29, 2025
Member
|
Can you add some |
MrAlias
reviewed
Sep 29, 2025
dashpole
force-pushed
the
change_exemplar_locking
branch
3 times, most recently
from
September 30, 2025 16:18
076cd9b to
b8d394b
Compare
dashpole
force-pushed
the
change_exemplar_locking
branch
5 times, most recently
from
September 30, 2025 16:56
c925a5d to
f7b1dab
Compare
MrAlias
reviewed
Sep 30, 2025
dashpole
force-pushed
the
change_exemplar_locking
branch
from
October 1, 2025 14:27
f7b1dab to
7b787f8
Compare
dashpole
force-pushed
the
change_exemplar_locking
branch
from
October 1, 2025 14:28
7b787f8 to
d0bf24b
Compare
MrAlias
approved these changes
Oct 1, 2025
MrAlias
left a comment
MrAlias
left a comment
Contributor
There was a problem hiding this comment.
Looks like the linter is failing for a missing package name. But other than that this looks good. 🚀
pellared
pushed a commit
that referenced
this pull request
Oct 3, 2025
This also fixes a bug introduced in #7423, where we were only locking around storage and not around other shared fields (e.g. count). Fixing the bug is required for benchmarks to run properly, but wasn't caught by concurrent safe tests because the SDK does not currently call exemplar methods concurrently. ``` goos: linux goarch: amd64 pkg: go.opentelemetry.io/otel/sdk/metric/exemplar cpu: Intel(R) Xeon(R) CPU @ 2.20GHz BenchmarkFixedSizeReservoirOffer-24 498955 248.4 ns/op 0 B/op 0 allocs/op BenchmarkHistogramReservoirOffer-24 478068 250.1 ns/op 0 B/op 0 allocs/op ```
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7388
Benchmarks seem like mostly noise. It isn't actually necessary to lock in the exemplar reservoir today because of our SDK design, but this allows us to make optimizations in the future. After #7427, improvements to exemplar reservoir locking will greatly improve the ExemplarEnabled benchmarks.
Parallel benchmarks:
Single-threaded benchmarks: