Add the tag registry and map OpenTelemetry tag names through it - #12354
Add the tag registry and map OpenTelemetry tag names through it#12354dougqh wants to merge 4 commits into
Conversation
dougqh
added
comp: core
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a98850bf24
ℹ️ 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".
| extends: base | ||
| tags: | ||
| - { dd-name: http.method, type: string, required: required, otel-name: http.request.method } | ||
| - { dd-name: http.status_code, type: int, required: conditional, otel-name: http.response.status_code } |
There was a problem hiding this comment.
For normal HTTP spans, http.status_code is intercepted into Metadata.httpStatusCode rather than retained as a TagMap.EntryReader, and MetaWriter.accept writes that field directly using the fixed HTTP_STATUS key. Consequently this declared rename is never consulted and OTLP still emits http.status_code instead of http.response.status_code for essentially every span with a status; the adjusted test masks this by accepting either name. Apply the registry mapping to the first-class metadata emission as well.
Useful? React with 👍 / 👎.
| String otelName = tagEntry.openTelemetryName(); | ||
| String key = otelName != null ? otelName : tagEntry.tag(); |
There was a problem hiding this comment.
When a span contains both a Datadog key and its OTel alias—for example, auto-instrumentation sets http.method while application OTel code sets http.request.method—both remain distinct entries in TagMap, but this projection writes both as http.request.method. The resulting OTLP attribute list has duplicate keys, so downstream map conversion selects one value according to iteration order. Canonicalize or deduplicate by tag ID before serialization and define which value wins.
Useful? React with 👍 / 👎.
| * "not yet resolved" (0L is a valid result -- unknown tag / inactive codec -- so it cannot be | ||
| * the sentinel). | ||
| */ | ||
| long lazyTagId = TAG_ID_NOT_COMPUTED; |
There was a problem hiding this comment.
perf: Every span tag is represented by an Entry, so this new long adds an unconditional eight-byte payload per tag even when OTLP export is disabled or tagId() is never called; entries are retained with spans until trace flushing, multiplying the heap and GC cost for high-tag-count or long traces. Keep this exporter-specific cache outside the ubiquitous entry object, or verify the tradeoff with JOL plus an allocation/retained-heap benchmark before accepting it.
AGENTS.md reference: AGENTS.md:L78-L78
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Currently, that's true, but I want to expose an API that allows each serializer to use the appropriate namespace easy in the future. And tagRegistry / tagId will be used to drive other changes in future PRs, too.
| - { dd-name: origin, kind: structural, field: origin } # trace-level field | ||
| - { dd-name: sampling.priority, kind: directive } | ||
| - { dd-name: manual.keep, kind: directive } | ||
| - { dd-name: manual.drop, kind: directive } | ||
| - { dd-name: measured, kind: directive } |
There was a problem hiding this comment.
The live keys intercepted by TagInterceptor are DDTags.ORIGIN_KEY (_dd.origin) and DDTags.MEASURED (_dd.measured), but these reserved rows register origin and measured instead. As a result, KnownTagCodec.keyOf returns zero for the keys the tracer actually uses while assigning reserved IDs to names that never reach these handlers, so consumers cannot use the new registry to classify those structural/directive tags. Use the existing constant values as the canonical names or explicitly register them as aliases.
Useful? React with 👍 / 👎.
| /** Full stored-tag universe (concrete span types' resolves + trace-level), de-duped by name. */ | ||
| fun allStoredTags(): List<Tag> { | ||
| val union = LinkedHashMap<String, Tag>() | ||
| for (type in concreteTypes()) for (t in resolve(type)) union.putIfAbsent(t.name, t) | ||
| for (t in traceLevel) union.putIfAbsent(t.name, t) | ||
| return union.values.toList() |
There was a problem hiding this comment.
allStoredTags() constructs the registry only from resolved concrete span types, but the new ci_visibility mixin applies to test, which the YAML explicitly says is not modeled yet. Consequently its actively used test.name, test.suite, test.status, and test.framework declarations are absent from KnownTags and tag-assignment.txt, and keyOf treats them as unknown. Include mixin declarations in the identity-only registry independently of layout resolution, or model the test span type before generating it.
Useful? React with 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
🟢 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. |
Introduce the tag-registry code generator (buildSrc plugin + the language-agnostic tag-conventions.yaml and its Java overlay) and the KnownTagCodec it populates, so a tag has one stable in-process id that name lookups resolve through. A tag id here is IDENTITY only: a globally unique serial plus classification bits (intercepted, trace-level). It carries no storage-layout coordinate -- bits [47-32] are documented and held vacant for the co-occurrence slot that the dense tag store assigns by graph coloring, which lands with that store. Nothing in this commit decides how, or whether, a tag is stored. - KnownTagCodec: id encoding (serialNum / reserved-vs-stored tiers / intercepted / trace-level) plus the register-once, then-locked Resolver. Always conceptually present: an empty NoKnownTagCodec installs itself on first use if nothing was registered, so keyOf returns 0 and every tag reads as unknown -- identical to having no registry at all. - Generated KnownTags (committed under src/generated, on the main compile path): per-tag NAME/ID couplets, the StringIndex-backed keyOf table, and the serialNum switch behind nameOf. - verifyKnownTags freshness gate wired into `check`, so a stale commit of the generated sources fails CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a98850b to
6d5eb31
Compare
A reader can now answer both "which tag is this" and "what is it called in the OpenTelemetry namespace" — the read surface a serializer needs. tagId() resolves lazily via KnownTagCodec.keyOf using the same memoized-field idiom as lazyTagHash (0L means unknown tag / inactive codec, so the not-yet-resolved sentinel is Long.MIN_VALUE). openTelemetryTag() is layered on it and never returns null. The naming policy stays in one place -- KnownTagCodec.openTelemetryTagOf, which yields the declared rename, else the Datadog name (pass-through), else null for a tag the registry does not know. The reader completes exactly that last case by falling back to its own key, which it can do and the codec cannot: a custom tag has no registry name, and only the holder of the entry knows what it was written under. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
OTLP is the OpenTelemetry wire format, so render each tag under its OpenTelemetry name: the rename the registry declares (http.method -> http.request.method, http.useragent -> user_agent.original, ...), else its Datadog name — pass-through, the default. Both emission paths resolve through the same registry policy: - Tags still in the TagMap, via TagMap.EntryReader.openTelemetryTag(). - Tags the tracer INTERCEPTS into first-class Metadata fields, which never reach that projection. http.status_code is the live case: it is lifted into Metadata.httpStatusCode and written under a fixed key, so a rename applied only to the map would never have reached it and OTLP would have kept emitting http.status_code for essentially every span with a status. Its key — and service.name, previously spelled out by hand here — now come from the registry. RemoteMapper.HTTP_STATUS is untouched: the Datadog writer must keep emitting http.status_code. Wire-visible change: OTLP now emits http.response.status_code instead of http.status_code, per the DDOT HTTP semconv RFC. Those intercepted keys live in a nested Keys holder so they resolve on FIRST USE rather than at class load. Resolving against an unregistered codec would silently freeze them to their Datadog names, and class load can precede tracer init (a caller may hand CoreTracer an already-built writer); first use, by contrast, is a span serialization, which cannot happen without a tracer. Being static final on an initialized class they still constant-fold on the write path. OtlpTraceProtoKeyResolutionForkedTest pins both halves — that class load leaves the registry dormant, and that the keys are the registry's names once it is live — in a JVM where nothing has built a tracer. The test's tag assertions are now EXACT rather than "either name": accepting either is what let the http.status_code rename hide. This is the straight rename projection only. Suppressing a Datadog-only tag from OpenTelemetry, per-exporter opt-in, additional namespaces, and de-duplicating a span that carries both spellings of one tag are follow-ons. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Parity test for the generated KnownTags registry and the resolver it installs: keyOf/nameOf round-tripping across the tag groups, the many->one inbound mapping (a Datadog name and an OpenTelemetry name both resolve to the one id) with the per-namespace accessors taking each back out, and the intercepted / reserved / stored / trace-level partitioning of the id. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
6d5eb31 to
d712502
Compare
| * tracer — and a tracer registers the resolver at init. Being {@code static final} on an | ||
| * initialized class, they still constant-fold on the write path. | ||
| */ | ||
| private static final class Keys { |
There was a problem hiding this comment.
Not sure how, I feel about this.
Extracting into a helper class that can be lazily initialized separately.
In part, this is hiding some of the fun of wiring KnownTags into KnownTagCodec.
|
|
||
| // Register the KnownTagCodec resolver so tag-id name resolution (keyOf/nameOf, and the | ||
| // OpenTelemetry name mapping the OTLP writer reads) is live for the life of the tracer. | ||
| KnownTags.init(); |
There was a problem hiding this comment.
I don't particularly like this bit of initialization coupling.
However, I did want to keep the KnownTagsCodec.Provider pluggable for testing purposes.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d712502ee8
ℹ️ 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".
| - { dd-name: manual.keep, kind: directive } | ||
| - { dd-name: manual.drop, kind: directive } | ||
| - { dd-name: measured, kind: directive } | ||
| - { dd-name: analytics.sample_rate, kind: directive } # legacy |
There was a problem hiding this comment.
When analytics sampling is set through the tracer API, TagInterceptor matches DDTags.ANALYTICS_SAMPLE_RATE, whose value is _dd1.sr.eausr; the repository has no handler for analytics.sample_rate. Consequently KnownTagCodec.keyOf("_dd1.sr.eausr") returns zero, while the generated reserved ID belongs to a key that is stored as an ordinary tag rather than intercepted. Use the live constant value as the reserved name or register it as an alias.
Useful? React with 👍 / 👎.
| # TagInterceptor chain. | ||
| # kind: structural -> sets a span/trace field (`field:` names it) | ||
| # kind: directive -> triggers sampling/trace behavior | ||
| reserved: |
There was a problem hiding this comment.
The reserved registry omits asm.keep, ai_guard.keep, _dd.p.ts, and _dd.p.debug, although TagInterceptor.needsIntercept and interceptTag actively consume all four instead of storing them. A repo-wide search finds no declarations for them in either conventions file, so KnownTagCodec.keyOf reports these live AppSec, AI Guard, and debugger directives as unknown and any ID-based handler built on the new registry cannot reproduce the current interceptor behavior.
Useful? React with 👍 / 👎.
| - http.method | ||
| - http.url | ||
| - servlet.context | ||
| - db.statement |
There was a problem hiding this comment.
For every db.statement set through the normal span API, TagInterceptor.interceptDbStatement returns true, so the value is converted into the resource name and never stored in TagMap. Declaring it in this stored-tag list nevertheless generates an ID for which KnownTagCodec.isStored is true, contradicting that API's documented meaning and causing any registry-driven storage routing to retain raw SQL instead of preserving the current consume-only behavior. Model this key as reserved structural data rather than intercepted-but-stored.
Useful? React with 👍 / 👎.
1 participant
What Does This Do
Extracts the tag registry — generated tag ids plus name resolution — from the dense-store stack, so it can land on its own.
A tag id here is identity only: a globally unique serial plus classification bits (intercepted, trace-level). It carries no storage-layout coordinate. Bits
[47-32]are documented and held vacant for the co-occurrence slot that the dense tag store assigns by graph coloring, which lands with that store. Nothing in this PR decides how — or whether — a tag is stored, andTagMapstorage behavior is unchanged.On top of that identity, the registry gives each tag a per-namespace name, and OTLP starts using it:
keyOfis many→one — a Datadog name or an OpenTelemetry name both resolve to the one id.datadogNameOf/openTelemetryNameOftake the name back out per namespace.nameOfstill returns the Datadog name; outbound is namespace-specific, not normalized.OtlpTraceProtorenders each known tag under its OpenTelemetry rename when it declares one (http.method→http.request.method,http.useragent→user_agent.original, …), falling back to the Datadog name otherwise.otel-nameis optional and tri-state in the conventions: absent means pass-through under the Datadog name (the RFC "retain" default), a value renames, and the literalnonereserves a Datadog-only tag. Because pass-through is the default, every known tag today has an OpenTelemetry name — so there is deliberately no OTel-applicability bit in the id; it would be constant. It returns once anonetag exists.Commits
tag-conventions.yaml+ Java overlay → committedsrc/generated/KnownTags.java),KnownTagCodec, and theverifyKnownTagsfreshness gate wired intocheck.TagMap.EntryReader— lazily-resolvedtagId(), andopenTelemetryName()layered on it.Note on the resolver hand-off
KnownTagCodecresolves its resolver exactly once via a holder class, using JVM class initialization as the lock rather than explicit synchronization. The payoff is on the read side:Installed.RESOLVERis astatic finalof an initialized class, so the JIT constant-folds it, and a constant receiver carries an exact klass — the resolver's switch devirtualizes and inlines outright, with no CHA dependency that a later class load could invalidate. Worth being precise that CHA alone would not get there:Resolverhas two implementors loaded (the generated one and the empty fallback), so a non-constant receiver would stay bimorphic at best.The ordering contract is the usual lock-once one:
registermust happen before the first name resolution (CoreTracerdoes this at init). A registration that loses that race gets the empty fallback — safe, every tag simply reads as unknown — andregisterreports it on a best-effort basis.Motivation
PR #12230 maps OpenTelemetry tag names via the registry, and another team is waiting on it. It was based on #12047, which sits on top of the dense store (#12045) and the colored-slot encoding (#12046) — so the OTel work was blocked behind the whole storage stack.
That dependency was real rather than incidental: #12047's generator produces ids in the slot-encoded format #12046 introduces. But the split is clean —
TagRegistry.build()already separates serial assignment and OTel-name validation (which never touchslot) from the graph coloring. This PR is the identity-and-names half, targeting master directly.Additional Notes
slot/NO_SLOT/isUnslotted/slotCount,DENSE_STORE/routesToDense,Resolver.slotCount(), and the two pure-layout reports (layout-by-type.txt,folded-types.txt). Those belong with the dense store.intercepted, andLEVEL_TRACE— conventions facts (the overlay'sintercepted:list, the YAML'strace_level:tier) with no runtime machinery behind them. Since the slot window is left zero and documented, re-adding coloring later is purely additive.Contributor Checklist
./gradlew spotlessApply:internal-api:test,:internal-api:spotbugsMain,:internal-api:spotlessJavaCheck,:internal-api:verifyKnownTagsall green:dd-trace-core:Otlp / TagMap / CoreTracer tests greenTracerConnectionReliabilityTestfailures were checked against a cleanorigin/masterworktree and reproduce there — pre-existing, unrelated to this changeJira ticket
N/A
🤖 Generated with Claude Code