Skip to content

Resolve direction-dependent tag names by the span's kind (otlp - tag registry - phase 3) - #12731

Open
dougqh wants to merge 22 commits into
masterfrom
dougqh/tag-registry-direction-resolution
Open

dougqh wants to merge 22 commits into
masterfrom
dougqh/tag-registry-direction-resolution

Conversation

@dougqh

@dougqh dougqh commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

What Does This Do

Phase 3 of the tag registry work: span directions at runtime. It is stacked on #12713 (registry directions) and #12715 (set known tags by id), and merges both. The base is #12713's branch, so until #12715 merges, its commits also show in this diff.

phase 1              phase 2                              phase 3
            ┌──► #12713 (registry directions) ──┐
#12354 ─────┤                                   ├──► this PR: resolve names by the span's direction
            └──► #12715 (set tags by id) ───────┘

#12713 taught the registry that a few names mean different tags depending on the span's direction: peer.port is the client's port on a server span and the server's on a client span, and server.address is http.hostname inbound but peer.hostname outbound. This PR makes the runtime use that.

KnownTagCodec: lookups that take a direction

  • keyOf(name, direction) and openTelemetryTagOf(id, direction), with descriptive int constants: DIRECTION_INBOUND (server, consumer), DIRECTION_OUTBOUND (client, producer), DIRECTION_NONE (internal), DIRECTION_UNKNOWN.
  • directionalKeyOf(name, direction): only the direction-dependent part. It's a switch over just those names, so a miss is far cheaper than a full lookup.
  • keyOf(name) is unchanged. Every name still costs one table lookup. The table marks the direction-dependent names SHARED_NAME (peer.port) or DIRECTION_SCOPED_NAME (server.address), and only those take a second step.
  • The generated resolver is now pure data: nameOf, openTelemetryNameOf(id, direction), lookup, directionalKeyOf. The codec owns the policy.

TagMap: reading a shared name finds whichever tag the map holds

  • A span holds only one of a shared name's tags, so get("peer.port"), containsKey, remove and read-through to a parent map probe each per-direction id. Only shared names take the probe; custom tags and direction-free names take exactly the path they took before. This keeps the Map contract for name-keyed readers, including getTags() copies.
  • getEntry(long) / getAndRemove(long) find exactly one tag.
  • A bucket holding a single entry now compares the hash as well as the name, as BucketGroup already did. Before this, a lookup by id could hit a custom tag of the same name.

DDSpanContext: resolve names by the span's kind

  • Name-keyed writes, reads (unsafeGetTag, so getTag, the stats aggregator's peer tags and sampling rules) and removes resolve direction-dependent names using the span's kind, then store by id.
  • Every other tag costs a switch miss before TagMap's usual single lookup.
  • A direction-dependent name written before the span has a kind is stored under its bare name and re-keyed when the kind is set. A flag keeps that off every other span.
  • Side effect: the agent's default peer-tags list includes server.address, so on client spans it now reads peer.hostname.

Also

Still to do (draft)

  • Builder tags: resolve eagerly once the kind is known; defer only direction-dependent names written before it.
  • OTLP output: pass the span's direction, so http.hostname → server.address inbound and peer.port → server.port / client.port.
  • Retire span-kind-neutral.
  • Tidy the generated code's indentation.
  • JMH on the setTag / getTag / TagMap lookup paths.

Known gap: a span kind that changes from one direction to the other doesn't re-key entries that were already resolved; only entries written before any kind existed get re-keyed.

Contributor Checklist

  • build-logic :tag-registry:test green
  • :internal-api:test (KnownTags*, TagMap*) and :dd-trace-core:test (DDSpan*, DDSpanContext*, *Otlp*, *TagInterceptor*, new DDSpanDirectionalTagsTest) green

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 2, 2026
@datadog-official

datadog-official Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

🎯 Code Coverage (details)
• Patch Coverage: 75.10%
• Overall Coverage: 59.53% (+0.06%)

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: cc43136 | Docs | Give us feedback!

@dougqh dougqh changed the title Resolve direction-dependent tag names by the span's kind Resolve direction-dependent tag names by the span's kind (otlp - tag registry - phase 3) Oct 2, 2026
@dd-octo-sts

dd-octo-sts Bot commented Oct 2, 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.77 s 14.66 s [-0.1%; +1.6%] (no difference)
startup:insecure-bank:tracing:Agent 13.52 s 13.60 s [-1.4%; +0.3%] (no difference)
startup:petclinic:appsec:Agent 17.21 s 17.08 s [-0.3%; +1.9%] (no difference)
startup:petclinic:iast:Agent 16.97 s 17.03 s [-1.2%; +0.5%] (no difference)
startup:petclinic:profiling:Agent 16.59 s 16.80 s [-2.4%; -0.1%] (maybe better)
startup:petclinic:sca:Agent 17.15 s 17.00 s [-0.2%; +1.8%] (no difference)
startup:petclinic:tracing:Agent 16.13 s 16.12 s [-0.8%; +0.9%] (no difference)

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

@bric3
bric3 added this pull request to stack #12734 October 2, 2026 15:54
@dougqh
dougqh force-pushed the dougqh/tag-registry-direction-resolution branch from f681c6d to 6000c50 Compare October 7, 2026 12:50
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-frames branch from bfc66c5 to 653c907 Compare October 7, 2026 12:50
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-registry-direction-resolution branch from 6000c50 to 4aa5e81 Compare October 8, 2026 12:56
@dougqh
dougqh force-pushed the dougqh/tag-registry-frames branch from 653c907 to 5d44b60 Compare October 8, 2026 12:56
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-registry-direction-resolution branch from 4aa5e81 to a17c830 Compare October 9, 2026 12:37
@dougqh
dougqh force-pushed the dougqh/tag-registry-frames branch from 5d44b60 to 885bdb5 Compare October 9, 2026 12:37
@dougqh
dougqh removed this pull request from stack #12734 October 9, 2026 13:01
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>
Base automatically changed from dougqh/tag-registry-frames to master October 9, 2026 14:50
dougqh and others added 5 commits October 9, 2026 11:33
Set-path routing is a per-language concern, so it lives in a Java overlay
next to tag-conventions.yaml rather than in the language-agnostic
conventions. The overlay declares the keys that exist only to be routed
(resource.name, error, sampling directives, ...) and lists every tag
TagInterceptor may route. Each listed tag's id carries the INTERCEPTED bit
(bit 1), so a setter called with a constant id can fold the interception
test away. Serial numbers become public so TagInterceptor can switch on
them.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TagInterceptor switches on the tag's serial rather than its name, so a
known tag is matched under any of its names and the *_OTEL_NAME cases go
away. needsIntercept(long) tests the INTERCEPTED bit first, which folds
away for a constant id. split-by-tags entries resolve to serials at
construction (each direction for a name declared per direction); only
custom tags are still matched by name. A test checks that exactly the
tags carrying the bit have a case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A name setter resolves a known tag's name to its id once and sets it as the
id-keyed setters do; only a custom tag is still handled by name. The
id-keyed setters no longer look up the name to feed the interceptor, so with
a constant id the interception test folds to the INTERCEPTED bit. Entry
paths (builder ledger, prototypes, default tags) route by the entry's id
and precheck before boxing.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The generated nameOf switched over every tag's serial: 491 bytes of
bytecode, too big for C2 to inline, so each id-keyed set paid two
out-of-line calls into it (the span's unknown-id guard and the entry's
name). Read a NAMES_BY_SERIAL array instead; nameOf is now 23 bytes and
inlines at every caller.

Also adds SetTagBenchmark (span setTag by constant id, non-constant id,
name, and custom name, against a bare and a synchronized TagMap store).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dougqh and others added 4 commits October 9, 2026 11:35
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The table is always allocated and never resized, so the split check -- the
only run-time check left on a non-intercepted constant-id set -- is a single
load, with no null or length test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The span's unknown-id guard called nameOf; KnownTagCodec.isKnown answers the
same question from the serial and a generated SERIAL_LIMIT, so it folds
away for a constant id. Also note that serials are not stable across
releases.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DDSpanContext.setTag calls interceptTag only for intercepted tags. Kept out
of line (over FreqInlineSize, 325 bytes of bytecode), it never brings the
profiled handler bodies into setTag's compiled code, which would push setTag
past InlineSmallCode and stop callers inlining it -- the inlining a constant
id needs to fold its interception test. A test now fails if the switch
shrinks below the limit.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh marked this pull request as ready for review October 9, 2026 18:38
@dougqh
dougqh requested review from a team as code owners October 9, 2026 18:38
@dougqh
dougqh requested review from bric3 and mhlidd and removed request for a team October 9, 2026 18:38
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 9, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-09T18:47:38.206541Z a17c830 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@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: a17c830e25

ℹ️ 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 on lines +1308 to +1312
long tagId = entry.tagId() == 0 ? directionalTagIdToStore(entry.tag()) : 0L;
if (tagId != 0) {
unsafeTags.set(tagId, entry.objectValue());
} else {
unsafeTags.set(entry);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Route bulk tag writes through directional resolution

When a directional key is supplied through CoreSpanBuilder.withTag(...), AgentSpan.setAllTags(...), or a SpanPrototype, the bulk paths in setAllTags, seedAbsentTags, and the ledger handling insert the entry directly instead of applying this resolution. Consequently, even when span.kind is known, peer.port remains a custom tag with ID 0 and is exported as peer.port rather than client.port/server.port; if the kind is assigned later, unresolvedDirectionalTags is never set and the entry is never re-keyed. Apply the same directional resolution to every bulk entry source.

Useful? React with 👍 / 👎.

Comment on lines +185 to 188
public static String openTelemetryTagOf(long tagId, int direction) {
Resolver resolver = Installed.RESOLVER;
String otelName = resolver.openTelemetryNameOf(tagId);
String otelName = resolver.openTelemetryNameOf(tagId, direction);
return otelName != null ? otelName : resolver.nameOf(tagId);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Pass the span direction to OTLP attribute naming

When OTel semantics are enabled, the production JSON and protobuf exporters still call the directionless TagMap.EntryReader.openTelemetryTag(), so this new overload has no production caller that supplies the span's direction. As a result, direction-scoped renames such as http.hostname to server.address and network.client.ip to network.peer.address are not applied on inbound spans, and OTLP output continues using the Datadog keys.

Useful? React with 👍 / 👎.

Comment on lines 925 to +929
public void setSpanKindOrdinal(String kind) {
spanKindOrdinal = spanKindOrdinalOf(kind);
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Re-key tags when the span direction changes

If span.kind is first set to client, a directional tag such as peer.port is stored under the outbound ID; changing the public tag later to server or consumer leaves unresolvedDirectionalTags false, so this method never moves that entry to the inbound ID. Reads then use the new direction and can miss the existing value, while a later write creates a second peer.port entry with the other ID, causing inconsistent map views and duplicate/conflicting OTLP attributes. Re-key existing directional entries whenever the ordinal changes direction, not only when they were initially unresolved.

Useful? React with 👍 / 👎.

Comment on lines +977 to +979
for (TagMap.Entry entry : unresolved) {
unsafeTags.remove(entry.tag());
unsafeTags.set(KnownTagCodec.directionalKeyOf(entry.tag(), direction), entry.objectValue());

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve last-write ordering while resolving aliases

When two names that will resolve to the same tag are written before span.kind—for example server.address followed by the canonical peer.hostname—the older custom entry is collected here and then unconditionally written over the newer known entry. With multiple unresolved aliases such as peer.port and server.port, the winner instead depends on bucket iteration order rather than call order. Re-keying must preserve the normal last-write-wins behavior so setting the kind does not change the value.

Useful? React with 👍 / 👎.

Comment on lines +956 to +959
if (tagId == KnownTagCodec.NO_TAG_IN_DIRECTION_SENTINEL
&& direction == KnownTagCodec.DIRECTION_UNKNOWN) {
unresolvedDirectionalTags = true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Synchronize publishing unresolved tags with setting the kind

When one thread writes a directional tag while another assigns the first span.kind, the writer can read DIRECTION_UNKNOWN, then the kind-setting thread can update the ordinal and observe unresolvedDirectionalTags == false, after which the writer sets this flag and stores the bare custom entry. No later kind transition is required, so that entry remains unresolved and is exported under the wrong name. Coordinate the ordinal/flag transition under the same unsafeTags lock or recheck the direction after publishing the unresolved entry.

Useful? React with 👍 / 👎.

@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

Directional tag updates can leave stale values visible, and assigning a span kind can overwrite newer alias values. Shared-name storage also violates replacement, parent-shadowing, and entry hash-code contracts.

Open Bits AI session

🤖 Bits Code Review · Commit a17c830

if (tagId != 0) {
unsafeTags.set(tagId, value);
} else {
unsafeTags.set(tag, value);

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 Apply directional resolution to bulk tag writes

On a client span, setting peer.port individually and then updating it through setAllTags leaves getTag("peer.port") returning the older value. The bulk Map, TagMap, and ledger paths bypass directional storage, creating bare-name entries instead of replacing resolved IDs. They also fail to mark pre-kind writes for later resolution. Apply the same directional policy across all bulk paths.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

/** {@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);
this.tagHash = tagId != 0 ? tagId : customHash(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 shared-name replacement and parent shadowing

A bare-name peer.port entry and a resolved port ID have different bucket hashes despite sharing the same Map key. Setting an outbound ID to 5432 and then calling put("peer.port", 6000) returns null, increases size to two, and still reads 5432. A resolved local entry also fails to shadow a bare-name parent entry, exposing duplicate serialized keys. Replacement and parent visibility must recognize both representations.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

}
for (TagMap.Entry entry : unresolved) {
unsafeTags.remove(entry.tag());
unsafeTags.set(KnownTagCodec.directionalKeyOf(entry.tag(), direction), entry.objectValue());

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 write order when resolving deferred tags

Writing server.address before the span kind, then writing a newer peer.hostname value, causes setting the kind to client to overwrite the newer value with the older deferred address. Deferred peer.port can similarly remove and overwrite a later explicit outbound-ID write. Re-keying must preserve write precedence; simply skipping occupied destinations would also mishandle the reverse write order.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

Comment on lines +927 to +929
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}

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 Synchronize the deferred-tag flag check with writes

A concurrent tag writer can read DIRECTION_UNKNOWN before the kind writer updates the ordinal, while the kind writer checks unresolvedDirectionalTags before the tag writer sets it. The directional tag then remains under its bare name indefinitely. For server.address on a client span, peer.hostname reads consequently miss the value. Check the deferred flag under the same unsafeTags lock used by tag writers.

Suggested change
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}
synchronized (unsafeTags) {
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}
}

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

this.lazyTagHash = hash;
return hash;
int hash() {
return bucketHash(this.tagHash);

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 Keep entry hash codes consistent with equality

Inbound and outbound peer.port entries with equal values compare equal because Entry.equals uses only the name and value. Entry.hashCode delegates to hash(), which now returns different ID-derived hashes for those entries. Hash-based collections can therefore miss equal entries or retain duplicates. Separate the public equality hash from the bucket hash required for ID lookup.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

dougqh and others added 7 commits October 9, 2026 15:08
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An overlay tag exists only to be routed, so the generator now marks it
intercepted without a second listing; intercepted: lists only the tags
declared in tag-conventions.yaml that the tracer also routes. Also restore
the doc comment and @Suppress that the overlay helper displaced, read the
serial limit one way, inline isUnknownTag, and drop an unused default.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A tag declared once per direction (peer.port) has two ids but one
shared Datadog name, and keyOf resolves that name to neither. An entry
keyed by one of those ids hashed by the id while name-based access
hashed the name as a custom tag, so get/remove/containsKey by name
missed it and a later set by name produced a duplicate key.

Encode a SHARED_NAME flag in the reserved id bit 0 so the check folds
for a constant id. TagMap's id-keyed create/get/remove reject such ids,
and DDSpanContext's id-keyed setters ignore them, as they do unknown
ids, until name resolution knows the span's direction.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Entry's package-private name factories are now custom-only: they skip the
registry and assert the name is not a known tag. Callers that start from a
name resolve it once -- TagMap.set(String, ...) forwards a known tag to
set(long, ...), while getAndSet, put, Ledger.set, putAll and the public
Entry.create(String, ...) API go through anyEntryFor and friends -- so a
custom tag pays one registry lookup instead of two.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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>
#12817 rejected shared-name ids (peer.port inbound/outbound) on the id
paths until name resolution could see a span's direction. With direction
resolution in place, accept them again: a read or removal by the shared
name already checks each direction's id. To keep one entry per tag, a write
by the shared name stores under whichever direction's tag the map holds
(otherwise under the name, which the span re-keys once it has a
direction), and a write by id drops a value held under the name alone.
isKeyableById goes; the SHARED_NAME bit stays and keeps the new check
foldable for a constant id.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@dougqh
dougqh force-pushed the dougqh/tag-registry-direction-resolution branch from a17c830 to d509119 Compare October 10, 2026 02:03
@dougqh
dougqh requested a review from a team as a code owner October 10, 2026 02:03

@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

Bulk updates and inherited defaults can leave bare-name and direction-specific entries for the same tag, producing stale peer.port reads and duplicate serialized keys. Concurrent kind assignment can also miss deferred resolution, and equal directional entries now receive different hashes.

Open Bits AI session

🤖 Bits Code Review · Commit d509119

Object value = tagEntry.objectValue();

if (!ctx.tagInterceptor.interceptTag(ctx, tag, value)) {
if (!ctx.tagInterceptor.interceptTag(ctx, tagEntry)) {

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 Resolve directional names in bulk tag writes

On a client span, set peer.port=80, then call setAllTags with a separate TagMap containing peer.port=81. The bulk path inserts a bare-name entry alongside the outbound-ID entry, so getTag("peer.port") still returns 80 instead of 81 and serialization can emit duplicate keys. Bulk, ledger, and prototype paths need the same directional resolution and deferred-resolution bookkeeping as individual setters.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

// One direction of a tag declared per direction, written by id, supersedes a value written
// under the shared name alone (before a span knew its direction), so the map never holds
// both.
this.removeLocal(newEntry.tag, Entry.customHash(newEntry.tag));

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 Shadow inherited bare-name tags when storing a directional ID

When a frozen parent contains bare-name peer.port and its child writes PEER_PORT_OUTBOUND_ID, removeLocal cannot hide the inherited entry. Parent visibility compares bucket hashes, which now differ between the two representations, so iteration and size expose both mappings. Configured tracer tags use this parent mechanism; copying the span's tags into an ordinary map can overwrite its explicit value with the inherited default. Parent shadowing must recognize equivalent shared-name representations.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

Comment on lines 925 to +928
spanKindOrdinal = spanKindOrdinalOf(kind);
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}

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 Synchronize kind assignment with deferred directional writes

A directional-tag setter can read DIRECTION_UNKNOWN under unsafeTags, then another thread can publish the first span kind and observe unresolvedDirectionalTags=false before the setter raises it. Resolution is permanently skipped: server.address remains a custom tag and getTag("peer.hostname") returns null on the client span. Publish the kind and inspect the deferred flag under the same lock as directional writes.

Suggested change
spanKindOrdinal = spanKindOrdinalOf(kind);
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}
synchronized (unsafeTags) {
spanKindOrdinal = spanKindOrdinalOf(kind);
if (unresolvedDirectionalTags) {
resolveDirectionalTags();
}
}

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

}

int hash() {
return bucketHash(this.tagHash);

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 equal hashes for name-equal tag entries

Entries created with PEER_PORT_INBOUND_ID and PEER_PORT_OUTBOUND_ID and the same value compare equal because Entry.equals compares their shared Datadog name and value. Entry.hashCode delegates to this changed method, giving those equal entries different hashes and causing failed lookups or duplicates in hash collections. Separate the internal ID-based bucket hash from the public equality-compatible hashCode.

Was this helpful? React 👍 or 👎
🤖 Bits Code Review · Open Bits AI session

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