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 5 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 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 8d60761 | Docs | View more details | 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. |
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 👍 / 👎.
80011c9 to
6439499
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e1491718c
ℹ️ 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".
| // supplying its own key for a custom tag the registry does not name. This is the straight | ||
| // rename projection only: suppressing a Datadog-only tag from OpenTelemetry, per-exporter | ||
| // opt-in, and additional namespaces are deferred to the OpenTelemetry follow-on. | ||
| String key = tagEntry.openTelemetryTag(); |
There was a problem hiding this comment.
When OtlpConfig.Protocol.HTTP_JSON is selected, OtlpWriter uses OtlpTraceJsonCollector, whose OtlpTraceJson.writeSpanTag still emits tagEntry.tag() and whose metadata writer still uses the fixed HTTP_STATUS key. Although this protobuf path now applies the registry projection, JSON exports therefore continue to emit Datadog names such as http.method and http.status_code instead of http.request.method and http.response.status_code; apply the same per-entry and intercepted-metadata mapping to the JSON encoder.
Useful? React with 👍 / 👎.
| private fun tagList(tags: Any?): List<Tag> = | ||
| (tags as? List<Map<String, Any?>>)?.map { m -> | ||
| Tag( | ||
| name = m["dd-name"].toString(), |
There was a problem hiding this comment.
If a future YAML declaration omits or misspells dd-name, Kotlin's nullable toString() converts the missing value into the literal tag name "null". Generation and the freshness check then succeed with a public NULL_* registry entry while the intended tag is silently absent, so a simple conventions typo can ship a corrupted registry instead of failing CI; require a nonblank string here as is already done for otel-name.
Useful? React with 👍 / 👎.
| writeSpanTag(buf, THREAD_NAME, metadata.getThreadName()); | ||
| if (metadata.getHttpStatusCode() != null) { | ||
| writeSpanTag(buf, HTTP_STATUS, metadata.getHttpStatusCode()); | ||
| writeSpanTag(buf, HTTP_STATUS_CODE_KEY, metadata.getHttpStatusCode()); |
There was a problem hiding this comment.
For the normal setHttpStatusCode path, Metadata.getHttpStatusCode() is a UTF8BytesString, so this call selects the CharSequence overload and writes http.response.status_code as an OTLP string_value. The conventions declare this renamed attribute as int, and downstream OTel consumers expecting the semantic-convention integer will see the wrong type for essentially every auto-instrumented HTTP status; pass the numeric status to the long-attribute overload instead.
Useful? React with 👍 / 👎.
Introduces a language-agnostic tag registry. tag-conventions.yaml declares each known tag once; a buildSrc Gradle plugin generates KnownTags.java from it into internal-api/src/generated, committed and on the main compile path, with a verifyKnownTags task wired into check so a stale checkout fails the build rather than drifting. A tag id is IDENTITY, not storage: a globally unique 16-bit serial in bits [63-48] plus the trace/span level bit. It says what a tag is, never how it is stored or set. Bits [47-32] are documented and held vacant for the co-occurrence slot the dense tag store assigns by graph coloring, so that store lands as a purely additive change. KnownTagCodec is the hand-written half of the registry -- bit layout and naming policy -- and generated KnownTags is the data half. Java has no partial classes, so the codec's Installed holder names KnownTags.RESOLVER directly: reading a tag name is what initializes the registry, with no registration call and no ordering to get wrong. The holder keeps the resolver in a static final of an initialized class, which the JIT constant-folds to an exact klass, so keyOf/nameOf devirtualize and inline with no lock, volatile read, null check or virtual call. The nesting is load-bearing: KnownTags calls back into the codec, so a field on the codec itself would re-enter a half-initialized class. keyOf is many-to-one -- a Datadog name or an OpenTelemetry name both resolve to the one id -- and datadogNameOf/openTelemetryNameOf take the name back out per namespace. openTelemetryTagOf is the single home for the pass-through policy (declared rename, else the Datadog name), so no serializer re-decides it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
tagId() resolves the entry's key through the registry, and openTelemetryTag() layers the namespace projection on top: the tag's declared OpenTelemetry rename when it has one, otherwise its own key. The reader supplies that last fallback because a custom tag has no registry name, and only the holder of the key can name it. Entry.tagId() is deliberately computed rather than memoized, unlike hash(). TagMap$Entry is the tracer's largest allocation source -- one per tag per span, on the app thread -- and it packs to 40 bytes with only 3 bytes of padding, so a long memo widens every entry to 48 and adds a putfield per construction. The only caller is serialization: background thread, once per entry, where keyOf is a single open-addressed probe over a static final table keyed on an already-cached String hash. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
OTLP is the OpenTelemetry wire format, so each known tag is rendered under its OpenTelemetry name: http.method as http.request.method, http.useragent as user_agent.original, and so on, falling back to the Datadog name for a tag that declares no rename and to the entry's own key for a custom tag the registry does not know. Tags the tracer intercepts into first-class Metadata fields never reach the per-entry projection, so their keys resolve through the same registry policy once each at class init -- otherwise service and http.status_code would keep emitting under Datadog names while every other tag moved. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
KnownTagsTest pins the registry itself: keyOf/nameOf round-trip for every known tag, many-to-one resolution from either namespace, serial encoding and the trace-level bit, and unknown names resolving to 0. TagMapNamespaceNamesTest covers EntryReader.openTelemetryTag() -- rename, pass-through, and the custom-tag fallback the codec cannot supply. OtlpTraceProtoTest asserts attributes now land under OpenTelemetry names. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The registry was built by resolving concrete span types, so a tag reached the union only through a type that could carry it. A mixin attaches via its applies: targets, and ci_visibility applies to `test`, which the conventions do not model yet -- so test.name, test.suite, test.status and test.framework were dropped, silently. keyOf reported live CI Visibility tags as unknown, and nothing in the generated output showed the four declarations had gone anywhere. An id is identity, and identity does not depend on layout: a declared tag gets an id whether or not a modeled span type can carry it today. So the registry is now built from declarationGroups() -- trace-level, span types and every mixin -- which is also what makes mixins behave uniformly rather than working only when their target happens to be modeled. The layout gap is real, just not fatal, so the generator reports it: resolved-tags.txt names each mixin whose applies: target is unmodeled and says its tags are registered but occupy no per-type slot. Declaring tags ahead of the span type that will carry them is a legitimate intermediate state; being unable to see it is not. Registry grows 47 -> 51 tags. Serials are assigned over the sorted name list, so the four insertions shift the ids of tags sorting after them -- fine, since ids are in-process with no cross-release stability. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
9e14917 to
8d60761
Compare
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 the trace/span level bit. It says what a tag is, never how it is stored or set. 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.openTelemetryTagOfis the one place the pass-through policy lives (declared rename, else the Datadog name), so no serializer re-decides it.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 in the conventions: absent means pass-through under the Datadog name (the RFC "retain" default), a value renames. 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 a Datadog-only tag exists.Commits
tag-conventions.yaml→ committedsrc/generated/KnownTags.java),KnownTagCodec, and theverifyKnownTagsfreshness gate wired intocheck.TagMap.EntryReaderits tag id and OpenTelemetry name —tagId()andopenTelemetryTag()layered on it.Rebased onto current master. Three corrective commits — dropping the
Entry.tagId()memo, removing intercept modelling, and linkingKnownTagCodecto its generated half — have been squashed into the four above, so each commit reads as its end state. The three sections below record why, since the reasoning is no longer visible in the history.Why
Entry.tagId()is not memoizedTagMap$Entryis the tracer's largest allocation source, one per tag per span on the app thread, and it packs to 40 bytes with only 3 bytes of padding — so a memo field widens every entry to 48 and adds aputfieldper construction. The only caller is serialization: background thread, once per entry, andkeyOfis a single open-addressed probe over astatic finaltable keyed on an already-cachedStringhash. Paying it there beats widening every entry. (The dense store will want an id-carryingset(long, String, Object)overload on the reused flyweight instead, which is free — everyEntrythat survives dense storage is a custom tag, so itstagId()is 0 and a memo's hit rate would be exactly zero.)Why intercept modelling is gone
The registry originally mirrored
TagInterceptor.needsInterceptas an id bit plus a serial-range tier. Codex found four places where the mirror had already drifted from the switch that is the actual authority, and nothing consumed the bit at runtime — it was unused speculative surface whose only effect was to go stale. An id says what a tag is, not how it is set; interception is aTagInterceptorproperty. The classification returns with the work that consumes it (the id→handler dispatch table that retiresTagInterceptor), and re-adding a bit then is purely additive.This also removed the need for a second conventions file:
tag-conventions.java.yamlexisted only to carry the Java-specificintercepted:list, so the registry is now one language-agnostic YAML.Why there is no resolver registration
KnownTagCodec(hand-written: bit layout, naming policy) and generatedKnownTags(the name↔id tables) are two halves of one class. Java has no partial classes, so the halves were originally joined at runtime:KnownTagsregistered its resolver in a static initializer, and the codec captured whatever had registered by the time it first resolved a name.That left a window.
Installed.RESOLVERisstatic final, so a read before registration latched the empty fallback permanently — every tag unknown for the life of the JVM, with no exception and no log line. Closing it took aCoreTracerpoke ofKnownTags.init(), and any class resolving a name in its own initializer needed a private lazy holder to dodge it (OtlpTraceProto.Keys, pinned by a forked test). A per-site guard against a process-global hazard, with silent failure for anyone who forgot.Since
KnownTagsis generated into the same module and package on the main compile path,Installedcan just nameKnownTags.RESOLVER— javac resolves the link, and reading a name is what initializes the registry. The window has zero width, soinit(), theCoreTracercoupling, theKeysholder and its forked test, the empty fallback codec,register()andisActive()all go away.The nested holder stays, and now earns its keep for a different reason:
KnownTagscalls back intoKnownTagCodec.serialNum, so wereRESOLVERa field of the codec itself, the codec's own initializer would re-enter on the same thread and silently read defaults. One class down means the codec is fully initialized beforeKnownTagsstarts. With it stays the read-side payoff —static finalof an initialized class, constant-folded, exact klass, so the resolver's switch devirtualizes and inlines outright.The trade:
KnownTagCodecno longer compiles without generatedKnownTags, soverifyKnownTagsbecomes load-bearing rather than a convenience. A build failure beats a silent runtime degradation.Resolverpluggability for tests goes with it — it had no users.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 separated 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.LEVEL_TRACE— conventions facts (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.ci_visibility:test.name,test.suite,test.status,test.framework) are silently absent from the generated registry. The generator should error on an unreachable declaration rather than drop it. Not fixed here.Contributor Checklist
./gradlew spotlessApply:internal-api:test,:internal-api:spotbugsMain,:internal-api:spotlessJavaCheck,:internal-api:verifyKnownTagsall green:dd-trace-core:test --tests '*Otlp*',:dd-trace-core:spotlessJavaCheck,:dd-trace-core:forkedTestgreenTracerConnectionReliabilityTestfailures were checked against a cleanorigin/masterworktree and reproduce there — pre-existing, unrelated to this changeJira ticket
N/A
🤖 Generated with Claude Code