fix: prevent duplicate trace_id after CallTraceHashTable expansion - #659
Merged
Conversation
…ROF-15396) Thread slot_base through LongHashTable generations so trace_id low bits stay unique across expansions instead of restarting from 0; guard against the slot range overflowing 2^32 with a counter for observability.
Contributor
CI Test ResultsRun: #29488003810 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-07-16 09:57:33 UTC |
Contributor
Benchmark Results (commit a6ab878)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124506625 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10287 ms (21 iters) | ✅ 10265 ms (21 iters) | ≈ -0.2% (±11.4%) | — / — |
| akka-uct | 25 | ✅ 8865 ms (24 iters) | ✅ 8823 ms (24 iters) | ≈ -0.5% (±9.4%) | — / — |
| finagle-chirper | 21 | ✅ 5948 ms (33 iters) | ✅ 6028 ms (33 iters) | ≈ +1.3% (±25.7%) | |
| finagle-chirper | 25 | ✅ 5455 ms (36 iters) | ✅ 5496 ms (36 iters) | ≈ +0.8% (±24.1%) | |
| fj-kmeans | 21 | ✅ 2847 ms (66 iters) | ✅ 2764 ms (68 iters) | 🟢 -2.9% | — / — |
| fj-kmeans | 25 | ✅ 2811 ms (66 iters) | ✅ 2804 ms (66 iters) | ≈ -0.2% (±2.6%) | — / — |
| future-genetic | 21 | ✅ 2063 ms (90 iters) | ✅ 2083 ms (89 iters) | ≈ +1% (±2.7%) | — / — |
| future-genetic | 25 | ✅ 2028 ms (91 iters) | ✅ 2101 ms (88 iters) | 🔴 +3.6% | — / — |
| naive-bayes | 21 | ✅ 1273 ms (134 iters) | ✅ 1273 ms (135 iters) | ≈ 0% (±33%) | — / — |
| naive-bayes | 25 | ✅ 1013 ms (169 iters) | ✅ 1013 ms (170 iters) | ≈ 0% (±31.4%) | — / — |
| reactors | 21 | ✅ 15847 ms (15 iters) | ✅ 16518 ms (15 iters) | ≈ +4.2% (±7.9%) | — / — |
| reactors | 25 | ✅ 18435 ms (15 iters) | ✅ 18690 ms (15 iters) | ≈ +1.4% (±4.3%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1947 / 1968 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / ✅ | 2288 / 2290 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 8620 / 8482 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 8154 / 8569 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 3 | 1268 / 1261 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 1302 / 1258 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 1 | 2907 / 2901 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 2 | 2961 / 2852 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 5 | 3511 / 3546 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 7 | 3475 / 3486 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1641 / 1558 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 1886 / 1870 | ✅ / ✅ | ✅ / ✅ |
Contributor
Benchmark Results (commit 9747d1a)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124510382 Commit: ✅ Within expected boundariesNo significant runtime deltas (all within run-to-run noise) and no internal-counter outliers. Runtime details (per benchmark × JDK)
Internal counter details (ddprof)ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
|
…ment Drive expandTableIfNeeded()'s slot-id overflow guard through a real put() call path via a test-only seed seam, and note the skip state is bounded by the next processTraces() rotation.
jbachorik
marked this pull request as ready for review
July 15, 2026 07:54
Contributor
Benchmark Results (commit 6e66143)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124689199 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10374 ms (21 iters) | ✅ 10289 ms (21 iters) | ≈ -0.8% (±11.2%) | — / — |
| akka-uct | 25 | ✅ 8897 ms (24 iters) | ✅ 8853 ms (24 iters) | ≈ -0.5% (±10.7%) | — / — |
| finagle-chirper | 21 | ✅ 5946 ms (33 iters) | ✅ 5918 ms (33 iters) | ≈ -0.5% (±25.7%) | |
| finagle-chirper | 25 | ✅ 5450 ms (36 iters) | ✅ 5485 ms (36 iters) | ≈ +0.6% (±24.4%) | |
| fj-kmeans | 21 | ✅ 2690 ms (70 iters) | ✅ 2752 ms (68 iters) | ≈ +2.3% (±2.8%) | — / — |
| fj-kmeans | 25 | ✅ 2846 ms (66 iters) | ✅ 2752 ms (68 iters) | 🟢 -3.3% | — / — |
| future-genetic | 21 | ✅ 2074 ms (89 iters) | ✅ 2146 ms (86 iters) | 🔴 +3.5% | — / — |
| future-genetic | 25 | ✅ 2042 ms (91 iters) | ✅ 2057 ms (91 iters) | ≈ +0.7% (±2.7%) | — / — |
| naive-bayes | 21 | ✅ 1257 ms (135 iters) | ✅ 1266 ms (135 iters) | ≈ +0.7% (±32.7%) | — / — |
| naive-bayes | 25 | ✅ 1011 ms (169 iters) | ✅ 1011 ms (169 iters) | ≈ 0% (±31.6%) | — / — |
| reactors | 21 | ✅ 17102 ms (15 iters) | ✅ 16692 ms (15 iters) | ≈ -2.4% (±7.6%) | — / — |
| reactors | 25 | ✅ 18787 ms (15 iters) | ✅ 18071 ms (15 iters) | ≈ -3.8% (±5.6%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 2027 / 1956 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 2307 / 2112 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 8895 / 8258 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | ✅ / 1 | 8318 / 8518 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 4 / 2 | 1271 / 1291 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1260 / 1274 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 2 | 2956 / 2989 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2815 / 2888 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 5 / 5 | 3489 / 3492 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 3 / 5 | 3491 / 3497 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 1670 / 1600 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1899 / 1819 | ✅ / ✅ | ✅ / ✅ |
zhengyu123
approved these changes
Jul 15, 2026
zhengyu123
left a comment
zhengyu123
left a comment
Contributor
There was a problem hiding this comment.
Looks good.
Nits:
nextGenerationCapacity() and wouldExceedSlotIdRange() can be private.
…ssor Addresses PR review nit: nextGenerationCapacity()/wouldExceedSlotIdRange() were public solely for direct test access. Route the test through a new CallTraceHashTableOverflowGuardTestAccessor friend, matching the existing CallTraceHashTableTestAccessor convention.
Contributor
Benchmark Results (commit 7735121)Pipeline: https://gitlab.ddbuild.io/DataDog/apm-reliability/benchmarking-platform/-/pipelines/124963941 Commit:
|
| Benchmark | JDK | Latest | Dev | Δ (dev vs latest) | Issues L/D |
|---|---|---|---|---|---|
| akka-uct | 21 | ✅ 10192 ms (21 iters) | ✅ 10333 ms (21 iters) | ≈ +1.4% (±11.4%) | — / — |
| akka-uct | 25 | ✅ 8866 ms (24 iters) | ✅ 8870 ms (24 iters) | ≈ +0% (±9.8%) | — / — |
| finagle-chirper | 21 | ✅ 5965 ms (33 iters) | ✅ 6042 ms (33 iters) | ≈ +1.3% (±25.1%) | |
| finagle-chirper | 25 | ✅ 5502 ms (36 iters) | ✅ 5471 ms (36 iters) | ≈ -0.6% (±24.2%) | |
| fj-kmeans | 21 | ✅ 2663 ms (69 iters) | ✅ 2821 ms (66 iters) | 🔴 +5.9% | — / — |
| fj-kmeans | 25 | ✅ 2841 ms (66 iters) | ✅ 2834 ms (66 iters) | ≈ -0.2% (±2.5%) | — / — |
| future-genetic | 21 | ✅ 2099 ms (88 iters) | ✅ 2114 ms (88 iters) | ≈ +0.7% (±2.8%) | — / — |
| future-genetic | 25 | ✅ 2102 ms (88 iters) | ✅ 2025 ms (92 iters) | 🟢 -3.7% | — / — |
| naive-bayes | 21 | ✅ 1223 ms (139 iters) | ✅ 1248 ms (136 iters) | ≈ +2% (±33.4%) | — / — |
| naive-bayes | 25 | ✅ 1022 ms (168 iters) | ✅ 985 ms (174 iters) | ≈ -3.6% (±30.8%) | — / — |
| reactors | 21 | ✅ 16348 ms (15 iters) | ✅ 16483 ms (15 iters) | ≈ +0.8% (±8.5%) | — / — |
| reactors | 25 | ✅ 18672 ms (15 iters) | ✅ 18638 ms (15 iters) | ≈ -0.2% (±4.2%) | — / — |
Internal counter details (ddprof)
ddprof internal counters, latest / dev (✅ = 0, · = unavailable):
| Benchmark | JDK | Dropped rec | Dropped jvmti | Dropped trace | Skipped WC | AGCT fail | Unwind fail |
|---|---|---|---|---|---|---|---|
| akka-uct | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 1 | 1973 / 1855 | ✅ / ✅ | ✅ / ✅ |
| akka-uct | 25 | ✅ / ✅ | ✅ / ✅ | 1 / 1 | 2102 / 2158 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 21 | ✅ / ✅ | ✅ / ✅ | 3 / 4 | 8665 / 8205 | ✅ / ✅ | ✅ / ✅ |
| finagle-chirper | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 8217 / 8297 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 21 | ✅ / ✅ | ✅ / ✅ | 1 / 3 | 1254 / 1268 | ✅ / ✅ | ✅ / ✅ |
| fj-kmeans | 25 | ✅ / ✅ | ✅ / ✅ | 2 / ✅ | 1270 / 1297 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 21 | ✅ / ✅ | ✅ / ✅ | ✅ / 5 | 2895 / 2962 | ✅ / ✅ | ✅ / ✅ |
| future-genetic | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 2945 / 2833 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 21 | ✅ / ✅ | ✅ / ✅ | 2 / 8 | 3528 / 3502 | ✅ / ✅ | ✅ / ✅ |
| naive-bayes | 25 | ✅ / ✅ | ✅ / ✅ | 7 / 6 | 3475 / 3491 | ✅ / ✅ | ✅ / ✅ |
| reactors | 21 | ✅ / ✅ | ✅ / ✅ | 1 / ✅ | 1709 / 1726 | ✅ / ✅ | ✅ / ✅ |
| reactors | 25 | ✅ / ✅ | ✅ / ✅ | 3 / ✅ | 1926 / 1933 | ✅ / ✅ | ✅ / ✅ |
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?:
CallTraceHashTable::put()could assign the sametrace_idto two different call traces once the table expanded: capacity doubles on expansion but the newLongHashTablegeneration's slot numbering restarted from 0, whileinstance_id(the upper 32 bits oftrace_id) stays fixed until the whole table retires. This PR threads aslot_basethrough eachLongHashTablegeneration so the low 32 bits oftrace_id(slot_base + slot) stay unique across all generations of one active tenure, and adds an overflow guard (with aCALLTRACE_STORAGE_EXPANSION_SKIPPEDcounter) that skips expansion rather than letslot_base + capacitycarry intoinstance_id's bits.Motivation:
An ASan stress test showed ~4% duplicate trace IDs after expansion, causing downstream JFR constant-pool/symbolication consumers to merge or misattribute samples for the colliding traces.
Additional Notes:
The expansion-overflow guard's boundary check and capacity-doubling logic are extracted into two pure, allocation-free static methods (
CallTraceHashTable::wouldExceedSlotIdRange,nextGenerationCapacity) so they can be unit tested directly — reaching the 2^32 boundary via realput()calls would require billions of inserts.How to test the change?:
New/updated gtest cases in
test_callTraceStorage.cpp:ExpansionProducesNoDuplicateTraceIdsTraceIdStableAcrossExpansionTwoExpansionsWithinOneTenureNoDuplicateTraceIdsCallTraceHashTableOverflowGuardTest.RejectsOnlyValuesThatExceedSlotIdRangeCallTraceHashTableOverflowGuardTest.NextGenerationCapacityIsDoubleRun via
:ddprof-lib:gtestDebug_test_callTraceStorage; all 18 cases pass.For Datadog employees:
credentials of any kind, I've requested a security review (run the
dd:platform-security-reviewskill, or file a request via the PSEC review form).
bewairealso runs automatically on every PR.Unsure? Have a question? Request a review!