Skip to content

Commit 98de087

Browse files
dougqhclaude
andcommitted
Address Codex review on #12056: gate sizing off default, fix record point, lane count, read-through sizing
- Do no harm: resolve a SizingHint only when KnownTagCodec.isActive() (the dense store is live). With dd.trace.dense.tags.enabled off (the default), skip the operationName.toString() + global-table probe entirely -- a hint buys nothing when known tags don't take the dense path. - Record point: move recordDenseSize() from DDSpan.finishAndAddToTrace to DDSpanContext.processTagsAndBaggage (at serialization). That is the single terminal point that runs for BOTH finish modes -- plain finish() and phasedFinish()+publish() (gRPC/Netty/WebFlux) -- and runs AFTER the lazy tag post-processors append serialization-time tags (_dd.integration, host, ...), so the recorded high-water mark is the span's true final footprint. - Read-through sizing: add TagMap.createFromParent(parent, hint) so the LOCAL dense store of a read-through map is sized from the hint too; route it in DDSpanContext. Sizing now fires on the common child / shared-parent path (previously the hint was discarded whenever mergedTracerTags was non-null). - Lane count: FlatHashtable.getOrCreate reports genuine inserts; SizingHintTable bumps the per-lane cardinality count only on a real insert, so a cold-start burst of one operation can't spend the budget on a single name. - Docs: correct the 'monotonic-max' over-claim to best-effort racy max (a concurrent finish can transiently lower size; benign -- only mis-sizes an array). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 292d2ca commit 98de087

7 files changed

Lines changed: 102 additions & 26 deletions

File tree

‎dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@
4040
import datadog.trace.api.EndpointTracker;
4141
import datadog.trace.api.IdGenerationStrategy;
4242
import datadog.trace.api.InstrumenterConfig;
43+
import datadog.trace.api.KnownTagCodec;
4344
import datadog.trace.api.KnownTags;
4445
import datadog.trace.api.Pair;
4546
import datadog.trace.api.SizingHint;
@@ -2200,8 +2201,19 @@ protected static final DDSpanContext buildSpanContext(
22002201
// enriching tags a child doesn't, so pick the lane by whether we have a local parent. A
22012202
// resolved hint sizes the span's TagMap and self-tunes on finish; null (no/unkeyable
22022203
// operation name) falls back to the generic default capacity.
2203-
final boolean entrySpan = !(resolvedParentSpanContext instanceof DDSpanContext);
2204-
final SizingHint sizingHint = SizingHintTable.hintFor(operationName, entrySpan);
2204+
//
2205+
// Gated on KnownTagCodec.isActive(): sizing only helps when the dense known-tag store is live
2206+
// (the experimental dd.trace.dense.tags.enabled path). With it off -- the default -- known
2207+
// tags don't take the dense path, so a hint buys nothing; skipping resolution here keeps the
2208+
// operationName.toString() and the global-table probe off the default span-creation path
2209+
// entirely (do no harm).
2210+
final SizingHint sizingHint;
2211+
if (KnownTagCodec.isActive()) {
2212+
final boolean entrySpan = !(resolvedParentSpanContext instanceof DDSpanContext);
2213+
sizingHint = SizingHintTable.hintFor(operationName, entrySpan);
2214+
} else {
2215+
sizingHint = null;
2216+
}
22052217

22062218
// some attributes are inherited from the parent
22072219
context =

‎dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,7 +157,6 @@ public boolean isFinished() {
157157
private void finishAndAddToTrace(final long durationNano) {
158158
// ensure a min duration of 1
159159
if (DURATION_NANO_UPDATER.compareAndSet(this, 0, Math.max(1, durationNano))) {
160-
context.recordDenseSize();
161160
setLongRunningVersion(-this.longRunningVersion);
162161
SpanWrapper wrapper = getWrapper();
163162
if (wrapper != null) {

‎dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -422,7 +422,7 @@ public DDSpanContext(
422422
final int capacity = Math.max((tagsSize <= 0 ? 3 : (tagsSize + 1)) * 4 / 3, 8);
423423
this.unsafeTags =
424424
readThroughParent != null
425-
? TagMap.createFromParent(readThroughParent)
425+
? TagMap.createFromParent(readThroughParent, sizingHint)
426426
: sizingHint != null ? TagMap.create(sizingHint) : TagMap.create(capacity);
427427
this.sizingHint = sizingHint;
428428

@@ -976,15 +976,21 @@ public void setTag(final String tag, final String value) {
976976
}
977977

978978
/**
979-
* Feeds this span's final dense-store size back into the sizingHint it was created from (if any).
980-
* Called once at span finish; lets a reused hint self-tune to the operation's observed known-tag
981-
* high-water mark, so subsequent spans of the operation are sized correctly.
979+
* Feeds this span's final dense-store size back into the sizingHint it was created from (if any),
980+
* so a reused hint self-tunes to the operation's observed known-tag high-water mark and later
981+
* spans of the operation are sized correctly.
982+
*
983+
* <p>Called from {@link #processTagsAndBaggage} at serialization — the one terminal point that
984+
* (a) runs for every serialized span regardless of finish mode (plain {@code finish()} AND {@code
985+
* phasedFinish()}+{@code publish()}, which the async gRPC/Netty/WebFlux paths use and which never
986+
* enter {@code finishAndAddToTrace}), and (b) runs AFTER the lazy tag post-processors have
987+
* appended their serialization-time tags ({@code _dd.integration}, host, ...), so the recorded
988+
* count is the span's true final footprint rather than an underestimate. The caller already holds
989+
* {@code synchronized (unsafeTags)}, so the {@code knownCount} read is consistent; the hint write
990+
* itself is best-effort racy by design (see {@link TagMap#recordSize}).
982991
*/
983992
void recordDenseSize() {
984993
if (sizingHint != null) {
985-
// Intentionally unsynchronized: recordSize is benign-racy by design (monotonic-max on a plain
986-
// int), and a stale knownCount read only ever costs one extra growth on a later span. The
987-
// lock would add nothing but contention on the finish path.
988994
unsafeTags.recordSize(sizingHint);
989995
}
990996
}
@@ -1316,6 +1322,10 @@ void processTagsAndBaggage(
13161322
// Tags
13171323
TagsPostProcessorFactory.lazyProcessor().processTags(unsafeTags, this, restrictedSpan);
13181324

1325+
// Self-tune the per-operation sizing hint now that every tag (incl. the just-appended
1326+
// serialization-time tags) is present -- the terminal point shared by all finish modes.
1327+
recordDenseSize();
1328+
13191329
// Links
13201330
if (injectLinksAsTags) {
13211331
String linksTag = DDSpanLink.toTag(restrictedSpan.getLinks());

‎internal-api/src/main/java/datadog/trace/api/SizingHint.java‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -26,14 +26,17 @@ public final class SizingHint {
2626
final String label;
2727
final int labelHash;
2828

29-
// Payload: the tuned dense-store size. Plain racy int, updated monotonic-max — a stale/lost read
30-
// only mis-sizes an array (over/under-provision), never corrupts tag data, so no synchronization.
29+
// Payload: the tuned dense-store size. Plain racy int, updated best-effort max (see
30+
// TagMap#recordSize -- not strictly monotonic under concurrent finishes) — a stale/lost/lowered
31+
// read only mis-sizes an array (over/under-provision), never corrupts tag data, so no
32+
// synchronization.
3133
int size;
3234

3335
// When true, {@code size} is fixed and recordSize won't grow it. For the shared default /
3436
// overflow
35-
// hint (a HETEROGENEOUS catch-all for operation-less / over-budget spans): self-tuning it via
36-
// monotonic-max would converge to the max across unlike sharers and over-provision the lean ones.
37+
// hint (a HETEROGENEOUS catch-all for operation-less / over-budget spans): self-tuning it toward
38+
// the observed max would converge to the max across unlike sharers and over-provision the lean
39+
// ones.
3740
final boolean capped;
3841

3942
SizingHint(String label, int labelHash, int seedSize) {

‎internal-api/src/main/java/datadog/trace/api/SizingHintTable.java‎

Lines changed: 18 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@
1717
* <p><b>Bounded + racy by design.</b> Each lane is a fixed-capacity {@link FlatHashtable} (never
1818
* resized) so memory is bounded even under unbounded/dynamic operation names; once a lane's
1919
* cardinality budget is spent, further operations share a capped default hint. Construction,
20-
* insertion, and the monotonic-max size update are all lock-free and deliberately racy — a lost
20+
* insertion, and the best-effort-max size update are all lock-free and deliberately racy — a lost
2121
* update or a double-mint only mis-sizes an array (over/under-provision) for a span or two, never
2222
* corrupts tag data (see {@link FlatHashtable} and {@link SizingHint} for the rationale).
2323
*
@@ -30,7 +30,7 @@
3030
public final class SizingHintTable {
3131
private SizingHintTable() {}
3232

33-
// Seed for a fresh per-operation hint: a floor that self-tunes up via monotonic-max recordSize.
33+
// Seed for a fresh per-operation hint: a floor that self-tunes up via best-effort-max recordSize.
3434
static final int SEED_SIZE = 1;
3535
// Fixed size for the shared over-budget hint: a small lean default. Capped (never grown by
3636
// recordSize) because it's a heterogeneous catch-all -- growing it would over-provision lean
@@ -84,15 +84,24 @@ public static SizingHint hintFor(CharSequence operationName, boolean entrySpan)
8484
if ((entrySpan ? entrySize : childSize) >= CARDINALITY_LIMIT) {
8585
return overflow;
8686
}
87-
final SizingHint created = FlatHashtable.getOrCreate(slots, key, HELPER);
88-
if (created == null) {
87+
// Count only a genuine insert. Between the get() miss above and this getOrCreate another thread
88+
// may have inserted the same operation's hint; getOrCreate then returns that existing entry to
89+
// us -- without `created` we'd still bump the lane count, so a cold-start burst of many spans
90+
// of
91+
// one operation could spend the budget on a single distinct name. `created[0]` is set only on
92+
// the actual store branch. Allocated here on the create path only, which is warmup-rare.
93+
final boolean[] created = new boolean[1];
94+
final SizingHint hint = FlatHashtable.getOrCreate(slots, key, HELPER, created);
95+
if (hint == null) {
8996
return overflow; // physically full -- shouldn't happen under the cap, but stay safe
9097
}
91-
if (entrySpan) {
92-
entrySize++; // racy approximate count
93-
} else {
94-
childSize++;
98+
if (created[0]) {
99+
if (entrySpan) {
100+
entrySize++; // racy approximate count; only a real insert bumps it
101+
} else {
102+
childSize++;
103+
}
95104
}
96-
return created;
105+
return hint;
97106
}
98107
}

‎internal-api/src/main/java/datadog/trace/api/TagMap.java‎

Lines changed: 29 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,19 @@ public static final TagMap create(SizingHint hint) {
9898
* local miss/removal for no benefit.
9999
*/
100100
public static final TagMap createFromParent(TagMap parent) {
101+
return createFromParent(parent, null);
102+
}
103+
104+
/**
105+
* Read-through variant of {@link #create(SizingHint)}: reads through to {@code parent} on local
106+
* misses AND sizes the LOCAL dense store from {@code hint}. The two are orthogonal — the parent
107+
* supplies inherited reads, while locally-set known tags live in this map's own dense array,
108+
* whose initial capacity the hint tunes. So the common child / shared-parent path (a non-null
109+
* read-through parent) still gets per-operation sizing; without this the local store would fall
110+
* back to the fixed default. A {@code null} hint behaves exactly like {@link
111+
* #createFromParent(TagMap)}.
112+
*/
113+
public static final TagMap createFromParent(TagMap parent, SizingHint hint) {
101114
if (parent != null) {
102115
if (!parent.frozen) {
103116
throw new IllegalStateException("read-through parent must be frozen");
@@ -106,7 +119,11 @@ public static final TagMap createFromParent(TagMap parent) {
106119
parent = null;
107120
}
108121
}
109-
return new TagMap(parent);
122+
TagMap tagMap = new TagMap(parent);
123+
if (hint != null && hint.size > 0) {
124+
tagMap.denseCapHint = hint.size;
125+
}
126+
return tagMap;
110127
}
111128

112129
/** Creates a new TagMap.Ledger */
@@ -1438,13 +1455,22 @@ private void ensureKnownCapacity() {
14381455

14391456
/**
14401457
* Feeds this map's final dense-entry count back to {@code hint} (call at a terminal point: freeze
1441-
* / span-finish). The hint self-tunes so future maps of the same operation size correctly.
1458+
* / serialization). The hint self-tunes so future maps of the same operation size correctly.
1459+
*
1460+
* <p>Best-effort max, not a strict monotonic-max: the read-compare-write is unsynchronized, so
1461+
* two spans of the same operation finishing concurrently can both read the same old {@code size}
1462+
* and the later write can lower it (a 10-tag span then a 5-tag span settles at 5). And {@code
1463+
* size} is a plain field, so a fresh update isn't promptly visible to span-creation threads. Both
1464+
* are deliberately tolerated: a stale or lowered hint only mis-sizes a later span's dense array
1465+
* (one extra grow-copy), never corrupts tag data — so it stays lock- and atomic-free, and the
1466+
* next heavy span of the operation nudges it back up. A CAS/atomic-max would buy exactness at the
1467+
* cost of contention on this terminal path, which isn't worth it for a sizing hint.
14421468
*/
14431469
public void recordSize(SizingHint hint) {
14441470
// Capped hints (the heterogeneous shared default) are fixed -- don't let unlike sharers grow
14451471
// them.
14461472
if (!hint.capped && this.knownCount > hint.size) {
1447-
hint.size = this.knownCount; // monotonic-max; benign racy plain-int write
1473+
hint.size = this.knownCount; // best-effort racy max (see javadoc); benign plain-int write
14481474
}
14491475
}
14501476

‎internal-api/src/main/java/datadog/trace/util/FlatHashtable.java‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -130,6 +130,20 @@ public static <K, V> V get(V[] table, K key, Helper<K, V> helper) {
130130
* double-create is acceptable only when the payload makes it benign (see class doc).
131131
*/
132132
public static <K, V> V getOrCreate(V[] table, K key, Helper<K, V> helper) {
133+
return getOrCreate(table, key, helper, null);
134+
}
135+
136+
/**
137+
* Like {@link #getOrCreate(Object[], Object, Helper)}, but reports whether this call actually
138+
* minted-and-stored a new entry: when {@code createdOut} is non-null, {@code createdOut[0]} is
139+
* set to {@code true} only on the store branch, and left untouched when an existing entry
140+
* matched. Callers that keep an approximate live-size counter (for a cardinality budget)
141+
* increment it only when {@code createdOut[0]} — so racing callers that get an already-inserted
142+
* entry back don't each over-count the same operation. Still racy by design: a benign double-mint
143+
* (two threads store into the same slot) reports {@code true} on both, an over-count of at most
144+
* the store-race width, not of every caller (see class doc).
145+
*/
146+
public static <K, V> V getOrCreate(V[] table, K key, Helper<K, V> helper, boolean[] createdOut) {
133147
final int mask = table.length - 1;
134148
final int start = helper.hash(key) & mask;
135149
int i = start;
@@ -138,6 +152,9 @@ public static <K, V> V getOrCreate(V[] table, K key, Helper<K, V> helper) {
138152
if (e == null) {
139153
final V created = helper.create(key);
140154
table[i] = created; // single-reference publish; benign clobber (see class doc)
155+
if (createdOut != null) {
156+
createdOut[0] = true;
157+
}
141158
return created;
142159
}
143160
if (helper.matches(key, e)) {

0 commit comments

Comments
 (0)