Conversation
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>
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>
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>
…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>
A dd-trace-core JMH benchmark covering the full create -> (tag) -> finish lifecycle, finished against a no-op DropWriter so -prof gc isolates create/tag/finish allocation from serialization. Pairs baseline shapes (web-server 7 tags, JDBC 9 tags; setTag and builder-withTag) with prototype arms: buildSpan(SpanPrototype).start() and the builder-free startSpan(SpanPrototype). Measured (Threads(8), -f3 -wi5 -i5 -prof gc): prototype construction cuts gc.alloc.rate.norm ~-5% web (-80 B/op) / ~-10% jdbc (-120 B/op) vs baseline -- tracking the number of baked constants (fewer per-span TagMap.Entry allocations). The builder-free startSpan is deterministic (no MultiSpanBuilder); buildSpan's builder is escape-analyzed away in this shallow micro, so startSpan is the EA-independent path for production's deeper/megamorphic call sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex review |
|
Codex Review: Didn't find any major issues. More of your lovely PRs please. Reviewed commit: ℹ️ About Codex in GitHubCodex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
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". |
This comment has been minimized.
This comment has been minimized.
Bits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 911fc57 · Any feedback? Reach out in #deveng-pr-agent |
🟢 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. |
…tion through it Introduce apply(SpanPrototype) as the single seam for stamping a prototype's constant initial state. It applies span type, constant tags, and integration name as fallback defaults -- only where the span has not already set them -- so it never clobbers explicit values, is order-independent, and self-neutralizes once construction has already seeded the same prototype. DDSpanContext.apply is the authoritative implementation (the context owns the tag map and will host the eventual bulk-share fast path + identity short-circuit); DDSpan.apply routes straight to it. The AgentSpan default is the best-effort fallback for non-core spans. The construction path (CoreSpanBuilder) now calls context.apply(prototype) instead of inlining the tag + integration-name seeding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
… off mocks Have BaseDecorator/ServerDecorator/ClientDecorator build a lazily-cached SpanPrototype (extension chain mirroring the decorator hierarchy) and apply it in afterStart via span.setSpanType/setAllTags/setIntegrationName, replacing the per-Entry setTag calls. Behavior-identical: setAllTags runs the same constant tags through the same interceptor path the per-tag calls used. Migrate the four afterStart specs from Spock mock-interaction assertions to a state-based harness (RecordingSpan/RecordingSpanContext accumulate applied state; ExpectedSpanState asserts the whole state at once), with three leniency modes matching Spock's polymorphic feature-method inheritance across the decorator hierarchy. Other specs (onPeerConnection/onConnection/onStatement/ beforeFinish) are unchanged. Also drop the born-dead SpanPrototype.Builder.initInstrumentationNames(String[]) overload (no caller); initInstrumentationName covers the single-name case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
284eae7 to
911fc57
Compare
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>
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>
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>
…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>
A dd-trace-core JMH benchmark covering the full create -> (tag) -> finish lifecycle, finished against a no-op DropWriter so -prof gc isolates create/tag/finish allocation from serialization. Pairs baseline shapes (web-server 7 tags, JDBC 9 tags; setTag and builder-withTag) with prototype arms: buildSpan(SpanPrototype).start() and the builder-free startSpan(SpanPrototype). Measured (Threads(8), -f3 -wi5 -i5 -prof gc): prototype construction cuts gc.alloc.rate.norm ~-5% web (-80 B/op) / ~-10% jdbc (-120 B/op) vs baseline -- tracking the number of baked constants (fewer per-span TagMap.Entry allocations). The builder-free startSpan is deterministic (no MultiSpanBuilder); buildSpan's builder is escape-analyzed away in this shallow micro, so startSpan is the EA-independent path for production's deeper/megamorphic call sites. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…tion through it Introduce apply(SpanPrototype) as the single seam for stamping a prototype's constant initial state. It applies span type, constant tags, and integration name as fallback defaults -- only where the span has not already set them -- so it never clobbers explicit values, is order-independent, and self-neutralizes once construction has already seeded the same prototype. DDSpanContext.apply is the authoritative implementation (the context owns the tag map and will host the eventual bulk-share fast path + identity short-circuit); DDSpan.apply routes straight to it. The AgentSpan default is the best-effort fallback for non-core spans. The construction path (CoreSpanBuilder) now calls context.apply(prototype) instead of inlining the tag + integration-name seeding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Add getIntegrationName() (default null) to AgentSpanContext, symmetric with the existing no-op setIntegrationName, so the default apply() can honor never-clobber like DDSpanContext.apply instead of unconditionally overwriting. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Mirrors the existing buildSpan(String,...) noop contract so the prototype builder path matches. startSpan(SpanPrototype) was already noop-safe. A chainable NoopSpanBuilder to fix the null-vs-NoopSpan asymmetry is left to a separate PR. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Cover every Builder branch and getter so the SpanPrototype.Builder jacoco rule (branch >= 0.7, instr >= 0.8) that failed test_base is satisfied: initInstrumentationNames null/empty/multi, extends_(null) + full copy, initComponentAndIntegration set/empty, initTag(Object)/initTag(EntryReader) null and non-null, and all five getters incl. integrationName(). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
eadba23 to
c2d5fb9
Compare
# Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java
…HEAD # Conflicts: # dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/BaseDecorator.java # dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/ClientDecorator.java # dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/instrumentation/decorator/ServerDecorator.java # dd-java-agent/agent-bootstrap/src/test/groovy/datadog/trace/bootstrap/instrumentation/decorator/BaseDecoratorTest.groovy # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java # internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java # internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/SpanPrototype.java # internal-api/src/test/java/datadog/trace/bootstrap/instrumentation/api/SpanPrototypeTest.java
No caller uses this generic setter -- tag population goes through the more specific component()/spanKind()/language() builder methods.
…se-prototype # Conflicts: # dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java
There was a problem hiding this comment.
Global span tags can now keep a conflicting span.kind or component value when a decorator starts a span. This can give client and server spans the wrong identity.
🤖 Datadog Autotest · Commit 2d899a8 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| * then any specialization). {@link #assertAppliedTo(RecordingSpan)} verifies the whole accumulated | ||
| * state at once instead of asserting individual mock interactions. | ||
| */ | ||
| final class ExpectedSpanState { |
There was a problem hiding this comment.
Not sure how I feel about this testing method. I'm open to other ideas. Do we want a similar fluent-API to what was just introduced for smoke tests?
There was a problem hiding this comment.
Having a similar SpanMatcher would help consistency
There was a problem hiding this comment.
Yeah, I wasn't necessarily planning on making a new assertion mechanism in this PR. I just didn't want to keep adding to the Groovy tests either.
There was a problem hiding this comment.
Went with expectedSpan() (skipping the "Of") and gave it a SpanMatcher-style factory/Javadoc — statically imported so call sites now read expectedSpan().spanType(...).component(...)..., matching span().service(...).tag(...). Kept the assert methods and RecordingSpan-specific fields as-is since they check mock-call state rather than wire fields, so a full SpanMatcher port didn't fit. Pushed in a966163.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2d899a8b43
ℹ️ 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".
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
BaseDecorator.afterStart was reusing AgentSpan#apply's fill-absent semantics, so a global tag from DD_TAGS/DD_TRACE_SPAN_TAGS on a decorator-owned key (component, span.kind, span.type) could silently suppress the decorator's own value, since the global tag is seeded onto the span before afterStart runs. Add applyOverwriting, an unconditional variant matching the old setTag/setSpanType behavior, and point the decorator seam at it, keeping apply's fill-absent semantics for the construction seam. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
When split-by-tags has both component and language, server spans now use java as the service name. The old code uses the decorator component because it handles component last.
🤖 Datadog Autotest · Commit 7de9ad5 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| if (spanType != null) { | ||
| setSpanType(spanType); | ||
| } | ||
| setAllTags(prototype.tags(), true); |
There was a problem hiding this comment.
Keep server tag processing order
Affected server spans use the wrong service name and split APM data into the java service.
Assertion details
- Input: A server span when trace.split-by-tags contains both component and language.
- Expected:
Keep the old order. Handle language first and component last, so the decorator component sets the final service name. - Actual:
Bulk tag processing handles component before language. The tag interceptor then leaves the service name as java.
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest · Open Bits AI session
There was a problem hiding this comment.
This looks like a legitimate concern
There was a problem hiding this comment.
I'm a bit torn about this issue. I tend to think this falls into undefined behavior where the precedence rules are really accidental and untested.
However for the sake of progress, I'm willing to tolerate some ugly code and maintain compatibility whereever I reasonably can. I do think there may come a point where that won't be possible, but time will tell I suppose.
| // The base spec runs polymorphically against every subclass decorator, so it only asserts the | ||
| // baseline identity every decorator applies, tolerating the tags subclasses layer on. Each | ||
| // level's exact tag set is asserted by its own afterStart spec. | ||
| ExpectedSpanState.expected() |
There was a problem hiding this comment.
A statically imported expectedSpanOf()... method or similar might read better?
There was a problem hiding this comment.
Okay, we can do that.
BaseDecorator now applies component/language/span.kind together via a single SpanPrototype, so which one wins the service name for trace.split-by-tags depended on TagMap iteration order instead of the fixed precedence the old sequential setTag calls guaranteed (component last, so component always won). Add SplitByTagsPriorities and gate DDSpanContext's split-by-tags service name updates on it, so component beats language regardless of application order. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Rename ExpectedSpanState.expected() to expectedSpan() and switch call sites to a static import, so usage reads like the smoke tests' SpanMatcher.span()...tag(...) builder instead of a qualified factory call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
What Does This Do
Wires the
SpanPrototypefrom #11894 into the decorator base classes, and modernizes the affected tests.Production —
BaseDecorator/ServerDecorator/ClientDecoratornow build a lazily-cachedSpanPrototypewhose extension chain mirrors the decorator hierarchy (super.buildSpanPrototype()→.extends_(...)→ add this level's constants).afterStartapplies it viaspan.setSpanType/span.setAllTags(prototype.tags())/spanContext().setIntegrationName(...), replacing the previous N separatespan.setTag(TagMap.Entry)calls.This is behavior-identical:
setAllTagsruns the same constant tags through the same tag-interceptor path the per-tag calls used today; it's a consolidation, not a new code path. Bulk-share (skipping per-tag interception) is deliberately deferred to the dense-store / tag-registry work.Tests — the four
afterStartspecs move off Spock mock-interaction assertions to a state-based harness:RecordingSpan/RecordingSpanContextaccumulate the applied state (extends the no-opImmutableSpan, so only the ~7 mutatorsafterStarttouches are overridden).ExpectedSpanStatebuilds the expected state per level and asserts it in one shot, with three leniency modes (identity / exact / allow-extra-tags) matching Spock's polymorphic feature-method inheritance down the decorator hierarchy.Other specs (
onPeerConnection/onConnection/onStatement/beforeFinish) are unchanged and still use mocks.Motivation
Replace
BaseDecorator.afterStart's per-tagsetTagstamping with a single baked-onceSpanPrototypeapplied via a fast bulk copy, and move theafterStartspecs off brittle mock-interaction assertions onto a state-based harness that survives the consolidation.Additional Notes
Stacked on #11894 (
dougqh/span-prototype-api). Review/merge that first; this PR's base retargets tomasteronce #11894 lands.Drops the born-dead
SpanPrototype.Builder.initInstrumentationNames(String[])overload (no caller;initInstrumentationNamecovers the single-name case) — shows as a 1-line deletion against the #11894 base.Test plan:
:dd-java-agent:agent-bootstrap:test— green (afterStart specs exercise the newsetAllTagspath):dd-java-agent:agent-bootstrap:spotbugsMain,spotlessJavaCheck— greenafterStartis a consolidation of existing per-tag work)🤖 Generated with Claude Code