fix(tracer): use fixed-point format for _dd.p.ksr tag#4603
Conversation
formatKnuthSamplingRate was using 'g' (significant digits) format which produces scientific notation for very small rates (e.g. "1e-06" instead of "0.000001"). Switch to 'f' (fixed-point) format with 6 decimal places to match the cross-language spec enforced by system tests. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
🚀 New features to boost your workflow:
|
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: db7b27f | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-03-26 13:41:37 Comparing candidate commit db7b27f in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 215 metrics, 9 unstable metrics.
|
ddtrace/tracer/sampler.go
Outdated
| s := strconv.FormatFloat(rate, 'f', 6, 64) | ||
| s = strings.TrimRight(s, "0") | ||
| s = strings.TrimRight(s, ".") | ||
| return s |
There was a problem hiding this comment.
I'd recommend benchmarking the old implementation against the new.
There was a problem hiding this comment.
Added a benchmark in b6922ba. Results:
Old ('g' format):
BenchmarkFormatKnuthSamplingRate-10 35274837 34.64 ns/op 5 B/op 0 allocs/op
New ('f' + TrimRight):
BenchmarkFormatKnuthSamplingRate-10 30153468 39.87 ns/op 8 B/op 1 allocs/op
~5ns/op overhead with 1 extra allocation (8 bytes). This function is called once per root span, so the difference should be negligible in practice. Does this look acceptable to you?
There was a problem hiding this comment.
@bm1549 We are battling allocations one by one. I can take care of it, as the improvement is clear, but we need to squeeze that allocation.
There was a problem hiding this comment.
Squeezed it out in db7b27f. Switched to strconv.AppendFloat with a stack-allocated [24]byte buffer and in-place trimming — 0 allocs now:
Old ('g' format):
BenchmarkFormatKnuthSamplingRate-10 35274837 34.64 ns/op 5 B/op 0 allocs/op
New (AppendFloat + trim):
BenchmarkFormatKnuthSamplingRate-10 30811003 39.17 ns/op 5 B/op 0 allocs/op
Same 0 allocs, same 5 B/op. The ~4.5ns difference is just 'f' formatting being slightly more work than 'g'.
Requested by reviewer to compare old ('g') vs new ('f') implementation.
Results show ~5ns/op overhead (35→40 ns/op), 1 extra alloc (8B), which
is negligible since this runs once per root span.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Use strconv.AppendFloat with a stack-allocated [24]byte buffer and in-place trimming instead of FormatFloat + strings.TrimRight, which was creating an extra heap allocation. The new implementation has 0 allocs/op matching the original 'g' format implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
What does this PR do?
Fixes
formatKnuthSamplingRateto use fixed-point ('f') formatting instead of general ('g') formatting. The'g'format uses significant digits, which produces scientific notation for very small rates (e.g."1e-06"instead of"0.000001"). The new implementation usesstrconv.AppendFloatwith a stack-allocated buffer and in-place trimming to avoid heap allocations.Before:
formatKnuthSamplingRate(0.000001)→"1e-06"After:
formatKnuthSamplingRate(0.000001)→"0.000001"Motivation
The
Test_Knuth_Sample_Rate.test_sampling_knuth_sample_rate_trace_sampling_ruleparametric system tests (added in system-tests#6466) enforce precision-boundary cases that fail with the current'g'format:'g'(before)'f'(after)0.000001"1e-06""0.000001""0.000001"0.0000001"1e-07""0""0"0.00000051"5.1e-07""0.000001""0.000001"Benchmark
Same 0 allocs/op, same 5 B/op. ~4.5ns overhead from
'f'formatting.Reviewer's Checklist
make lintlocally.make testlocally.make generatelocally.make fix-moduleslocally.🤖 Generated with Claude Code