Introduce SpanPrototype: baked-once constant span-tag descriptor (phase 1b)#11894
Introduce SpanPrototype: baked-once constant span-tag descriptor (phase 1b)#11894dougqh wants to merge 4 commits into
Conversation
dougqh
added
comp: core
|
🎯 Code Coverage (details) 🔗 Commit SHA: 9521907 | Docs | Datadog PR Page | 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. |
Design note: SpanPrototype × TagInterceptor (resolve before baking is wired into construction, #11834)Several tags a prototype naturally bakes as constants are exactly the keys So there's a fork once prototypes write baked tags directly to storage:
Proposed resolution (fits "baked once"): the interceptor's inputs are constant for a given prototype, so its outputs are too — bake the post-interception result at prototype-construction time (resolve the effect once: set the Ties into the TagInterceptor-retirement direction (absorb interceptor logic into the map as inlinable data) — baking the resolved effect is a step in that same direction. |
The builder API (extends_/init*) plus its per-mechanism microbenchmark and a pure-API test, split out from the combined span-prototype work so the abstraction lands independently of the decorator demo. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
3af8e13 to
41666f5
Compare
dougqh
changed the title
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
SpanPrototype is a well-designed immutable descriptor for constant span tags. The builder correctly handles null values, supports inheritance via extends_(), and freezes tags at construction. No behavioral regressions detected from synthetic scenarios covering identity fields, tag accumulation, inheritance chains, and edge cases.
📊 Validated against 8 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 41666f5 · What is Autotest? · Any feedback? Reach out in #autotest
![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: 41666f5648
ℹ️ 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".
| if (value != null) { | ||
| this.tags.set(key, value); |
There was a problem hiding this comment.
When a decorator constant is an empty String/CharSequence, this path stores an actual empty tag in the prototype. The existing per-span stamping paths either go through AgentSpan.setTag(String|CharSequence), which removes empty values, or cached TagMap.Entry.create(...), whose null-tolerance contract treats empty values as "no tag"; TagMap.set(key, value) does not apply that filtering. A prototype built with something like initComponent("") or initKind("") would therefore emit a tag that is absent today, so the builder should use the null-tolerant Entry.create/set(EntryReader) path for these constants.
Useful? React with 👍 / 👎.
| public Builder initKind(final CharSequence kind) { | ||
| return initTag(Tags.SPAN_KIND, kind); |
There was a problem hiding this comment.
When construction uses the advertised fast prototype.tags() copy, baking span.kind as just another raw tag misses the side effect that existing AgentSpan.setTag(TagMap.EntryReader) performs through TagInterceptor: it updates DDSpanContext's cached span-kind ordinal. DDSpan.isOutbound() and isKind() read that ordinal rather than falling back to the tag map, so client/producer prototypes would serialize span.kind but still behave as if the kind is unset unless the prototype carries this side effect separately or requires intercepted application for this tag.
Useful? React with 👍 / 👎.
| public Builder initComponent(final CharSequence component) { | ||
| return initTag(Tags.COMPONENT, component); |
There was a problem hiding this comment.
When a prototype is used to replace BaseDecorator.afterStart, recording component only as a tag loses the existing span.spanContext().setIntegrationName(component) side effect. That context field is what IntegrationAdder later serializes as _dd.integration; copying prototype.tags() would still include component but would omit _dd.integration unless the prototype stores the integration name separately or construction explicitly derives and sets it from this component value.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8e1b32a9dd
ℹ️ 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".
| default AgentSpan startSpan(SpanPrototype prototype, CharSequence operationName) { | ||
| return buildSpan(prototype, operationName).start(); |
There was a problem hiding this comment.
When no real tracer has registered (or tests force NOOP_TRACER), this default method is inherited by NoopTracerAPI; its buildSpan(String, ...) implementation returns null, so AgentTracer.get().startSpan(prototype, ...) throws a NullPointerException instead of returning NoopSpan like the existing startSpan overloads. Any instrumentation adopting this entry point can crash before/without tracer registration; please override this in NoopTracerAPI or make the default tolerate a null/no-op builder.
Useful? React with 👍 / 👎.
| // interception for non-intercepted tags) is deferred to the dense-store / tag-registry | ||
| // work, | ||
| // which will expose intercept status at the internal-api level. | ||
| context.setAllTags(spanPrototype.tags()); |
There was a problem hiding this comment.
When local-root, contextual, or extracted tags contain a key baked into the prototype (for example span.kind or component), this applies the prototype before those later construction tags are applied, so those tags can overwrite the decorator constants and the cached span-kind ordinal. BaseDecorator.afterStart currently stamps these constants after span construction, so moving decorators to prototypes changes spans under those tag configurations; seed the prototype after the inherited/root/contextual tags while still letting explicit builder tags win.
Useful? React with 👍 / 👎.
| public Builder instrumentationName(final String[] instrumentationNames) { | ||
| return (instrumentationNames == null || instrumentationNames.length == 0) | ||
| ? this | ||
| : instrumentationName(instrumentationNames[0]); |
There was a problem hiding this comment.
When this helper is used with an existing decorator whose instrumentationNames() returns an empty array, it silently leaves instrumentationName unset. Starting a span from that prototype then passes null into DDSpan/SpanMetricRegistry.computeIfAbsent, which throws at runtime; please fail during prototype construction or require an explicit fallback name instead of building a prototype that cannot start spans.
Useful? React with 👍 / 👎.
| final CharSequence integrationName = spanPrototype.integrationName(); | ||
| if (integrationName != null) { | ||
| context.setIntegrationName(integrationName); |
There was a problem hiding this comment.
When a prototype uses initComponentAndIntegration(...) and the caller explicitly overrides the component tag with builder tags, this stores the prototype integration name before the later tagLedger overwrite runs, so serialization can emit component=override but _dd.integration=<prototype component>. Since the new construction contract says explicit builder tags override prototype constants, the integration-name side effect should be updated after those overrides or otherwise kept in sync with the final component value.
Useful? React with 👍 / 👎.
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 9521907 · Any feedback? Reach out in #deveng-pr-agent |
A prototype constant that is null or an empty CharSequence should be "no tag" -- matching AgentSpan.setTag and the decorators' cached-Entry path -- not a baked empty tag. Add TagMap.Entry.isEmptyValue as the single definition of an empty value (both Entry.create overloads now delegate to it), and gate SpanPrototype.Builder.initTag on it via the plain set(key, value) path so no Entry is allocated (the wrong path once tags are stored densely). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8e1b32a to
45f121f
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 45f121f777
ℹ️ 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".
| * {@link #buildSpan(SpanPrototype, CharSequence)}. | ||
| */ | ||
| default AgentSpan startSpan(@Nonnull SpanPrototype prototype, CharSequence operationName) { | ||
| return buildSpan(prototype, operationName).start(); |
There was a problem hiding this comment.
When the active provider is NOOP_TRACER (for example before a real tracer is installed or after it has been reset), this default method dispatches to NoopTracerAPI.buildSpan(...), which currently returns null, and then immediately calls .start(). That makes the new prototype start path throw an NPE instead of matching the existing startSpan(String, ...) overloads' no-op behavior; please override this in the no-op tracer or make the default tolerate a null builder.
Useful? React with 👍 / 👎.
| @Benchmark | ||
| public TagMap newBulkApply() { | ||
| TagMap tags = TagMap.create(); | ||
| tags.putAll(prototype.tags()); |
There was a problem hiding this comment.
This benchmark copies prototype tags with a raw TagMap.putAll, but the committed construction path in CoreTracer applies context.setAllTags(spanPrototype.tags()), which runs every entry through TagInterceptor so span.kind, analytics sample rate, and other intercepted constants get their side effects. As written, newBulkApply can report a much cheaper path than spans actually pay when using buildSpan(prototype, ...); please add a benchmark for the intercepted DDSpanContext.setAllTags path or make this explicitly future-only so the results are not used as evidence for the current implementation.
Useful? React with 👍 / 👎.
| // Seed the frozen constant tags through the interceptor. A cheaper bulk-share path that | ||
| // skips interception for non-intercepted tags is deferred to the dense-store / tag-registry | ||
| // work, which will expose intercept status at the internal-api level. | ||
| context.setAllTags(spanPrototype.tags()); |
There was a problem hiding this comment.
When a prototype replaces BaseDecorator.afterStart, applying its tags here changes precedence because coreTags, rootSpanTags, and contextual tags are still applied afterward. In configurations where an extracted/header/root tag uses a reserved key like span.kind or component, that later tag can overwrite the decorator constant and even update the cached span-kind ordinal after the prototype set it; the current afterStart flow writes those constants after span construction, so they win. Please seed prototype constants after the construction-time tag maps, while preserving the intended explicit builder override behavior.
Useful? React with 👍 / 👎.
| CoreSpanBuilder builder = | ||
| createMultiSpanBuilder(prototype.instrumentationName(), operationName); |
There was a problem hiding this comment.
perf: The inherited startSpan(SpanPrototype, ...) routes through this buildSpan method, so each prototype-based auto-instrumented span allocates a MultiSpanBuilder before starting. Existing CoreTracer.startSpan(String, ...) avoids that by calling CoreSpanBuilder.startSpan directly; if follow-up integrations use the new prototype start path, this adds one allocation per span. Please override startSpan(SpanPrototype, ...) in CoreTracer to use the direct construction path.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
45f121f to
2143299
Compare
|
@codex review |
Thread a SpanPrototype through span construction: AgentTracer gains buildSpan/startSpan(SpanPrototype, operationName) (defaults seed identity only, correct for the noop tracer, with an explicit NoopTracerAPI.startSpan override). CoreTracer overrides buildSpan to seed the prototype's frozen constant tags in buildSpanContext at the precedence slot just before the builder's own tags (prototype and builder form one precedence atom; explicit builder tags win), and overrides startSpan to seed builder-free via the static CoreSpanBuilder.startSpan path (no MultiSpanBuilder allocation, mirroring startSpan(String,...)). Explicit operationName wins; null falls back to the prototype's. Intercepted constants (e.g. span.kind) seed through the interceptor so their context side-effects still fire. Prototype params @nonnull. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
2143299 to
4276da0
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4276da0399
ℹ️ 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".
| default SpanBuilder buildSpan(@Nonnull SpanPrototype prototype, CharSequence operationName) { | ||
| return buildSpan( | ||
| prototype.instrumentationName(), | ||
| operationName != null ? operationName : prototype.operationName()); |
There was a problem hiding this comment.
When the registered tracer is still NOOP_TRACER (for example during early instrumentation or tests before tracer registration), this new default is inherited by NoopTracerAPI and delegates to buildSpan(String, ...), which returns null. The new startSpan(prototype, ...) path was explicitly overridden to avoid this NPE, but callers using the advertised builder path (AgentTracer.get().buildSpan(prototype, op).withTag(...).start()) still fail immediately instead of behaving as a no-op; add a noop SpanBuilder/override for this overload as well.
Useful? React with 👍 / 👎.
…formly init* BaseDecorator.afterStart sets the integration name as a side effect alongside the component tag (setIntegrationName(component)), which IntegrationAdder later serializes as _dd.integration. A prototype baking only the component tag would drop that. Add initComponentAndIntegration(component): sets the component tag AND records it as the integration name (inherited via extends_), applied via setIntegrationName at construction. Rename the builder setters to a uniform init* surface now that a component sibling exists and to convey "everything here bakes the prototype's initial state": initComponent -> initComponentOnly, instrumentationName -> initInstrumentationName(s), operationName -> initOperationName, spanType -> initSpanType. Accessors are unchanged. Renames are confined to SpanPrototype.Builder and its callers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
4276da0 to
9521907
Compare
1 participant
What Does This Do
Introduces
SpanPrototype— a baked-once, immutable descriptor of a span's constant identity + tags (builder API:extends_/init*) — and the construction path that builds a span from one:buildSpan(SpanPrototype, operationName)/startSpan(SpanPrototype, operationName).A decorator declares its constant span shape once (identity +
span.kind,component, …); construction seeds those constants with a fast TagMap-to-TagMap copy that reuses the underlyingTagMap.Entryobjects, instead ofBaseDecorator.afterStartstamping each tag per span.The side effects
afterStartperforms alongside tag writes are preserved, not just the tags:span.kindseeds through theTagInterceptor, so the cached span-kind ordinal (which drivesisOutbound/isKind) is set — not merely the tag.componentviainitComponentAndIntegrationalso records the integration name (setIntegrationName), which theIntegrationAdderpost-processor serializes as_dd.integration. PlaininitComponentstays tag-only.AgentSpan.setTag.This replaces the per-tag approach used today in
BaseDecorator/ TagProcessors, which requires working at the TagMap level rather than the span level and a cachedTagMap.Entryfield per tag.Motivation
The constant tags a span carries are applied today via per-
afterStartsetTagcalls.SpanPrototypesets up those constants once so a span can be constructed with them with fast, light copying.Additional Notes
Part of the #11828 split. The decorator demo that bakes prototypes in real integrations is the stacked follow-up, #11895.
Commits:
TagMap.Entry.isEmptyValuebuildSpan/startSpan(SpanPrototype))component→_dd.integration)SpanPrototype.NONEis intentionally kept: the no-prototype sentinel (CoreTracerdefault field +!= NONEguard), not dead code.🤖 Generated with Claude Code