Route tags by registry id instead of by name - #12398
Draft
dougqh wants to merge 3 commits into
Draft
Conversation
The domain spec (tag-conventions.yaml) deliberately models only what a tag IS,
and defers set-path routing to "a per-language overlay alongside this file".
This adds that overlay for Java, restoring the intercepted/reserved taxonomy
from the pre-OTLP-split generator work.
intercepted: domain tags this tracer also routes (named, not redeclared --
they already have an identity; this only flags it)
reserved: keys accepted by setTag but diverted to a span field or a trace
directive, which exist only because this tracer routes them and
so have no place in a cross-language contract
Both get bit 3 of the tag id, INTERCEPTED. The point is speed: a TagMap entry
carries its own tag id, so screening a bundle for anything the interceptor cares
about is a mask test on an id already in hand -- no name lookup, no side table.
An earlier version of this bit was deleted because it could disagree with
TagInterceptor's switch. It returns because the agreement becomes a test rather
than a convention; that test lands with the dispatch rework that consumes it.
Nothing here records whether a routed tag is also STORED. That is decided per
call from the value -- http.url is routed and stored, manual.keep is consumed
only when its value coerces to a boolean -- so it is not a property of the tag,
and a static flag mirroring it would be the same drift in a new place.
Reserved serials are assigned after every domain serial, so a Java-only key
cannot renumber the domain block: of the 51 existing tags, only the 8 named in
`intercepted` change at all, and only in that flag bit.
TagInterceptor's two parallel string switches become one id lookup: the pre-screen is a mask test on the INTERCEPTED flag, and the dispatch is an int switch over dense serials (a tableswitch, where the name switch was a lookupswitch on string hashes plus an equals() per hit). DDSpanContext resolves the id once and hands it to both, so a routed tag is never looked up twice; a TagMap entry already carries its own id, so the bundle screens cost a mask per entry. Because keyOf is many->one, a tag now routes under every name it is known by. The hand-maintained "service.name"/"service" pair of case labels collapses into the one `service` serial, and OpenTelemetry names route without a second label. splitServiceTags stays a name lookup -- it is user configuration and may name a custom tag with no id -- but is now skipped outright when unset. TagInterceptorRoutingTest asserts the declared flag set is exactly the set the switch handles. That test is what licenses the flag: an earlier version of it was deleted because the declaration and the switch could drift apart silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
Contributor
🟢 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. |
Both the id-based screen/dispatch and a frozen copy of the pre-change name switch live here as arms: the old switch is deleted, so there is no one-binary flag, and a two-jar A/B would conflate master drift. This is a CPU-not-allocation lever, so flat gc.alloc.rate.norm across every arm is the correctness check, not the result. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Stacked on #12354 — review that first; this PR's diff is the last two commits.
Retires
TagInterceptor's two parallel string switches in favour of the tag registry's ids.The Java overlay arrives.
tag-conventions.yamlis the language-agnostic domain spec, and its header defers set-path routing to "a per-language overlay alongside this file".tag-conventions.java.yamlis that overlay. It has two sections:intercepted:— names of domain tags this tracer also routes. Named, not redeclared: they already have ids.reserved:— keys accepted bysetTagbut diverted to a span field or a trace directive, existing only because this tracer routes them (manual.keep,_dd.origin,sampling.priority, …). These get ids of their own.Both sections' keys carry a new
INTERCEPTEDflag (bit 3) in their id. Domain serials are assigned first, so a Java-only reserved key cannot renumber the shared spec's generated output.Routing moves onto ids. The pre-screen becomes a mask test on that flag; the dispatch becomes an int switch over dense serials — a
tableswitch, where the name switch was alookupswitchon string hashes plus anequals()per hit.DDSpanContextresolves the id once and hands it to both, so a routed tag is never looked up twice, and aTagMapentry already carries its own id, so the per-bundle screens inCoreTracercost one mask per entry with no name comparison at all.splitServiceTagsstays a name lookup — it is user configuration and may name a custom tag with no id — but is now skipped outright when unset.Motivation
TagInterceptorkept two hand-maintained string switches — one to decide whether a tag needs routing at all, one to dispatch it — listing the same twenty-odd tags twice. The registry already knows which tags exist and what they are called; asking it removes the duplication, and the id it returns is reusable by everything downstream (dispatch, storage, outbound naming) where a name has to be re-examined at each step.It also removes a class of silent drift.
KnownTagCodecrecords that an earlier classification bit was deleted because it could disagree with the interceptor's switch, with no error when it did. The bit returns here only because that agreement is now asserted by a test.Both matter for the follow-on OpenTelemetry naming work (#12230), which needs one identity per tag across namespaces rather than a name the interceptor happens to recognise.
Additional Notes
Behaviour change worth a look
keyOfis many→one, so a tag now routes under every name it is known by. That collapses the hand-maintained"service.name"/"service"pair of case labels into the oneserviceserial, which is pure cleanup. But it also means OpenTelemetry names now route rather than merely store:url.fullhttp.urldoes)db.query.textdb.statementdoes)http.response.status_codeThis is the registry's premise — one identity across namespaces — and it is what the follow-on OTel work wants. It is called out here because it is a semantic expansion, not a refactor, and is the one thing in this PR that a reviewer might want gated behind a flag instead.
Namespace handling is a deliberate choice, not a side effect. The registry can emit more than one name→id table -- a Datadog-names table, an OpenTelemetry-names table, and a combined one -- and each entry point can resolve against whichever it should. This PR uses the combined table, so a tag routes under every name it is known by. That is the simple option and it is chosen on purpose.
The alternative considered was restricting the Datadog set path to Datadog spellings, which would have made this PR strictly behaviour-preserving. It was not taken because the resulting behaviour is the one customers want: an OpenTelemetry user setting
url.fulltoday gets a span with no resource name, while a Datadog user settinghttp.urlgets one -- the same span, worse product, decided only by which SDK the call came through. The combined table closes that gap.What is deferred, not decided here: whether the OTel bridge should translate names on the way in (arguably the layer this belongs in), and whether the default table per entry point should be declared in the shared conventions rather than chosen per language. Both become cheap once the per-namespace tables exist; neither blocks this PR.
Why the flag is allowed back
KnownTagCodecrecords that an earlier classification bit was deleted because it could disagree withTagInterceptor's switch, silently. It returns only because the agreement is now asserted:TagInterceptorRoutingTestwalks every serial the registry has assigned and checks, behaviourally, whether the tag reaches the switch'sdefaultbranch — configuring every known tag as a split-service tag makes that branch, and only that branch, callsetServiceName(value, SPLIT_BY_TAGS). The declared set must equal the handled set in both directions. Verified to have teeth: deleting one case label fails it with the right message.The generator also fails the build on two ways the overlay can be wrong: a
reserved:key the domain spec already declares (that would mint a second identity for one tag), and anintercepted:name that matches no domain tag (a typo there would silently flag nothing and stop the pre-screen recognising a key the interceptor still handles).Benchmark
TagInterceptorScreenBenchmark(new,dd-trace-core/src/jmh). Both implementations live in it as arms: the old name switch is deleted by this PR, so there is no one-binary flag to A/B, and a two-jar master-vs-branch run would conflate master drift. TheByNamearms are a frozen verbatim copy of the pre-change 22-label switch.-f3, 5×5 iterations,@Threads(8),-prof gc. Throughput, ops/us, higher is better.Where the id is already in hand. This is the path that matters:
CoreTracerscreensdefaultSpanTags/localRootSpanTags/mergedTracerTagsas bundles, andDDSpanContext.setTag(EntryReader)reads the entry's own id. NokeyOfat all. Bundle is a 7-entry web-shapedTagMapwith nothing routed in it, so the scan runs to completion.Where the name must be resolved first. Mixed with the shipped config, and clearly negative once split-by-tags is populated:
That is the
keyOftax: at aStringcall site the id path pays an open-addressed string lookup thelookupswitchdid not, and on a miss neither path can then skip thesplitServiceTagsset probe.Three things to read alongside those rows, in order of weight:
screenById_*callsneedsIntercept(String), which resolves the id internally and discards it. Production does not:DDSpanContext.setTagresolves once and hands the same id to both the screen andinterceptTag. The arm payskeyOfand gets none of the dispatch saving back, so −28% is a worst-case bound on a call shape the tracer does not have, not the production delta.splitByTagsships empty, sooffis the default-config column.screenById_custom offat +82% is the one number with no mechanism offered for it. It is reported, not relied on.Allocation is flat and ≈0 on all twenty rows (
gc.alloc.rate.norm10⁻⁵–10⁻⁷ B/op,gc.count ≈ 0). This is a CPU lever, so that is the correctness check passing, not the result.Directional only. The micro over-states its own share of a real span; PetClinic is the acceptance number.
Not in this PR
kind: structural|directiveandfield:from the earlier overlay draft. That is the id→handler dispatch table's payload; it lands with the PR that actually builds the table, so the overlay never carries a field nothing reads.unsafeGetTag(HTTP_URL), which still asks by Datadog name).Testing
:dd-trace-core:test— 4293 tests, 3 failures, all pre-existing and environmental in this checkout: twoTracerConnectionReliabilityTestcases fail onCould not find a valid Docker environment(Testcontainers, no local Docker) andPendingTraceBufferTest.testingTracerFlareDumpWithMultipleTracesis the known timing flake.spotbugsMainandspotlessJavaCheckclean.New tests: the routing drift sweep, the pre-screen/flag agreement (including a custom split tag with no id), alternate-name routing, and the bundle screen.
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issue🤖 Generated with Claude Code