Conversation
…tific notation Very small sampling rates (e.g. 0.0000001) were formatted using toPrecision(6) + toString() which outputs scientific notation like "1e-7". This changes to explicit rounding at the integer level and toFixed(6) formatting to always produce decimal notation with up to 6 decimal digits, trailing zeros stripped (e.g. "0.000001"). Fixes APMAPI-1869 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
bm1549
added
the
AI Generated
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7846 +/- ##
==========================================
- Coverage 80.45% 80.45% -0.01%
==========================================
Files 749 749
Lines 32479 32482 +3
==========================================
+ Hits 26130 26132 +2
- Misses 6349 6350 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
Overall package sizeSelf size: 5.04 MB Dependency sizes| name | version | self size | total size | |------|---------|-----------|------------| | import-in-the-middle | 3.0.0 | 81.15 kB | 815.98 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB |🤖 This report was automatically generated by heaviest-objects-in-the-universe |
|
✅ Tests 🎉 All green!❄️ No new flaky tests detected 🎯 Code Coverage (details) 🔗 Commit SHA: ec8b672 | Docs | Datadog PR Page | Was this helpful? React with 👍/👎 or give us feedback! |
BenchmarksBenchmark execution time: 2026-03-24 22:19:03 Comparing candidate commit ec8b672 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 226 metrics, 34 unstable metrics. |
2 tasks
BridgeAR
reviewed
Mar 24, 2026
Member
BridgeAR
left a comment
BridgeAR
left a comment
There was a problem hiding this comment.
I guess it is good to be defensive. I think we could just make this simpler and or faster :)
| */ | ||
| function formatKnuthRate (rate) { | ||
| return Number(rate.toPrecision(6)).toString() | ||
| return (Math.round(rate * 1e6) / 1e6).toFixed(6).replace(/\.?0+$/, '') |
Member
There was a problem hiding this comment.
Do we need to manually round? toFixed should already do that, if I am not mistaken :)
I am also uncertain if rate is guaranteed to be a number or not. Leaning on the safe side, I guess calling Number() first is good (the type here is likely ignored in many cases).
Suggested change
| return (Math.round(rate * 1e6) / 1e6).toFixed(6).replace(/\.?0+$/, '') | |
| return Number(rate).toFixed(6).replace(/\.?0+$/, '') |
We could also prevent the relative expensive replace call by checking the trailing zeroes as micro-optimization.
Suggested change
| return (Math.round(rate * 1e6) / 1e6).toFixed(6).replace(/\.?0+$/, '') | |
| const string = Number(rate).toFixed(6) | |
| for (let i = string.length - 1; i > 0; i--) { | |
| if (string[i] !== '0') { | |
| return string.slice(0, i + 1) | |
| } | |
| } |
… perf Addresses PR review feedback: replaces the regex-based trailing-zero strip with a manual loop to avoid regex overhead on the hot path. Also adds Number() coercion as defensive measure per reviewer suggestion. Math.round pre-rounding is intentionally kept — toFixed(6) has imprecise rounding for sub-precision values in V8 (e.g. 0.0000005.toFixed(6) returns '0.000000' in Node 23). JSDoc updated to explain this. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
BridgeAR
reviewed
Mar 24, 2026
| * Formats a sampling rate as a string with up to 6 significant digits and no trailing zeros. | ||
| * Formats a sampling rate as a string with up to 6 decimal digits and no trailing zeros. | ||
| * Pre-rounds to 6 decimal places before calling toFixed to work around V8's imprecise | ||
| * rounding for sub-precision values (e.g. 0.0000005.toFixed(6) returns '0.000000'). |
Member
There was a problem hiding this comment.
Not blocking, just -0.5:
I do not think we have to worry about that. I would rather have the less precise version that is faster, since this is an edge case that should never happen in the first place and even if it would, it is still fine to handle this as an acceptable rounding result in my opinion.
| function formatKnuthRate (rate) { | ||
| return Number(rate.toPrecision(6)).toString() | ||
| const string = (Math.round(Number(rate) * 1e6) / 1e6).toFixed(6) | ||
| const dot = string.indexOf('.') |
Member
There was a problem hiding this comment.
This is just adding overhead and has no behavioral benefit. Using toFixed will always add a dot and that will be detected in the loop as not being zero. Thus, the loop will do the same amount of work and having the indexOf in addition is just adding CPU cycles.
The loop will also always find something that is not zero, so the return at the end is also not needed.
Drop Math.round pre-rounding — the edge case (0.0000005 rounding down in
V8) is acceptable imprecision. Test updated to use 0.00000051 which rounds
up unambiguously without Math.round.
Drop indexOf('.') — check for '.' directly in the loop instead, which
handles integer rates (0, 1) without the overhead of a separate scan.
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
BridgeAR
reviewed
Mar 24, 2026
Co-authored-by: Ruben Bridgewater <[email protected]>
BridgeAR
previously approved these changes
Mar 24, 2026
- Flip negated conditions to positive checks (unicorn/no-negated-condition) - Remove @returns tag since linter can't statically verify loop always returns (jsdoc/require-returns-check) - Preserve BridgeAR's single-check-per-iteration optimization Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
bm1549
force-pushed
the
brian.marks/fix-ksr-scientific-notation
branch
from
2dfedef to
ec8b672
Compare
gh-worker-dd-mergequeue-cf854d bot
pushed a commit
to DataDog/dd-trace-py
that referenced
this pull request
Mar 25, 2026
…tific notation (#17086) ## Description Fix `_dd.p.ksr` span tag formatting for very small sampling rates. Previously, rates below 0.001 were formatted using Python's `:.6g` format which outputs scientific notation (e.g. `0.000001` → `"1e-06"`). This changes to explicit integer-level rounding and `:.6f` formatting to always produce decimal notation with up to 6 decimal digits, trailing zeros stripped. **Related PRs:** - dd-trace-rb: DataDog/dd-trace-rb#5497 - dd-trace-js: DataDog/dd-trace-js#7846 - system-tests: DataDog/system-tests#6466 Fixes APMAPI-1869 ## Testing - Added parametrized unit tests in `tests/tracer/test_sampler.py::test_ksr_formatting` covering: - Rate 1.0 → `"1"` (trailing zeros stripped) - Rate 0.000001 → `"0.000001"` (6 decimal precision boundary) - Rate 0.0000001 → `"0"` (below precision, rounds to zero) - Rate 0.0000005 → `"0.000001"` (rounds up to one millionth) - Rate 0.5 → `"0.5"` (simple case) - Rate 0.7654321 → `"0.765432"` (truncation at 6 decimal places) - These match the system test cases in DataDog/system-tests#6466 ## Risks None — the formatting change only affects `_dd.p.ksr` string values for very small rates. Values ≥ 0.001 produce identical output to the previous `:.6g` format. ## Additional Notes Uses `math.floor(rate * 1e6 + 0.5) / 1e6` instead of `round(rate * 1e6) / 1e6` to avoid Python's banker's rounding which would round `0.0000005` down to `0` instead of up to `0.000001`. Co-authored-by: brian.marks <[email protected]>
BridgeAR
approved these changes
Mar 25, 2026
dd-octo-sts bot
pushed a commit
that referenced
this pull request
Mar 26, 2026
…tific notation (#7846) * fix(tracing): format _dd.p.ksr with decimal notation instead of scientific notation Very small sampling rates (e.g. 0.0000001) were formatted using toPrecision(6) + toString() which outputs scientific notation like "1e-7". This changes to explicit rounding at the integer level and toFixed(6) formatting to always produce decimal notation with up to 6 decimal digits, trailing zeros stripped (e.g. "0.000001"). Fixes APMAPI-1869 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(tracing): replace regex with loop in formatKnuthRate for hot-path perf Addresses PR review feedback: replaces the regex-based trailing-zero strip with a manual loop to avoid regex overhead on the hot path. Also adds Number() coercion as defensive measure per reviewer suggestion. Math.round pre-rounding is intentionally kept — toFixed(6) has imprecise rounding for sub-precision values in V8 (e.g. 0.0000005.toFixed(6) returns '0.000000' in Node 23). JSDoc updated to explain this. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(tracing): simplify formatKnuthRate loop per review feedback Drop Math.round pre-rounding — the edge case (0.0000005 rounding down in V8) is acceptable imprecision. Test updated to use 0.00000051 which rounds up unambiguously without Math.round. Drop indexOf('.') — check for '.' directly in the loop instead, which handles integer rates (0, 1) without the overhead of a separate scan. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Update packages/dd-trace/src/priority_sampler.js Co-authored-by: Ruben Bridgewater <[email protected]> * fix(tracing): resolve lint errors in formatKnuthRate - Flip negated conditions to positive checks (unicorn/no-negated-condition) - Remove @returns tag since linter can't statically verify loop always returns (jsdoc/require-returns-check) - Preserve BridgeAR's single-check-per-iteration optimization Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: Ruben Bridgewater <[email protected]>
Merged
tlhunter
pushed a commit
that referenced
this pull request
Mar 26, 2026
…tific notation (#7846) * fix(tracing): format _dd.p.ksr with decimal notation instead of scientific notation Very small sampling rates (e.g. 0.0000001) were formatted using toPrecision(6) + toString() which outputs scientific notation like "1e-7". This changes to explicit rounding at the integer level and toFixed(6) formatting to always produce decimal notation with up to 6 decimal digits, trailing zeros stripped (e.g. "0.000001"). Fixes APMAPI-1869 Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> * fix(tracing): replace regex with loop in formatKnuthRate for hot-path perf Addresses PR review feedback: replaces the regex-based trailing-zero strip with a manual loop to avoid regex overhead on the hot path. Also adds Number() coercion as defensive measure per reviewer suggestion. Math.round pre-rounding is intentionally kept — toFixed(6) has imprecise rounding for sub-precision values in V8 (e.g. 0.0000005.toFixed(6) returns '0.000000' in Node 23). JSDoc updated to explain this. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * fix(tracing): simplify formatKnuthRate loop per review feedback Drop Math.round pre-rounding — the edge case (0.0000005 rounding down in V8) is acceptable imprecision. Test updated to use 0.00000051 which rounds up unambiguously without Math.round. Drop indexOf('.') — check for '.' directly in the loop instead, which handles integer rates (0, 1) without the overhead of a separate scan. Co-Authored-By: Claude Sonnet 4.6 <[email protected]> * Update packages/dd-trace/src/priority_sampler.js Co-authored-by: Ruben Bridgewater <[email protected]> * fix(tracing): resolve lint errors in formatKnuthRate - Flip negated conditions to positive checks (unicorn/no-negated-condition) - Remove @returns tag since linter can't statically verify loop always returns (jsdoc/require-returns-check) - Preserve BridgeAR's single-check-per-iteration optimization Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]> --------- Co-authored-by: Claude Opus 4.6 (1M context) <[email protected]> Co-authored-by: Ruben Bridgewater <[email protected]>
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
2 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.
What does this PR do?
Fix
_dd.p.ksrspan tag formatting for very small sampling rates. Rates below 0.0000001 were formatted usingtoPrecision(6)+toString()which outputs scientific notation (e.g.0.0000001→"1e-7").The
formatKnuthRatefunction now usestoFixed(6)for decimal notation, with a loop to strip trailing zeros (replacing a regex for hot-path performance). The loop checks for'.'explicitly to correctly handle integer rates like0and1.Motivation
Fixes APMAPI-1869. System tests expect decimal notation for
_dd.p.ksrvalues (see DataDog/system-tests#6466).Related PRs:
Additional Notes
All 10 priority_sampler tests pass. Added 3 new test cases for small-rate formatting:
"0.000001"(decimal notation)"0"(rounds to zero)"0.000001"(rounds up)Addressed review feedback from @BridgeAR:
Number()coercion as defensive measure'.'inline (noindexOfoverhead)Math.roundpre-rounding per reviewer preference; test value updated to0.00000051(unambiguously above halfway point)