Deprecate AgentScope.Continuation and migrate call sites to ContextContinuation#11951
Deprecate AgentScope.Continuation and migrate call sites to ContextContinuation#11951mcculls wants to merge 5 commits into
Conversation
mcculls
added
tag: no release notes
dd-octo-sts
Bot
added
the
tag: ai generated
|
🎯 Code Coverage (details) 🔗 Commit SHA: 26e6e31 | Docs | Datadog PR Page | Give us feedback! |
![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: 4041cbed27
ℹ️ 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".
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The migration is behaviorally faithful: resume()/release() in ScopeContinuation are the new primary implementations with activate()/cancel() delegating to them, the (AgentScope) resume() cast is safe (both return paths give ContinuableScope or NoopScope), and ContinuableScopeManager's explicit installLegacyContextManager() call is correctly wired in Agent bootstrap, native-image activation, and all test bases. Two SQS test classes covering the non-legacy path are intentionally @Disabled pending the broader migration (APMLP-829).
📊 Validated against 23 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 4041cbe · What is Autotest? · Any feedback? Reach out in #autotest
🟢 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. |
Kafka / consumer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
09dab12 to
ed5b5ef
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Kafka / producer-benchmarkParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 3 metrics, 0 unstable metrics. See unchanged results
|
55ebf56 to
f26db87
Compare
…ntinuation AgentScope.Continuation was a bridging interface that extended both TraceScope.Continuation (public API) and ContextContinuation (modern context API). It is now @deprecated — internal call sites migrate to ContextContinuation directly. Key changes: - AgentScope.Continuation marked @deprecated - AgentTraceCollector.register/removeContinuation now accept ContextContinuation - AgentTracer.captureActiveSpan()/captureSpan() static methods return ContextContinuation (implementations still return AgentScope.Continuation for Tracer interface compat) - ScopeContinuation gains resume()/release() for the ContextContinuation contract - State, ConcurrentState, Wrapper, VirtualThreadState, AdviceUtils, TPEHelper all migrated from AgentScope.Continuation to ContextContinuation/ContextScope - ~85 instrumentation files updated: field types use ContextContinuation, activate()→resume(), cancel()→release(), scope types use ContextScope Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
f26db87 to
7ddef31
Compare
…gent-scope-continuation-1-migrate-call-sites
PerfectSlayer
left a comment
PerfectSlayer
left a comment
There was a problem hiding this comment.
Left minor questions about possible behavior change (just need confirmation) and about some null-checks - if we need to start adding some even if they should not throw for now. This can be in follow up PR if ever needed.
| } | ||
| RedissonClientDecorator.DECORATE.beforeFinish(scope); | ||
| scope.span().finish(); | ||
| AgentSpan.fromContext(scope.context()).finish(); |
There was a problem hiding this comment.
💭 thought: Should we handle null span here?
There was a problem hiding this comment.
See #11951 (comment) - we can avoid null checks when we are sure that the captured continuation's context contained a span.
| } | ||
| RedissonClientDecorator.DECORATE.beforeFinish(scope); | ||
| scope.span().finish(); | ||
| AgentSpan.fromContext(scope.context()).finish(); |
There was a problem hiding this comment.
❔ question: Same question about null span here too
There was a problem hiding this comment.
See #11951 (comment) - we can avoid null checks when we are sure that the captured continuation's context contained a span.
| } | ||
| } | ||
| return null; | ||
| return null != continuation ? continuation.resume() : null; |
There was a problem hiding this comment.
💭 thought: Is this a behavior change? It only resumes continuation without adding the current context to it.
There was a problem hiding this comment.
simplification - look at what the old code was doing:
if (null != continuation) {
AgentScope agentScope = continuation.activate();
try {
return agentScope.span().attachWithContext();
} finally {
agentScope.close();
}
}
First it activated the continuation - this attached the span from the continuation without merging in the surrounding context (which is correct, you don't want to pick up context that might be hanging around when activating the continuation at this point.)
Then it attached the span from the current scope - which will be same as the continuation's span / context because we just activated it. When attaching it merged in the context from the current active scope, but this will just be the same span again because activating a continuation does not merge it with the surrounding context.
TLDR: it really just needed to activate (or resume) the continuation's context - bonus fix is that we now restore the full context and not just the span.
| ContextContinuation continuation = xchg.getAttachment(DATADOG_UNDERTOW_CONTINUATION); | ||
| if (continuation != null) { | ||
| continuation.span().getRequestContext().getTraceSegment().effectivelyBlocked(); | ||
| AgentSpan.fromContext(continuation.context()) |
There was a problem hiding this comment.
❔ question: Should we have null check here too? It's not supposed to be null but I wonder if it could bite us later 🤔
There was a problem hiding this comment.
As we move away from AgentScope this question will come up. Basically I don't want to litter the codebase with null-checks.
For this specific PR we know the context captured in the continuation was a span (or included a span) so we know this will not be null on the other side. For this kind of situation - where we know what we put in and therefore know what we'll get out, we can avoid the check. Similarly for enter/exit advice where we know the scope passed to the exit advice has the same context as the enter advice, and we know that had a span.
In a separate PR I'm looking at introducing an "invalid" span like OTel to avoid widespread null-checks, but need to see how that plays out.
…gent-scope-continuation-1-migrate-call-sites
3 participants
What Does This Do
AgentScope.Continuationas@Deprecated— it was a bridging interface extending bothTraceScope.Continuation(public API) andContextContinuation(modern context API).AgentTraceCollector.register/removeContinuationnow acceptContextContinuation.AgentTracer.captureActiveSpan()/captureSpan()static methods returnContextContinuation(implementations still returnAgentScope.Continuationfor Tracer interface compat).ScopeContinuationgainsresume()/release()to satisfy theContextContinuationcontract.State,ConcurrentState,Wrapper,VirtualThreadState,AdviceUtils,TPEHelpermigrated fromAgentScope.ContinuationtoContextContinuation/ContextScope.ContextContinuation,activate()→resume(),Motivation
AgentScope.Continuationmixes a legacy public-API contract with the modern context API, forcing internal code to depend on a bridging type it doesn't need. Moving internal call sites ontoContextContinuationdirectly clears the path to eventually removing the legacy interface, without changing behavior.Additional Notes
This is a mechanical migration with no intended behavior change —
AgentScope.Continuationremains available (deprecated) for public API / Tracer interface compatibility. No release note required.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 issueJira ticket: [PROJ-IDENT]