Make the TagMap Entry pathway null-tolerant by dougqh · Pull Request #11963 · DataDog/dd-trace-java

dougqh · 2026-07-15T14:57:14Z

What Does This Do

Makes the TagMap Entry pathway null-tolerant, so a null/empty value flows through as "no tag" without any caller guarding:

Entry.create(Object) / Entry.create(CharSequence) return null for a null or empty value (@Nullable). create(Object) now applies the empty-CharSequence check by runtime type, so an empty String passed as Object skips the same as via the CharSequence overload — behavior no longer depends on the static type at the call site.
The Entry sinks — getAndSet(Entry) and set(EntryReader) — treat a null Entry as a no-op (@Nullable params). Two guards because set(EntryReader) derefs .entry() before delegating.
The strict (key, value) setters keep their contract: their values (set(String, Object) / set(String, CharSequence)) are @Nonnull.
Keys stay strict everywhere they're written: all tag keys on the write/create surface (put / set / getAndSet / create) are @Nonnull — a null key is a bug, not "no tag". So null-tolerance is scoped to values and Entries on the Entry pathway; keys and strict-setter values are non-null. The annotations make the whole split self-describing.

Motivation

Make handling easier - and avoid NPEs

The Entry pathway is the prebuilt/producer path (create(...) → set(Entry)); a null Entry is the designed "nothing to add" value, mirroring DDSpanContext.setTag's null/empty ⇒ absent convention. Centralizing the tolerance in the primitive means callers needn't each remember a guard — which they don't: this fixes a latent NPE by construction in RemoteHostnameAdder, which sets create(TRACER_HOST, hostname) guarding only null (not empty), so an empty hostname previously NPE'd on set(null).

Notes

getAndSet's null check is the first statement, ahead of checkWriteAccess() and any field read, so it dead-code-eliminates when inlined at a provably-non-null call site (e.g. set(String,value) → getAndSet(newAnyEntry(...))) and stays only where the arg can actually be null. A null Entry is thus a complete no-op and does not assert write access.
Scope: primitive + test only. Caller-side guard cleanup — dropping the now-redundant guards (incl. InternalTagAdder: defensively guard against null prebuilt tagmap entry #11958) and simplifying InternalTagsAdder — is a fast follow-up.
The @Nonnull keys cover the write/create surface; read/lookup keys (getString, remove, getXxxOrDefault) are intentionally left unannotated here — same "null key = bug" logic, but a broader, Map-contract-adjacent sweep worth doing on its own.

Test

TagMapNullToleranceTest pins the contract: create(...) → null for null/empty (incl. empty-as-Object); set/getAndSet null no-op; end-to-end create()→set() with null/empty leaves no tag. Existing TagMapTest / TagMapEntryTest green (no behavior regression).

tag: ai generated
tag: no release note

🤖 Generated with Claude Code

@nonnull

create(Object)/create(CharSequence) may return null for a null or empty value, and the Entry sinks -- getAndSet(Entry) / set(EntryReader) -- treat a null Entry as a no-op, so a null/empty value flows through the Entry pathway as "no tag" without any caller guarding. create(Object) now applies the empty-CharSequence check by runtime type, so the null/empty => absent convention holds regardless of the static type at the call site. The strict (key,value) setters keep their contract -- their values are now @nonnull -- so null tolerance is scoped to the Entry pathway. The annotations make the split self-describing. Fixes a latent NPE by construction: RemoteHostnameAdder sets create(TRACER_HOST, hostname) guarding only null, not empty, so an empty hostname previously NPE'd on set(null). Caller-side guard cleanup (incl. the redundant #11958 guard) is left to a follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

datadog-prod-us1-5 · 2026-07-15T15:14:50Z

🎯 Code Coverage (details)
• Patch Coverage: 100.00%
• Overall Coverage: 56.74% (-0.45%)

_{This comment will be updated automatically if new data arrives.

🔗 Commit SHA: c451336 | Docs | Datadog PR Page | Give us feedback!}

dd-octo-sts · 2026-07-15T15:21:03Z

🟢 Java Benchmark SLOs — All performance SLOs passed

Suite	Status
Startup	🟢 pass

SLO thresholds are defined here based on automatically generated metrics. A warning is raised when results are within 5% of the threshold.

PR vs. master results

Scenario	Candidate	master	Δ (95% CI of mean)
startup:insecure-bank:iast:Agent	14.01 s	13.98 s	[-0.5%; +0.9%] (no difference)
startup:insecure-bank:tracing:Agent	12.85 s	13.06 s	[-2.5%; -0.7%] (maybe better)
startup:petclinic:appsec:Agent	16.88 s	16.97 s	[-1.4%; +0.4%] (no difference)
startup:petclinic:iast:Agent	16.39 s	16.95 s	[-7.6%; +1.0%] (no difference)
startup:petclinic:profiling:Agent	16.77 s	16.61 s	[-0.3%; +2.2%] (no difference)
startup:petclinic:sca:Agent	17.06 s	16.88 s	[+0.0%; +2.1%] (maybe worse)
startup:petclinic:tracing:Agent	16.13 s	16.14 s	[-1.3%; +1.2%] (no difference)

Commit:c451336b · CI Pipeline · Benchmarking Platform UI

Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion.

@nonnull

A tag has no valid null key, so put/set/getAndSet/create now take @nonnull tag. This completes the null contract alongside the value/Entry side: keys are strict (null = a bug), values/Entries on the Entry pathway are tolerant (null = no tag). Scoped to the write/create surface; read/lookup keys (getString, remove, getXxxOrDefault) are left for a possible follow-up. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

datadog-prod-us1-5

More details

The null guard in getAndSet(Entry) is placed before checkWriteAccess(), which means a null entry on a frozen map silently returns null instead of throwing an AccessException — this is intentional and documented. All new and existing tests pass (4 + 181 + 112 = 297 tests); adversarial coverage confirmed the empty-StringBuilder-as-Object path, frozen-map no-op contract, and no-existing-tag-removal invariant all hold correctly.

Was this helpful? React 👍 or 👎

_{📊 Validated against 20 scenarios · Open Bits AI session}

_{🤖 Datadog Autotest · Commit fc7bc19 · What is Autotest? · Any feedback? Reach out in #autotest}

@nonnull

The interface -> final-class fold removed the TagMap interface decls, which carried the @Nullable/@nonnull param annotations from #11963. Re-home them onto the now-concrete methods: @nonnull tag keys and strict-setter values, @nullable on the set(EntryReader)/getAndSet(Entry) sinks (+ the getAndSet contract javadoc). The null-tolerance behavior was already preserved by the fold; this restores the self-describing contract on the write surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

A fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

dd-octo-sts · 2026-07-15T19:23:15Z

/merge

gh-worker-devflow-routing-ef8351 · 2026-07-15T19:23:19Z

View all feedbacks in Devflow UI.

2026-07-15 19:23:19 UTC ℹ️ Start processing command /merge

2026-07-15 19:23:24 UTC ℹ️ MergeQueue: pull request added to the queue

The expected merge time in master is approximately 2h (p90).

2026-07-15 20:25:12 UTC ℹ️ MergeQueue: This merge request was merged

@nonnull

The interface -> final-class fold removed the TagMap interface decls, which carried the @Nullable/@nonnull param annotations from #11963. Re-home them onto the now-concrete methods: @nonnull tag keys and strict-setter values, @nullable on the set(EntryReader)/getAndSet(Entry) sinks (+ the getAndSet contract javadoc). The null-tolerance behavior was already preserved by the fold; this restores the self-describing contract on the write surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

A fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@nonnull

Fold OptimizedTagMap into a final class TagMap (drop the interface) TagMap was an interface with a single implementation, OptimizedTagMap. The split was vestigial scaffolding from when a second (HashMap-backed) impl existed; with one impl it is false generalization. Collapse them into one `public final class TagMap`: - The interface's abstract method declarations are removed; OptimizedTagMap's bodies become TagMap's methods. - Nested types that were implicitly `public static` in the interface (EntryChange, EntryRemoval, EntryReader, Entry, Ledger) are now written out explicitly as `public static`. - Static factories (create/fromMap/ledger/...) and the EMPTY constant become explicit `public static` members; the EmptyHolder lazy-init note is updated now that there is no interface<->impl class-init cycle. - putAll(TagMap) loses its `instanceof` dispatch (always true once there is one class) and calls the fast path directly. No behavior change; motivation is code simplicity, not performance (a single final class is monomorphic by construction, but CHA already devirtualized the sole impl). Public API is preserved, so callers are unchanged; the 3 tests that referenced OptimizedTagMap now reference TagMap. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Drop EmptyHolder, make factories final, simplify test-helper instanceof Post-fold tidy, all TagMap-scoped: - Remove EmptyHolder: with one class there is no interface<->impl class-init cycle to break, and the private constructor reads no statics, so EMPTY is a direct `new TagMap(new Object[1], 0)` initializer. - Static factories (create/fromMap/ledger/...) are now `public static final` (not expressible on the old interface). - assertSize/assertNotEmpty/assertEmpty/checkIntegrity test helpers dropped their now-always-true `instanceof TagMap` guard + redundant cast. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Restore null-contract annotations lost when folding away the interface The interface -> final-class fold removed the TagMap interface decls, which carried the @Nullable/@nonnull param annotations from #11963. Re-home them onto the now-concrete methods: @nonnull tag keys and strict-setter values, @nullable on the set(EntryReader)/getAndSet(Entry) sinks (+ the getAndSet contract javadoc). The null-tolerance behavior was already preserved by the fold; this restores the self-describing contract on the write surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Mark fromMap/fromMapImmutable map param @nonnull Per review: these factories NPE on a null map (map.size()/putAll), so the input is non-null by contract. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

A fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

@VisibleForTesting

…hase 1a) (#11789) Add TagMap read-through support (level-split phase 1 mechanism) A fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Mark parent field @VisibleForTesting and size the tombstone HashSet small Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Collapse redundant isDefinitelyEmpty() call in isEmpty() A live read-through parent is never definitely-empty: createFromParent drops an empty parent, copy() only forwards an already-vetted parent, and the parent is frozen so it can't become empty. So the size==0 / no-tombstones branch of isEmpty() always returned false via the parent chain walk — collapse it to a documented `return false`. Addresses mcculls' review comment on #11789. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

@VisibleForTesting

…plit) (phase 1a) (#11932) Add TagMap read-through support (level-split phase 1 mechanism) A fresh, mutable TagMap can read through to a frozen parent on local misses, so a span can layer its own tags over a shared, immutable set (e.g. merged tracer tags) without copying them. - createFromParent(parent): the only way to attach a parent; the parent must be frozen and is fixed at construction (no re-parenting), so read-through can treat it as stable. Single-parent by design in phase 1. - Reads resolve local-first, then the parent; a local entry shadows the parent's (local-wins). Removing a parent key locally records a lazy tombstone (removedFromParent) so it stops reading through; the tombstone set is null until first needed, keeping the hot paths untouched. - size()/isEmpty() are exact (Map contract) and resolve the parent; isDefinitelyEmpty()/estimateSize() are the cheap conservative variants for the hot path. copy() preserves the parent and tombstones; forEach walks local then parent. Built on the folded final-class TagMap (#11967); composes cleanly with the null-tolerant Entry pathway (#11963). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Wire mergedTracerTags as a read-through parent at span build (level-split phase 1) Attach the trace's merged tracer tags to each span's TagMap as a frozen read-through parent (via TagMap.createFromParent) at span construction, instead of copying them into every span. The span sees the shared tags on read and only stores its own local tags, so the common trace-level bundle is held once per trace rather than duplicated per span. - CoreTracer builds the frozen merged-tracer-tags parent once; config version is kept out of that bundle. - DDSpanContext attaches the parent at construction (fixed, no re-parenting). - Adds TagMapReadThroughBenchmark (copy-down vs read-through, -prof gc). Stacked on the read-through mechanism (#11789), which builds on the folded final-class TagMap (#11967). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Mark parent field @VisibleForTesting and size the tombstone HashSet small Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Collapse redundant isDefinitelyEmpty() call in isEmpty() A live read-through parent is never definitely-empty: createFromParent drops an empty parent, copy() only forwards an already-vetted parent, and the parent is frozen so it can't become empty. So the size==0 / no-tombstones branch of isEmpty() always returned false via the parent chain walk — collapse it to a documented `return false`. Addresses mcculls' review comment on #11789. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Merge branch 'dougqh/tagmap-read-through' into dougqh/tagmap-read-through-consumer Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'dougqh/tagmap-read-through' into dougqh/tagmap-read-through-consumer Merge branch 'master' into dougqh/tagmap-read-through Merge branch 'dougqh/tagmap-read-through' into dougqh/tagmap-read-through-consumer Make TagMap fillMap/fillStringMap read-through aware Both bulk-fill helpers walked only this.buckets, so a parent-backed TagMap materialized through them dropped tags visible only via the parent chain and ignored shadowing/tombstones -- unlike putAll, forEach, and iteration, which already honor the read-through union. Branch to the visible-union walk (forEach) when a parent is attached, mirroring putAllOptimizedMap; keep the untouched local-only loop for the common no-parent case so it pays zero overhead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Drop observationally-empty read-through parents (exact isEmpty) createFromParent used isDefinitelyEmpty(), which only checks each level's local size and ignores shadowing/tombstones. With multi-level read-through, a frozen intermediate can be observationally empty (no local entries, every inherited key tombstoned) yet report isDefinitelyEmpty() == false because a farther ancestor still holds entries. Attaching such a parent then let a child take isEmpty()'s no-tombstone fast path and return false while size(), lookup, and iteration all report empty -- a Map-contract violation. Use exact isEmpty() so semantically empty parents are dropped, which also restores the invariant isEmpty()'s fast path relies on: an attached parent always contributes at least one visible entry. The hot path is unaffected -- isEmpty() short-circuits on size != 0, only walking tombstones in the rare all-tombstoned case this fix targets. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Merge remote-tracking branch 'origin/master' into dougqh/tagmap-read-through-consumer # Conflicts: # internal-api/src/main/java/datadog/trace/api/TagMap.java # internal-api/src/test/java/datadog/trace/api/TagMapReadThroughTest.java Co-authored-by: devflow.devflow-routing-intake <devflow.devflow-routing-intake@kubernetes.us1.ddbuild.io>

dougqh added comp: core Tracer core tag: no release notes Changes to exclude from release notes type: refactoring tag: ai generated Largely based on code generated by an AI or LLM labels Jul 15, 2026

dougqh marked this pull request as ready for review July 15, 2026 16:40

dougqh requested a review from a team as a code owner July 15, 2026 16:40

dougqh requested a review from ygree July 15, 2026 16:40

dougqhand others added 2 commits July 15, 2026 12:46

Brace single-statement null-check ifs per style convention
c451336
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

datadog-prod-us1-5Bot reviewed Jul 15, 2026
View reviewed changes

mhdatie approved these changes Jul 15, 2026
View reviewed changes

dougqh mentioned this pull request Jul 15, 2026
Fold OptimizedTagMap into a final class TagMap #11967
Merged

dougqh added this pull request to the merge queue Jul 15, 2026

github-merge-queueBot removed this pull request from the merge queue due to failed status checks Jul 15, 2026

gh-worker-dd-mergequeue-cf854dBot merged commit d80a30e into masterJul 15, 2026
586 checks passed

gh-worker-dd-mergequeue-cf854dBot deleted the dougqh/tagmap-null-entry-tolerance branch July 15, 2026 20:25

github-actionsBot added this to the 1.65.0 milestone Jul 15, 2026

Navigation Menu

Uh oh!

Make the TagMap Entry pathway null-tolerant - #11963

Make the TagMap Entry pathway null-tolerant#11963
gh-worker-dd-mergequeue-cf854d[bot] merged 3 commits into
masterfrom
dougqh/tagmap-null-entry-tolerance

dougqh commented Jul 15, 2026•
edited
Loading

Uh oh!

datadog-prod-us1-5Bot commented Jul 15, 2026•
edited by datadog-official Bot
Loading

Uh oh!

dd-octo-stsBot commented Jul 15, 2026•
edited
Loading

Uh oh!

datadog-prod-us1-5Bot left a comment

Uh oh!

dd-octo-stsBot commented Jul 15, 2026

Uh oh!

gh-worker-devflow-routing-ef8351Bot commented Jul 15, 2026•
edited
Loading

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

dougqh commented Jul 15, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What Does This Do

Motivation

Notes

Test

Uh oh!

datadog-prod-us1-5Bot commented Jul 15, 2026• edited by datadog-official BotLoading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dd-octo-stsBot commented Jul 15, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🟢 Java Benchmark SLOs — All performance SLOs passed

Uh oh!

datadog-prod-us1-5Bot left a comment

Choose a reason for hiding this comment

Uh oh!

dd-octo-stsBot commented Jul 15, 2026

Uh oh!

gh-worker-devflow-routing-ef8351Bot commented Jul 15, 2026• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

dougqh commented Jul 15, 2026•
edited
Loading

datadog-prod-us1-5Bot commented Jul 15, 2026•
edited by datadog-official Bot
Loading

dd-octo-stsBot commented Jul 15, 2026•
edited
Loading

gh-worker-devflow-routing-ef8351Bot commented Jul 15, 2026•
edited
Loading