Skip to content

Set known tags by id on TagMap and spans (otlp - tag registry - phase 2) - #12715

Open
dougqh wants to merge 4 commits into
masterfrom
dougqh/tag-id-setters
Open

dougqh wants to merge 4 commits into
masterfrom
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

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 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
@datadog-official

This comment has been minimized.

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.

@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.74 s 14.57 s [+0.4%; +2.0%] (maybe worse)
startup:insecure-bank:tracing:Agent 13.54 s 13.67 s [-1.9%; -0.1%] (maybe better)
startup:petclinic:appsec:Agent 17.16 s 17.05 s [-0.1%; +1.2%] (no difference)
startup:petclinic:iast:Agent 17.02 s 16.99 s [-0.5%; +0.7%] (no difference)
startup:petclinic:profiling:Agent 16.77 s 16.57 s [+0.2%; +2.2%] (maybe worse)
startup:petclinic:sca:Agent 17.06 s 17.10 s [-0.9%; +0.5%] (no difference)
startup:petclinic:tracing:Agent 16.05 s 15.73 s [-2.4%; +6.4%] (no difference)

Commit: 2d87cde8 · 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
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

@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
@dougqh dougqh changed the title Set known tags by id on TagMap and spans Set known tags by id on TagMap and spans (otlp - tag registry - phase 2) Oct 2, 2026
Comment thread dd-trace-core/src/main/java/datadog/trace/core/DDSpanContext.java Outdated
Comment thread internal-api/src/main/java/datadog/trace/api/TagMap.java Outdated
dougqh added a commit that referenced this pull request Oct 7, 2026
Builds on the registry directions (#12713) and id-keyed setters
(#12715, included here): KnownTagCodec resolves names by direction,
TagMap finds a shared name's tag whichever direction it holds, and
DDSpanContext resolves direction-dependent names by the span's kind.
Squashed from the review history of #12731.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh force-pushed the dougqh/tag-registry-otel branch from c3c341e to e49a183 Compare October 7, 2026 12:50
dougqh added a commit that referenced this pull request Oct 7, 2026
Adds id-keyed setters, getters and removal to TagMap, TagMap.Entry and
spans, so a writer that knows the tag skips the name lookup. Squashed
from the review history of #12715.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh force-pushed the dougqh/tag-id-setters branch from c947ea5 to 55ab351 Compare October 7, 2026 12:50
@dougqh
dougqh force-pushed the dougqh/tag-registry-otel branch from e49a183 to 44e132c Compare October 7, 2026 13:11
@dougqh
dougqh requested a review from a team as a code owner October 7, 2026 13:11
@dougqh
dougqh removed the request for review from a team October 7, 2026 13:11
@dougqh
dougqh requested a review from vandonr October 7, 2026 13:11

@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

Clearing an unknown tag id with a null or empty String throws instead of following the other overloads’ no-op behavior. The alias-splitting candidate was excluded because it already exists at the supplied base commit.

Open Bits AI session

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

Comment thread dd-trace-core/src/main/java/datadog/trace/core/DDSpan.java
dougqh added a commit that referenced this pull request Oct 8, 2026
Adds id-keyed setters, getters and removal to TagMap, TagMap.Entry and
spans, so a writer that knows the tag skips the name lookup. Squashed
from the review history of #12715.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh added a commit that referenced this pull request Oct 8, 2026
Builds on the registry directions (#12713) and id-keyed setters
(#12715, included here): KnownTagCodec resolves names by direction,
TagMap finds a shared name's tag whichever direction it holds, and
DDSpanContext resolves direction-dependent names by the span's kind.
Squashed from the review history of #12731.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh force-pushed the dougqh/tag-id-setters branch from 55ab351 to b569cd2 Compare October 8, 2026 12:56
Base automatically changed from dougqh/tag-registry-otel to master October 8, 2026 21:33

@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

Alias-based service splitting breaks for builder and default tags: db.system becomes db.type before interception, so a split-by-tags=db.system rule no longer matches.

Open Bits AI session

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


/** {@code tagId} is a known id, or 0 for the custom tag {@code customTag}. */
private Entry(long tagId, String customTag, byte type, long prim, Object obj) {
super(tagId != 0 ? KnownTagCodec.nameOf(tagId) : customTag);

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.

P2 Preserve alias-based split-service rules for builder tags

With dd.trace.split-by-tags=db.system, a span built with withTag("db.system", "postgresql") should use postgresql as its service name. Entry construction instead canonicalizes the key to db.type before interception, while splitServiceTags still contains the literal db.system, so service splitting silently stops working. Default-tag maps have the same mismatch; setting the tag after creation still works because interception precedes canonicalization. Normalize configured split-service keys and interception inputs consistently across these paths.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · @DataDog review to ask questions · Open Bits AI session

Adds id-keyed setters, getters and removal to TagMap, TagMap.Entry and
spans, so a writer that knows the tag skips the name lookup. Squashed
from the review history of #12715.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh added a commit that referenced this pull request Oct 9, 2026
Builds on the registry directions (#12713) and id-keyed setters
(#12715, included here): KnownTagCodec resolves names by direction,
TagMap finds a shared name's tag whichever direction it holds, and
DDSpanContext resolves direction-dependent names by the span's kind.
Squashed from the review history of #12731.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh force-pushed the dougqh/tag-id-setters branch from b569cd2 to 3ab5862 Compare October 9, 2026 12:37
dougqh and others added 3 commits October 9, 2026 10:01
span.setTag(0L, "") or a null value reached TagMap.getAndRemove(long),
which rejects unknown ids, so clearing threw IllegalArgumentException
while every id-keyed setter ignores an unknown id. removeTag(long) now
ignores it too.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The long-valued setters prechecked with needsIntercept, but when the
interceptor declined the tag they discarded the box it was given and stored
the primitive. Follow the same precheckIntercept -> setBox shape as the
other primitive setters, so TagMap keeps the box.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
peer.port is declared per direction in the registry (#12713), so it has no
direction-free id: BaseDecorator, shared by client and server decorators,
sets it by name, and the set-by-id tests use an unshared int tag.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh added a commit that referenced this pull request Oct 10, 2026
Builds on the registry directions (#12713) and id-keyed setters
(#12715, included here): KnownTagCodec resolves names by direction,
TagMap finds a shared name's tag whichever direction it holds, and
DDSpanContext resolves direction-dependent names by the span's kind.
Squashed from the review history of #12731.

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

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.

3 participants