InternalTagAdder: defensively guard against null prebuilt tagmap entry#11958
Conversation
There was a problem hiding this comment.
More details
PR adds a defensive null check to prevent NullPointerException when DD_SERVICE is empty. The baseServiceEntry field becomes null due to TagMap.Entry.create() returning null for empty CharSequences; without the guard, calling unsafeTags.set(null) would crash. The fix is minimal, targeted, and correct — tested with new case for empty DD_SERVICE configuration.
📊 Validated against 3 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 041bd68 · What is Autotest? · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 041bd68 | Docs | Datadog PR Page | Give us feedback! |
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
|
Related to #11555 |
d3d2e12
into
master
Make the TagMap Entry pathway null-tolerant 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> Mark tag keys @nonnull on the TagMap write/create surface 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> Brace single-statement null-check ifs per style convention 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>
What Does This Do
There is a codepath where, if DD_SERVICE is set empty, the prebuilt tagmap entry for _dd.base_service is null and can break the advice from the span finish callsite
Having an empty service name is not something we should allow. We prevent only null becoming unnamed-java-service but when it's set to the config it can be empty.
This PR implements a defensive fix
Motivation
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]