Add _dd.p.ksr propagated tag for Knuth sampling rate #10802
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -646,10 +646,38 @@ public DDSpan setSamplingPriority( | |||||
| int samplingPriority, CharSequence rate, double sampleRate, int samplingMechanism) { | ||||||
| if (context.setSamplingPriority(samplingPriority, samplingMechanism)) { | ||||||
| setMetric(rate, sampleRate); | ||||||
| if (samplingMechanism == SamplingMechanism.AGENT_RATE | ||||||
| || samplingMechanism == SamplingMechanism.LOCAL_USER_RULE | ||||||
| || samplingMechanism == SamplingMechanism.REMOTE_USER_RULE | ||||||
| || samplingMechanism == SamplingMechanism.REMOTE_ADAPTIVE_RULE) { | ||||||
| context.getPropagationTags().updateKnuthSamplingRate(formatKnuthSamplingRate(sampleRate)); | ||||||
| } | ||||||
| } | ||||||
| return this; | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Formats the sampling rate for the _dd.p.ksr propagated tag. Uses up to 6 significant digits | ||||||
| * with no trailing zeros, matching the Go/Python reference implementations (%.6g format). | ||||||
| */ | ||||||
| static String formatKnuthSamplingRate(double rate) { | ||||||
| // Use %.6g format: up to 6 significant digits, no trailing zeros | ||||||
| // This matches Go's strconv.FormatFloat(rate, 'g', 6, 64) and Python's f"{rate:.6g}" | ||||||
| String formatted = String.format("%.6g", rate); | ||||||
|
bm1549 marked this conversation as resolved.
Outdated
Contributor
There was a problem hiding this comment. This should fix the localization issue:
Suggested change
Note you'll also need to add an import for
Contributor
There was a problem hiding this comment. This is a pretty hot place to be using String.format. Profiles already show that PropagationTags is a major source of allocation. That said, I'm not going to block this PR, but I do expect that I'll end up reworking this sooner than later.
Contributor
There was a problem hiding this comment. I'm wondering if we can replace that with a tiny custom formatter that rounds to 6 decimal places and trims trailing zeros? That would avoid Formatter allocations while keeping the output stable and locale-independent.
Contributor
There was a problem hiding this comment. Yes, I think we'll have to. That String.format call is quite expensive in terms of allocation.
Contributor
There was a problem hiding this comment. Apparently, the boxing, var-args, and String were just the tip of the iceberg. After looking into it, format ends up allocating a bunch of streams, etc. For comparison sake, I'll see what repeatedly using a formatter does, but ultimately, I think we'll just have to roll our own lighter solution.
Contributor
Author
There was a problem hiding this comment. @dougqh just handled this in the latest commit I've replaced To be safe, I've also added a JMH benchmark (KnuthSamplingRateFormatBenchmark) comparing all three approaches.
Contributor
There was a problem hiding this comment. I made some suggestions about the benchmark.
Contributor
There was a problem hiding this comment. The new approach is certainly much better, but there's still a significant increase in allocation. Fortunately, I think propagation tags itself can be improved to avoid work most of the time. Concurrently, I'll look into further improvements to PropagationTags to see if the overhead can be deferred. That said, we need to be very careful adding to propagation in general because it is one of the hottest portions of any tracer.
Contributor
Author
There was a problem hiding this comment. Addressed in the latest commit. Added a static cache at the class level Now updateRateFreshTrace shows ~ 0 B/op structurally, not JIT-dependent. Also updated the benchmark per your suggestions: throughput mode, 8 threads, -prof gc. |
||||||
| // Remove trailing zeros after decimal point (avoid forbidden String#replaceAll) | ||||||
| if (formatted.contains(".")) { | ||||||
|
bm1549 marked this conversation as resolved.
Outdated
|
||||||
| int end = formatted.length(); | ||||||
| while (end > 0 && formatted.charAt(end - 1) == '0') { | ||||||
| end--; | ||||||
| } | ||||||
| if (end > 0 && formatted.charAt(end - 1) == '.') { | ||||||
| end--; | ||||||
| } | ||||||
| formatted = formatted.substring(0, end); | ||||||
| } | ||||||
| return formatted; | ||||||
| } | ||||||
|
|
||||||
| @Override | ||||||
| public DDSpan setSpanSamplingPriority(double rate, int limit) { | ||||||
| context.setSpanSamplingPriority(rate, limit); | ||||||
|
|
||||||
Uh oh!
There was an error while loading. Please reload this page.