-
Notifications
You must be signed in to change notification settings - Fork 843
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use low precision Clock#now when computing timestamp for exemplars #6417
Use low precision Clock#now when computing timestamp for exemplars #6417
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6417 +/- ##
============================================
- Coverage 91.12% 91.11% -0.01%
- Complexity 5856 5868 +12
============================================
Files 636 636
Lines 17062 17082 +20
Branches 1733 1741 +8
============================================
+ Hits 15548 15565 +17
- Misses 1019 1021 +2
- Partials 495 496 +1 ☔ View full report in Codecov by Sentry. |
sdk/common/src/main/java/io/opentelemetry/sdk/common/Clock.java
Outdated
Show resolved
Hide resolved
sdk/common/src/main/java/io/opentelemetry/sdk/common/Clock.java
Outdated
Show resolved
Hide resolved
@@ -68,8 +68,8 @@ synchronized void recordDoubleMeasurement(double value, Attributes attributes, C | |||
|
|||
private void offerMeasurement(Attributes attributes, Context context) { | |||
this.attributes = attributes; | |||
// Note: It may make sense in the future to attempt to pull this from an active span. | |||
this.recordTime = clock.now(); | |||
// High precision time is not worth the additional performance expense it incurs for exemplars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
what JVM version were you using to benchmark with? (not that it matters much..but might be nice to include in the PR description for posterity). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Updated! |
Resolves #6403.
Introduces a new
Clock#now(boolean highPrecision)
overload, which uses a performance optimized millisecond precision whenhighPrecision=false
. UpdatesReservoirCell
to obtain measurement times usingClock#now(false)
, which shaves a good chunk of time off the time to record each exemplar (~50ns on my machine).This has a greater impact the more frequent measurements are candidates to be stored in ReservoirCell. The savings are especially impactful for the histogram exemplar reservoir, where the algorithm requires us to take the last measurement for each bucket. This means we compute the time for every measurement recorded.
Here's a benchmark output comparing the high precision and low precision
Clock#now
implementations. Benchmark run usingOpenJDK Runtime Environment Temurin-17.0.3+7
.