Expose isFinished() on AgentSpan#11884
Conversation
Add a non-breaking default boolean isFinished() to the AgentSpan interface (internal-api), surfacing the finished state DDSpan already tracks (durationNano != 0) so other modules can query it without reaching into dd-trace-core. DDSpan's existing method overrides the default; the TrackingSpanDecorator test wrapper delegates; leaf/synthetic spans (NoopSpan etc.) keep the false default. Adds a DDSpanTest case covering false-before / true-after-finish via the interface. Motivated by the FFE span-enrichment interceptor (PR #11658), which needs to distinguish a truly-finished local root from a still-running one (long-running traces) and resolve it in O(1); that consumer lands in a separate follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
🟢 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. |
There was a problem hiding this comment.
Pull request overview
Adds a non-breaking isFinished() default method to the AgentSpan interface so modules depending on internal-api can query span completion without reaching into dd-trace-core, while preserving existing behavior for DDSpan.
Changes:
- Introduces
default boolean isFinished()onAgentSpan(defaultfalse). - Adds a
DDSpanTestcase assertingisFinished()transitions fromfalsetotrueafterfinish(), via theAgentSpaninterface. - Updates
TrackingSpanDecorator(test wrapper) to delegateisFinished()to the wrapped span.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentSpan.java | Adds isFinished() as a default method on the core span interface. |
| dd-trace-core/src/test/java/datadog/trace/core/DDSpanTest.java | Adds coverage to validate AgentSpan.isFinished() reflects DDSpan completion. |
| dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/TrackingSpanDecorator.groovy | Delegates isFinished() in the AgentSpan test wrapper. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
🎯 Code Coverage (details) 🔗 Commit SHA: abd6fab | Docs | Datadog PR Page | Give us feedback! |
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM.
It would probably be worth getting one more approval from an experienced reviewer before merging.
What Does This Do
Adds a non-breaking
default boolean isFinished()to theAgentSpaninterface (internal-api), surfacing the finished state thatDDSpanalready tracks (durationNano != 0) so other modules can query it without reaching intodd-trace-core.DDSpan's existingisFinished()overrides the default — no behavior change.TrackingSpanDecoratortest wrapper (the onlyAgentSpanwrapper) delegates to its wrapped span.NoopSpan, etc.) keep thefalsedefault.DDSpanTestcase assertingfalsebefore finish /trueafter, exercised through theAgentSpaninterface.Motivation
The FFE span-enrichment interceptor (#11658) needs to tell a truly-finished local-root span from a still-running one — with long-running traces enabled, a periodic flush emits the still-running root, and the interceptor currently mis-reads "root present in the flushed batch" as "trace finished". It also wants to resolve the root in O(1) instead of scanning the batch. Both need a finished signal on the interface it uses (
AgentSpan).DDSpanalready has it, but the interface didn't expose it; this closes only that gap. The consumer change (the interceptor rewrite) lands as a separate follow-up on #11658.Additional Notes
defaultmethod, so no existingAgentSpanimplementer has to change. Making it abstract would break every implementer.DDSpanalready returns this value.MutableSpan/dd-trace-api(the FFE interceptor casts toAgentSpan).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/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)