Rework shim between ContextManager and the legacy scope manager#11938
Rework shim between ContextManager and the legacy scope manager#11938mcculls wants to merge 1 commit into
Conversation
mcculls
added
comp: core
This comment has been minimized.
This comment has been minimized.
🟢 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. |
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
from
7a1048e to
09eb365
Compare
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Reworks how the Datadog Context API’s ContextManager is wired when running with the legacy scope manager, so the legacy shim can be registered before instrumentation and can be toggled via isLegacyContextManagerEnabled. This helps support end-to-end testing across both context-manager implementations while keeping context requests routed through AgentTracer to the active tracer implementation.
Changes:
- Adds
Context-API bridging methods toAgentTracer.TracerAPIand introduces an installableLegacyContextManagershim. - Stops
ContinuableScopeManagerfrom self-registering as aContextManager; instead, registration happens conditionally inAgentandCoreTracer. - Updates benchmarks/tests accordingly and temporarily disables a few non-legacy-mode context swap tests that currently mix legacy
activeSpan()and the Context API.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal-api/src/main/java/datadog/trace/bootstrap/instrumentation/api/AgentTracer.java | Adds Context API bridging to TracerAPI and a legacy ContextManager shim + installer. |
| internal-api/build.gradle.kts | Excludes the new legacy shim class from coverage. |
| dd-trace-core/src/main/java/datadog/trace/core/scopemanager/ContinuableScopeManager.java | Removes ContextManager implementation/registration behavior from the legacy scope manager. |
| dd-trace-core/src/main/java/datadog/trace/core/CoreTracer.java | Conditionally installs the legacy context manager shim and exposes Context API methods via TracerAPI. |
| dd-trace-core/src/jmh/java/datadog/context/ContextManagerBenchmark.java | Updates benchmark to wrap ContinuableScopeManager now that it no longer implements ContextManager. |
| dd-trace-core/src/test/java/datadog/trace/core/scopemanager/ScopeManagerTest.java | Registers the test tracer with AgentTracer so Context API shimming can resolve to the correct tracer. |
| dd-trace-core/src/test/java/datadog/trace/core/PendingTraceTestBase.java | Same as above for pending-trace tests. |
| dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/Agent.java | Conditionally installs the legacy context manager shim during agent bootstrap. |
| dd-java-agent/instrumentation/kafka/kafka-clients-3.8/src/test/groovy/KafkaClientTestBase.groovy | Temporarily ignores a non-legacy context swap forked test. |
| dd-java-agent/instrumentation/kafka/kafka-clients-0.11/src/test/groovy/KafkaClientTestBase.groovy | Temporarily ignores a non-legacy context swap forked test. |
| dd-java-agent/instrumentation/jms/javax-jms-1.1/src/test/groovy/JMS1Test.groovy | Temporarily ignores a non-legacy context swap forked test. |
| dd-java-agent/instrumentation/aws-java/aws-java-sqs-2.0/src/test/java/SqsClientTest.java | Temporarily disables non-legacy context swap forked tests. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: 09eb3654f1
ℹ️ 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".
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
|
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
from
b04815d to
7209561
Compare
|
@codex review |
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
from
7209561 to
1f47531
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 720956194a
ℹ️ 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".
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
|
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
3 times, most recently
from
6025621 to
3e30c64
Compare
Debugger benchmarksParameters
See matching parameters
SummaryFound 0 performance improvements and 0 performance regressions! Performance is the same for 10 metrics, 5 unstable metrics. See unchanged results
Request duration reports for reportsgantt
title reports - request duration [CI 0.99] : candidate=None, baseline=None
dateFormat X
axisFormat %s
section baseline
noprobe (349.546 µs) : 325, 374
. : milestone, 350,
basic (307.938 µs) : 300, 316
. : milestone, 308,
loop (9.003 ms) : 8995, 9010
. : milestone, 9003,
section candidate
noprobe (361.135 µs) : 322, 400
. : milestone, 361,
basic (310.232 µs) : 300, 320
. : milestone, 310,
loop (9.011 ms) : 8990, 9032
. : milestone, 9011,
|
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
from
3e30c64 to
b2fe61d
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2fe61d8dc
ℹ️ 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".
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
2 times, most recently
from
7cdd09c to
43805c0
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43805c08e3
ℹ️ 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 (InstrumenterConfig.get().isLegacyContextManagerEnabled()) { | ||
| AgentTracer.installLegacyContextManager() | ||
| } |
There was a problem hiding this comment.
In the Spock instrumentation suite, once a legacy spec executes this branch, ContextProviders.customManager remains the legacy shim; DDSpecification.allowContextTesting() is cached and will not reset it for the next spec. A later spec that sets legacy.context-manager.enabled=false simply skips this block, so its Context API calls still route through the legacy scope manager and the intended non-legacy mode is not actually exercised. Reset the manager back to the test/default manager when this flag is false, or re-allow context testing before each spec.
Useful? React with 👍 / 👎.
…for proper swapping over to ThreadLocalContextManager.
mcculls
force-pushed
the
mcculls/rework-legacy-ctx-mgr-shim
branch
from
43805c0 to
2be7a5f
Compare
What Does This Do
Rework Context -> legacy scope manager shim so it can be registered before instrumentation. Context requests are dispatched via
AgentTracerto the currently registered tracer implementation. This helps migrate instrumentations to the Context API without requiring the tracer to be installed earlier than it is at the moment.We deliberately don't install the legacy shim for
dd-trace-ot/DDTracerbecause that artifact doesn't expose the Context API to its users, and none of the implementation needed for the OpenTracing API uses Context. Note that the legacy scope manager implementation deliberately avoids calling back into the Context API (exceptContext.root()but that's safe because it's a constant and does not result in a context/scope manager call.)Motivation
Supports clean switching between the two context managers for complete end-to-end testing.
Additional Notes
Uses the
isLegacyContextManagerEnabledfeature-flag to decide whether to register the legacy scope manager or leave the new context manager in place. (Previously the legacy scope manager was unconditionally installed by the tracer.)A few non-legacy tests are disabled because they mix both the old
activeSpan()call and the Context API. These tests passed previously because the tracer unconditionally installed the legacy context manager regardless of the feature-flag. They will be re-enabled once migration makes them consistent in non-legacy mode (they're already consistent in legacy mode.)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]