Skip to content

Set known tags by id on TagMap and spans - #12715

Open
dougqh wants to merge 17 commits into
dougqh/tag-registry-otelfrom
dougqh/tag-id-setters
Open

dougqh wants to merge 17 commits into
dougqh/tag-registry-otelfrom
dougqh/tag-id-setters

Conversation

@dougqh

@dougqh dougqh commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

What Does This Do

Adds setting a known tag by its id to TagMap and the span API, on top of the tag registry from #12354 (stacked on it; the diff here is only this PR).

  • TagMap.set(long tagId, value) for Object, CharSequence, and the primitives. The entry is built under the tag's canonical name from KnownTagCodec.nameOf, skipping the canonicalizing name lookup the String setters pay on every new entry. An id that names no known tag is rejected.
  • TagMap.Entry.create(long tagId, value), the public way to build an entry ahead of time (as decorators do for cached entries), at the same layer as AgentSpan.setTag. It has the same contract as the name-keyed create: a null or empty value yields no entry.
  • AgentSpan.setTag(long tagId, value), as default methods that resolve the id to its name and delegate to the String setter, so every AgentSpan implementation stays correct with no changes.
  • DDSpan / DDSpanContext overrides with the same behavior as the String family: TagInterceptor still runs on the tag's name (it is name-keyed), primitives still box only when the tag might be intercepted, and the http.status_code quirk is kept. The difference is that a stored tag is set by id.
  • TagMap hashes entries by tag id. TagMap.Entry holds a 64-bit tag hash, computed once at construction: a known tag's id, or a custom tag's name hash in the low 32 bits. An id's serial sits in bits 63-48, so the two can never collide.
    • tagId() becomes a field read instead of a keyOf lookup.
    • Name lookups (getEntry, getAndRemove) take one keyOf, instead of canonicalizing and then hashing the name separately.
    • Buckets fold the hash from bit 48, so known tags spread by serial. A custom tag's bucket hash is unchanged.
    • BucketGroup keeps its int hashes, so only Entry grows: int to long costs 8 bytes per entry because of alignment. That is an accepted temporary cost until the dense store.
  • First caller: BaseDecorator.onPeerConnection / setPeerPort, which run on every client span, now set peer.hostname, peer.ipv4/ipv6 and peer.port by KnownTags id.

Motivation

Under the OTLP <-> Datadog mapping, an ID identifies one tag exactly, where a name may not. #12713 is making the tag registry direction-aware, and there peer.port becomes two tags (the server's port on outbound spans, the client's on inbound ones) that share a Datadog name. Writers then need to set the specific tag, which is what an id expresses. This PR provides that API, independent of the direction work:

            ┌──► #12713  (registry: span directions)  ──┐
#12354 ─────┤                                           ├──► next: writers resolve to ids by direction
            └──► this PR (set tags by id)  ─────────────┘

It follows the API shape of #11901, but implemented on the current TagMap rather than the dense store, which #11901 is stacked on. The dense store can later swap the implementation behind the same signatures.

Additional Notes

  • Not in this PR: id-keyed reads and Ledger/builder support. Each will arrive with a caller.
  • Map contract unchanged, deliberately: Entry.hashCode() still returns the bucket hash rather than key.hashCode() ^ value.hashCode(), and TagMap still compares by identity. Both were already the case before this PR; changing them is a separate behavior change.
  • Shared names after Scope OpenTelemetry tag names by span direction in the tag registry #12713: an entry set by a per-direction id (for example PEER_PORT_OUTBOUND_ID) hashes by that id, so a lookup by the shared name "peer.port" will not find it. Phase 2 resolves names by direction before reaching TagMap, which keeps TagMap direction-agnostic.
  • Merge order with Scope OpenTelemetry tag names by span direction in the tag registry #12713: once Scope OpenTelemetry tag names by span direction in the tag registry #12713 splits peer.port, KnownTags.PEER_PORT_ID no longer exists, so BaseDecorator.setPeerPort stops compiling until it picks a direction. That is intended: it forces the direction to be chosen explicitly.
  • Open for reviewers — how much of the decorator change to keep:
    1. All of it (as is): a real hot caller for the id API, but it couples this PR to Scope OpenTelemetry tag names by span direction in the tag registry #12713 through PEER_PORT_ID.
    2. Drop only setPeerPort: peer.hostname/ipv4/ipv6 keep the same ids in Scope OpenTelemetry tag names by span direction in the tag registry #12713, so onPeerConnection stays a real caller with no coupling. peer.port, which server decorators also set for the client's port, moves to the direction-aware follow-up.
    3. Drop the decorator change entirely: this PR becomes purely core, but has no production caller yet.
  • Behavior coupling with Scope OpenTelemetry tag names by span direction in the tag registry #12713 (affects option 1): once Scope OpenTelemetry tag names by span direction in the tag registry #12713 splits peer.port into a tag per direction, a tag stored under a per-direction id hashes by that id, but the shared name "peer.port" resolves to no id, so a lookup by name misses it. If setPeerPort switched to PEER_PORT_OUTBOUND_ID, a later getTag("peer.port") would come back empty. Until direction-aware reads land, setPeerPort therefore has to keep setting peer.port by name, which stores it as a custom tag that name lookups still find. So whichever of the two PRs merges second keeps setPeerPort name-keyed; onPeerConnection's peer.hostname/ipv4/ipv6 keep the same ids in Scope OpenTelemetry tag names by span direction in the tag registry #12713 and are unaffected.
  • Spock tests that mock AgentSpan would see the id-keyed call directly, since a mock intercepts the default method, so every call site moved to ids would break its test's expectations. Instead, the decorator specs now mock NameKeyedAgentSpan, a test-only AgentSpan whose id-keyed overloads are final and delegate to the name-keyed ones. A mock can't override a final method, so it only sees setTag(name, value). BaseDecoratorTest.mockSpan() creates one. The specs' expectations are unchanged from master, and moving further call sites to ids needs no test change. (Spock only allows Mock() inside a spec class, so the factory lives on the base spec rather than in a trait. Other modules' 50-odd Mock(AgentSpan) sites can adopt it as they migrate.)
  • Tags.PEER_PORT and friends stay as they are; a @Deprecated pointing at the ids comes once callers have migrated.

Contributor Checklist

  • :internal-api:test (TagMap* incl. TagMapFuzzTest, 384 tests; KnownTags*; new TagMapSetByIdTest, TagMapTagHashTest, TagMapEntryCreateByIdTest), :internal-api:spotlessCheck

  • :dd-trace-core:test (DDSpan* incl. new DDSpanSetTagByIdTest, *taginterceptor*, *otlp*, common.writer.*, common.metrics.*; 1279 tests), :dd-trace-core:spotlessCheck. DDAgentWriterCombinedTest.unixSocketTimeoutKeepsWorkerAliveAndReconnects failed once and passed on rerun here and on the base branch; it's a timing test.

  • New TagMapInsertBenchmark, before vs. after hashing entries by tag id. MacBook, JDK 21, default flags, @Fork(2) triage; 12 tags per op:

    Benchmark before ns/op after ns/op change B/op
    knownByName 130.9 ± 1.8 121.1 ± 3.8 −7.5% 688 → 736
    knownById 100.7 ± 5.5 94.7 ± 1.7 −5.9% 688 → 736
    customByName 82.2 ± 2.7 80.6 ± 2.4 −1.9% (noise) 640 → 736

    The first run caught a regression: knownById was +17%, because the id constructor resolved the name twice. That is fixed in 62f1026. Known tags net only +48 B because spreading them by serial removed a bucket collision.

  • :dd-java-agent:agent-bootstrap:test (*Decorator*, 413 tests), :dd-java-agent:agent-bootstrap:spotlessCheck

Jira ticket

N/A

🤖 Generated with Claude Code

dougqh and others added 6 commits October 1, 2026 14:38
TagMap.set(long tagId, value) builds the entry under the known tag's
canonical name from KnownTagCodec.nameOf, skipping the canonicalizing name
lookup the String setters pay. An id that names no known tag is rejected.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
AgentSpan gains setTag(long tagId, ...) defaults that resolve the id to its
name and delegate, so every implementation stays correct. DDSpan and
DDSpanContext override them with the same behavior as the String family --
interception runs on the tag's name, and the http.status_code quirk is
kept -- but store the tag by id.

Follows the API shape of #11901, implemented on the current TagMap rather
than the dense store, so the dense store can later swap the
implementation behind the same signatures.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
onPeerConnection and setPeerPort run on every client span; they now set
peer.hostname, peer.ipv4/ipv6 and peer.port by KnownTags id. First caller
of the id-keyed setTag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The decorator tests mock AgentSpan, so they observe the id-keyed call
directly rather than the String call the default would delegate to.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh dougqh added type: feature Enhancements and improvements comp: core Tracer core tag: no release notes Changes to exclude from release notes tag: ai generated Largely based on code generated by an AI or LLM labels Oct 1, 2026
dougqh and others added 2 commits October 1, 2026 14:50
TagMap.Entry now holds a 64-bit tag hash, computed once at construction:
a known tag's id, or a custom tag's name hash in the low 32 bits. An id's
serial is in bits 63-48, so the two can never collide. The canonicalizing
keyOf the String constructor already pays yields the id, so tagId() becomes
a field read instead of a lookup, and name lookups (getEntry,
getAndRemove) take one keyOf instead of canonicalizing and then hashing the
name separately.

Buckets fold the hash from bit 48, so known tags spread by serial; a custom
tag's bucket hash is unchanged. BucketGroup keeps its int hashes, and
Entry.hashCode() keeps returning the bucket hash, as before.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@datadog-official

This comment has been minimized.

dougqh and others added 2 commits October 1, 2026 15:04
The id constructor validated the id with one nameOf and named the entry
with another, so every id-keyed insert paid two resolver switches. On
TagMapInsertBenchmark that made knownById 17% slower than before the
tag-hash change; resolving once makes it 6% faster.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Inserts a span's worth of tags by Datadog name, by id, and as custom tags,
single-threaded with -prof gc. Records the before/after of hashing entries
by tag id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
String ip = remoteAddress.getHostAddress();
if (resolved && Config.get().isPeerHostNameEnabled()) {
span.setTag(Tags.PEER_HOSTNAME, hostName(remoteAddress, ip));
span.setTag(KnownTags.PEER_HOSTNAME_ID, hostName(remoteAddress, ip));

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are included because eventually we'll need to use ID to disambiguate some of these concepts for OTLP, and I wanted to illustrate usage.

That said, I'm happy to leave these to another PR if others prefer.

The before/after comparison lives in the PR description.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dd-octo-sts

dd-octo-sts Bot commented Oct 1, 2026 •

Copy link
Copy Markdown
Contributor

🟢 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.85 s 14.69 s [+0.2%; +1.9%] (maybe worse)
startup:insecure-bank:tracing:Agent 13.77 s 13.74 s [-0.5%; +1.0%] (no difference)
startup:petclinic:appsec:Agent 17.22 s 17.10 s [-0.1%; +1.5%] (no difference)
startup:petclinic:iast:Agent 16.36 s 17.16 s [-8.7%; -0.6%] (maybe better)
startup:petclinic:profiling:Agent 16.69 s 16.68 s [-1.0%; +1.0%] (no difference)
startup:petclinic:sca:Agent 17.18 s 16.88 s [+0.8%; +2.8%] (maybe worse)
startup:petclinic:tracing:Agent 16.30 s 16.27 s [-0.6%; +1.0%] (no difference)

Commit: e41d7ecb · 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.

dougqh and others added 2 commits October 1, 2026 15:32
Entry.create is the public way to build an entry ahead of time, at the same
layer as AgentSpan.setTag, so it gets the same id-keyed overloads: same
contract as the name-keyed ones (a null or empty value yields no entry),
without the name lookup.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh marked this pull request as ready for review October 1, 2026 19:37
@dougqh
dougqh requested review from a team as code owners October 1, 2026 19:37
@dougqh
dougqh requested review from bric3 and mcculls and removed request for a team October 1, 2026 19:37

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f38b1f88fe

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread internal-api/src/main/java/datadog/trace/api/TagMap.java Outdated
dougqh and others added 3 commits October 1, 2026 15:43
A Spock mock of AgentSpan intercepts setTag(long, ...) directly, so every
call site moved to KnownTags ids broke its test's expectations even though
the tag set was the same. NameKeyedAgentSpan makes the id-keyed overloads
final and delegates them to the name-keyed ones; a mock cannot override a
final method, so it only ever sees setTag(name, value). BaseDecoratorTest
gains mockSpan(), and the decorator specs use it.

This replaces the per-call-site expectation edits: the specs' expectations
are back to tag names, and moving another call site to ids needs no test
change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
BucketGroup treats a zero hash as a vacant slot. Folding a tag id to its
bucket hash yields zero when the serial equals the id's flag bits:
_dd.djm.enabled has serial 4 and the trace-level bit, 4. Sharing a bucket
group, it then looked empty -- a later insert could overwrite it, and size
and copy skipped it. A folded zero now maps to the nonzero sentinel the
name hash already uses. Found by Codex review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Checks every generated id, and inserts every known tag into one map and a
copy. Both fail without the fix.

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

@datadog-official datadog-official Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bits Code Review: FAIL

The new hash fold maps _dd.djm.enabled to the reserved zero sentinel, allowing bucket collisions to lose the tag or cause lookup failures.

Open Bits AI session

🤖 Bits Code Review · Commit f38b1f8 · @DataDog review to ask questions

Comment thread internal-api/src/main/java/datadog/trace/api/TagMap.java Outdated

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp: core Tracer core tag: ai generated Largely based on code generated by an AI or LLM tag: no release notes Changes to exclude from release notes type: feature Enhancements and improvements

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant