Repository navigation
[Debugger] DEBUG-4637 Fix DI remote enablement - #9344
Merged
Merged
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
dudikeleti
requested
a balanced review from Copilot
and removed request for
a team
October 1, 2026 05:57
Contributor
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The enablement precedence is correctly implemented and covered by focused unit and integration tests.
Review effort: Balanced
Findings: None
What changed in this PR
Fixes Dynamic Instrumentation startup when enabled through remote configuration while preserving explicit disablement precedence.
Changes:
- Computes effective DI enablement from local and remote settings.
- Expands unit coverage across enablement combinations.
- Makes integration tests wait for completed initialization before snapshots.
| File | Description |
|---|---|
tracer/src/Datadog.Trace/Debugger/DynamicInstrumentation.cs |
Applies effective local/remote enablement during initialization. |
tracer/test/Datadog.Trace.Tests/Debugger/DynamicInstrumentationTests.cs |
Adds matrix-based enablement tests. |
tracer/test/Datadog.Trace.Debugger.IntegrationTests/DebuggerManagerDynamicTests.cs |
Waits for expected lifecycle logs before final assertions. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
BenchmarksBenchmark execution time: 2026-10-01 11:10:24 Comparing candidate commit 40ba240 in PR branch Found 0 performance improvements and 11 performance regressions! Performance is the same for 61 metrics, 0 unstable metrics, 71 known flaky benchmarks, 52 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9344) and master. ✅ No regressions detected |
dudikeleti
added a commit
to DataDog/system-tests
that referenced
this pull request
Oct 1, 2026
DEBUG-4637 is fixed in dd-trace-dotnet 3.55.0 (DataDog/dd-trace-dotnet#9344). .NET has code origin on by default since 3.45.0, so the code origin test now expects it enabled at startup, as for Node.js.
dudikeleti
requested review from
zacharycmontoya
and removed request for
a team
October 1, 2026 08:08
…nges Remote config only delivers new or changed files, so clearing the active set on every update dropped configs that are still listed in client_configs. Removals are already reported explicitly, so rely on those.
dudikeleti
requested
a balanced review from Copilot
and removed request for
zacharycmontoya
October 1, 2026 10:24
jpbempel
approved these changes
Oct 2, 2026
dudikeleti
enabled auto-merge (squash)
October 15, 2026 23:31
Member
|
@DataDog review |
lucaspimentel
approved these changes
Oct 20, 2026
![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
DI startup now follows DebuggerManager’s existing remote/environment precedence. Retaining unchanged APM_TRACING configs is consistent with explicit removal handling, and the completed static review identified no reportable defects.
🤖 Bits Code Review · Commit 40ba240
tylfin
approved these changes
Oct 20, 2026
Contributor
|
Please make sure that probes are removed when DI is disabled remotely. When investigating something across tracers, my clunkers claim that they wouldn't be. |
Contributor
Author
This is a known gap that was intentionally left out of scope, as complete probe removal requires additional work and thorough testing. For now, remote disable stops probe processing without fully reverting the instrumentation. Native removal involves waiting for Revert/ReJIT to complete and requires careful handling of concurrency, locking, process shutdown, rapid disable/re-enable, and preserving Exception Replay probes on shared methods. I'll address this in a separate PR with dedicated lifecycle tests. |
zacharycmontoya
approved these changes
Oct 22, 2026
1 task
dudikeleti
added a commit
to DataDog/system-tests
that referenced
this pull request
Oct 27, 2026
DataDog/dd-trace-dotnet#9344 merged after 3.56.0 was released, so the first release with the fix is 3.57.0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dudikeleti
added a commit
to DataDog/system-tests
that referenced
this pull request
Oct 27, 2026
DataDog/dd-trace-dotnet#9344 merged after 3.56.0 was released, so the first release with the fix is 3.57.0. Code origin enablement doesn't depend on #9344 and is enabled from 3.56.0 in #8123, so this PR no longer touches it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dudikeleti
added a commit
to DataDog/system-tests
that referenced
this pull request
Oct 27, 2026
…as a bug The start_time filter only read the top-level timestamp of a snapshot payload. .NET only sets it in the snapshot itself, so the filter never applied, and the apm_multiconfig setup matched the simple exception snapshot captured by the previous test while exception replay was off. Fall back to the snapshot timestamp. Test_Debugger_InProduct_Enablement_Dynamic_Instrumentation stays bug (DEBUG-4637): with DataDog/dd-trace-dotnet#9344 in the dev build, remote enablement installs the probe, but it never emits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dudikeleti
added a commit
to DataDog/system-tests
that referenced
this pull request
Oct 27, 2026
…from 3.57.0 ASP.NET Core MVC calls actions through a compiled expression tree, and the JIT inlines the 12-byte LogProbe into it on the first /debugger/log request. The in-product DI test calls /debugger/log while DI is off, so after remote enablement the tracer ReJITs LogProbe but requests keep running the inlined copy, and the line probe never emits. Scenarios that enable DI at startup install the probe before the first call. Mark LogProbe NoInlining on its existing attribute line so no line number in the file moves, and declare the DI test bug (DEBUG-4637) only below 3.57.0, the first release with DataDog/dd-trace-dotnet#9344. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 29, 2026
dudikeleti
added a commit
that referenced
this pull request
Oct 30, 2026
…otely (#9560) ## Summary of changes - When Dynamic Instrumentation (DI) is turned off at runtime by remote configuration (APM_TRACING `dynamic_instrumentation_enabled: false`, or the config that enabled it is removed), the installed probes are now removed and the instrumented methods go back to their original code. - When DI is turned on again, the probes are installed again and keep working. - Process shutdown still only disposes DI, without reverting. ## Reason for change - Follow-up to the review comment on #9344: "Please make sure that probes are removed when DI is disabled remotely". - Disabling DI only disposed the `DynamicInstrumentation` instance and never asked the native tracer to revert the instrumented methods. While DI was off, span probes kept creating `dd.dynamic.span` spans, span decoration probes kept adding tags, and log, metric and snapshot probes kept capturing on every hit and then dropping the result. - Java, Python, PHP and Ruby already uninstall all probes when DI is disabled remotely. ## Implementation details - New `DynamicInstrumentation.RemoveProbesAndDispose()` reverts the native instrumentation of every probe with a native representation, then disposes the instance. It holds `_instanceLock` until disposal, so a concurrent probe update can't instrument a method after the revert. - `DebuggerManager.DisableDynamicInstrumentation` calls it for every runtime disable, immediate or debounced. If process exit has already started, it only disposes. - Unlike deleting a probe in the UI, disabling DI keeps the probe processors and samplers registered. `ProbeDataCollection` caches them by native probe index and probe ID, and the native side usually gives a probe that is added again the same index. Removing them left re-enabled log and snapshot probes with a removed processor and a disposed sampler that never refills its budget, so they stopped capturing. Kept, they are updated in place when the probes are added again. A TODO in the code tracks clearing those cache entries on removal instead. - No new lock order: `Dispose` already ran under `_syncLock`, and hot paths don't take these locks. ## Test coverage - Unit (`DynamicInstrumentationTests.ProbeRemovalOnDisableTests`): - a remote disable reverts every installed probe (remote `false`, config removed, env `true` with remote `false`); - after a disable and re-enable, the cached `ProbeData` uses the registered processor and sampler; - later probe updates are ignored; shutdown, and a disable after process exit started, don't revert. - Integration (`ProbesTests.SpanProbeIsRemovedWhenDynamicInstrumentationIsDisabledRemotely`, DI enabled by env var or remotely): a span probe creates a span, DI is disabled remotely, then re-enabled; exactly two probe spans must arrive across the three runs. - Before the fixes, the remote-disable unit tests and the integration test failed; the cached `ProbeData` test failed on f28d359. - With c9122ee, on Windows x64, all pass: the Debugger and dynamic configuration unit tests on net10.0 (960 passed) and net48 (950 passed), and the integration tests on net8.0 (new test, `LineProbesWithRevertTest`, `ConditionAndTemplateChangeTest`) and net48 (new test, `DebuggerManagerDynamicTests`, `LineProbesWithRevertTest`). - Not run on Linux locally. ## Other details - System test: DataDog/system-tests#8229 (draft) checks that probes are removed when DI is disabled remotely. The existing in-product DI test only checks that probes stop emitting, which passes in .NET even with this bug. <!-- Fixes #{issue} -->
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary of changes
DD_DYNAMIC_INSTRUMENTATION_ENABLEDis unset.falsefrom either the env var or remote configuration keeps DI off.CombineApmTracingConfigurationtests.DebuggerManagerDynamicTestsnow waits for DI to finish initializing, not just for the instance to be constructed, and waits for log entries before the final memory snapshot.Reason for change
DebuggerManagercreated theDynamicInstrumentationinstance, recorded it as initialized and reported DI as enabled in telemetry. However,Initialize()only checked the env var, so the instance never subscribed toLIVE_DEBUGGINGand never started its uploaders or probe status poller. Probes created in the UI were never applied. Because the manager still counted that instance as started, it ignored later enable requests until restart, even after the config was deleted and added again. The integration test missed this because it waited for "Initializing Dynamic Instrumentation", which the constructor logs even if DI never starts.ApmTracingConfigMergerhad its own copy of the field merge without the three debugger fields, so merging two or more configs returned them as null. DI is off unless a config saystrue, so it turned off.client_configsbut not resent.DynamicConfigurationManager.CombineApmTracingConfigurationcleared all active configs on every update, so changing one config (for example, turning Code Origin off for a service) dropped a wildcard config that enabled DI, and DI turned off.Implementation details
Initialize()now checksDynamicInstrumentationCanBeEnabled && (DynamicSettings.DynamicInstrumentationEnabled ?? DynamicInstrumentationEnabled):falsesetsCanBeEnabledto false, so DI stays off;falseturns DI off even if the env var istrue;DebuggerManagerneeds no change. It already applies the same precedence before callingInitialize(), and itsDynamicInstrumentationproperty only returns an initialized instance, so once DI starts, later updates and deletes are applied.ApmTracingConfigMergernow usesApmTracingConfig.MergeLibConfigs(nowinternal) instead of its own incomplete copy, so one method merges every field.CombineApmTracingConfigurationkeeps active configs until they are removed explicitly.RcmSubscriptionManageralready reports every applied config that is missing fromclient_configsas removed, and only forgets an applied config when it reports it, so the active set stays in sync.falsedisables them).Test coverage
dotnet test tracer/test/Datadog.Trace.Tests/Datadog.Trace.Tests.csproj -f net10.0 --filter "FullyQualifiedName~Datadog.Trace.Tests.Configuration.DynamicConfigurationManagerTests|FullyQualifiedName~Datadog.Trace.Tests.Configuration.ApmTracingConfigMergerTests|FullyQualifiedName~Datadog.Trace.Tests.Debugger.DynamicInstrumentationTests"on 40ba240: 124 passed. This includes:DynamicInstrumentationEnablement_ServicesCalledOnlyWhenEnabled: env on; remote on with env unset (the remote-only case); both unset; env off; env off with remote on; env on with remote off;MergeConfigurations_DebuggerSettings_CorrectPriorityMerging: a wildcard config with DI, Exception Replay and Code Origin on, plus a service+env config with Exception Replay off, merges to Exception Replay off and DI and Code Origin on;CombineApmTracingConfiguration_WhenNewConfigIsProvided_KeepsUnchangedConfigs: updating one config keeps the other active config.DebuggerManagerDynamicTests(integration) not run locally.Other details