[Dynamic Instrumentation] Add Snapshot Exploration Test infrastructure#8572
Open
dudikeleti wants to merge 25 commits into
Open
[Dynamic Instrumentation] Add Snapshot Exploration Test infrastructure#8572dudikeleti wants to merge 25 commits into
dudikeleti wants to merge 25 commits into
Conversation
BenchmarksBenchmark execution time: 2026-06-17 18:03:55 Comparing candidate commit f20c7cf in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 70 metrics, 1 unstable metrics, 62 known flaky benchmarks, 64 flaky benchmarks without significant changes.
|
dudikeleti
force-pushed
the
dudik/snapshot-exploration-test-infra
branch
from
June 3, 2026 12:20
baaa4d5 to
dcf4b8a
Compare
This comment has been minimized.
This comment has been minimized.
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8572) and master. ✅ No regressions detected - check the details below Full Metrics ComparisonFakeDbCommand
HttpMessageHandler
Comparison explanationExecution-time benchmarks measure the whole time it takes to execute a program, and are intended to measure the one-off costs. Cases where the execution time results for the PR are worse than latest master results are highlighted in **red**. The following thresholds were used for comparing the execution times:
Note that these results are based on a single point-in-time result for each branch. For full results, see the dashboard. Graphs show the p99 interval based on the mean and StdDev of the test run, as well as the mean value of the run (shown as a diamond below the graph). Duration chartsFakeDbCommand (.NET Framework 4.8)gantt
title Execution time (ms) FakeDbCommand (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (74ms) : 70, 78
master - mean (73ms) : 71, 75
section Bailout
This PR (8572) - mean (77ms) : 74, 80
master - mean (77ms) : 75, 79
section CallTarget+Inlining+NGEN
This PR (8572) - mean (1,091ms) : 1048, 1133
master - mean (1,094ms) : 1050, 1139
FakeDbCommand (.NET Core 3.1)gantt
title Execution time (ms) FakeDbCommand (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (115ms) : 109, 121
master - mean (113ms) : 109, 116
section Bailout
This PR (8572) - mean (114ms) : 111, 117
master - mean (116ms) : 110, 122
section CallTarget+Inlining+NGEN
This PR (8572) - mean (795ms) : 770, 820
master - mean (793ms) : 769, 817
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (103ms) : 98, 107
master - mean (104ms) : 98, 111
section Bailout
This PR (8572) - mean (103ms) : 98, 108
master - mean (101ms) : 98, 104
section CallTarget+Inlining+NGEN
This PR (8572) - mean (947ms) : 912, 983
master - mean (949ms) : 908, 991
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (99ms) : 97, 102
master - mean (99ms) : 96, 103
section Bailout
This PR (8572) - mean (103ms) : 99, 107
master - mean (103ms) : 100, 107
section CallTarget+Inlining+NGEN
This PR (8572) - mean (823ms) : 784, 861
master - mean (821ms) : 785, 858
HttpMessageHandler (.NET Framework 4.8)gantt
title Execution time (ms) HttpMessageHandler (.NET Framework 4.8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (200ms) : 195, 206
master - mean (201ms) : 193, 209
section Bailout
This PR (8572) - mean (204ms) : 199, 210
master - mean (205ms) : 197, 213
section CallTarget+Inlining+NGEN
This PR (8572) - mean (1,200ms) : 1158, 1243
master - mean (1,197ms) : 1163, 1231
HttpMessageHandler (.NET Core 3.1)gantt
title Execution time (ms) HttpMessageHandler (.NET Core 3.1)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (286ms) : 277, 296
master - mean (287ms) : 281, 293
section Bailout
This PR (8572) - mean (288ms) : 284, 293
master - mean (285ms) : 279, 291
section CallTarget+Inlining+NGEN
This PR (8572) - mean (967ms) : 947, 987
master - mean (964ms) : 945, 984
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (281ms) : 272, 290
master - mean (282ms) : 274, 289
section Bailout
This PR (8572) - mean (280ms) : 275, 285
master - mean (282ms) : 275, 289
section CallTarget+Inlining+NGEN
This PR (8572) - mean (1,163ms) : 1115, 1211
master - mean (1,161ms) : 1126, 1197
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8572) - mean (278ms) : 267, 288
master - mean (281ms) : 273, 289
section Bailout
This PR (8572) - mean (279ms) : 267, 290
master - mean (282ms) : 276, 288
section CallTarget+Inlining+NGEN
This PR (8572) - mean (1,039ms) : 992, 1086
master - mean (1,038ms) : 1007, 1070
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dudikeleti
force-pushed
the
dudik/snapshot-exploration-test-infra
branch
2 times, most recently
from
June 5, 2026 20:46
10a46c1 to
3f67c3c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new “snapshot exploration test” mode for Dynamic Instrumentation: a build-driven pipeline that installs many method probes against third-party test projects, captures a single snapshot per probe, writes per-process CSV artifacts, and then analyzes managed/native logs + CSVs to grade probe validity.
Changes:
- Added snapshot-exploration runtime plumbing (test-host detection/constants, CSV snapshot sink, probe-status CSV reporting/polling, and factory wiring to use no-op uploaders in this mode).
- Updated snapshot/log sink/uploader plumbing (introduced
ISnapshotSink, made snapshot payload nullable end-to-end, and adjusted uploader base disposal semantics). - Added Nuke targets + analysis implementation for generating probes and validating results (
SetUpSnapshotExplorationTests/RunSnapshotExplorationTests) plus a unit test for the new sink.
Reviewed changes
Copilot reviewed 26 out of 30 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/test/Datadog.Trace.Tests/Telemetry/config_norm_rules.json | Adds normalization rule for new internal snapshot exploration config key. |
| tracer/test/Datadog.Trace.Tests/Debugger/SnapshotExplorationTestSinkTests.cs | New unit test validating root-level logger extraction behavior. |
| tracer/test/Datadog.Trace.Tests/Debugger/DynamicInstrumentationTests.cs | Updates mocks for nullable snapshot payload signature. |
| tracer/src/Datadog.Trace/Generated/netstandard2.0/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.Debugger.g.cs | Adds generated const for snapshot exploration root path key. |
| tracer/src/Datadog.Trace/Generated/netcoreapp3.1/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.Debugger.g.cs | Same generated key addition (netcoreapp3.1). |
| tracer/src/Datadog.Trace/Generated/net6.0/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.Debugger.g.cs | Same generated key addition (net6.0). |
| tracer/src/Datadog.Trace/Generated/net461/Datadog.Trace.SourceGenerators/ConfigurationKeysGenerator/ConfigurationKeys.Debugger.g.cs | Same generated key addition (net461). |
| tracer/src/Datadog.Trace/Debugger/Snapshots/DebuggerSnapshotCreator.cs | Small snapshot JSON writer robustness comment/guard. |
| tracer/src/Datadog.Trace/Debugger/SnapshotExplorationConstants.cs | New constants + vstest-host detection helper for snapshot exploration mode. |
| tracer/src/Datadog.Trace/Debugger/Sink/SnapshotUploader.cs | Switches to ISnapshotSink, accepts nullable snapshots, and adds sink disposal hook. |
| tracer/src/Datadog.Trace/Debugger/Sink/SnapshotSink.cs | Implements ISnapshotSink, routes to CSV sink in exploration mode, and propagates nullable snapshots. |
| tracer/src/Datadog.Trace/Debugger/Sink/SnapshotExplorationTestSink.cs | New CSV-writing snapshot sink for exploration test reporting. |
| tracer/src/Datadog.Trace/Debugger/Sink/NoOpDebuggerUploader.cs | New no-op uploader used to disable diagnostics in exploration mode. |
| tracer/src/Datadog.Trace/Debugger/Sink/ISnapshotUploader.cs | Updates snapshot payload to nullable. |
| tracer/src/Datadog.Trace/Debugger/Sink/ISnapshotSink.cs | New sink abstraction used by uploaders/sinks. |
| tracer/src/Datadog.Trace/Debugger/Sink/DebuggerUploaderBase.cs | Makes disposal idempotent via an interlocked flag and adds protected virtual dispose hook. |
| tracer/src/Datadog.Trace/Debugger/ProbeStatuses/SnapshotExplorationProbeStatusReporter.cs | New per-process CSV writer for probe status events in exploration mode. |
| tracer/src/Datadog.Trace/Debugger/ProbeStatuses/SnapshotExplorationProbeStatusPoller.cs | New probe status poller that writes to CSV instead of normal diagnostics flow. |
| tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionsProcessor.cs | Removes unused Newtonsoft import. |
| tracer/src/Datadog.Trace/Debugger/Expressions/ProbeExpressionEvaluator.cs | Refactors/relocates CompileAll helper. |
| tracer/src/Datadog.Trace/Debugger/ExceptionAutoInstrumentation/ExceptionReplay.cs | Switches snapshot sink type + nullable snapshot handling. |
| tracer/src/Datadog.Trace/Debugger/DynamicInstrumentation.cs | Exploration-mode rate limit override and nullable snapshot/log plumbing. |
| tracer/src/Datadog.Trace/Debugger/DebuggerSettings.cs | Adds snapshot exploration root path setting and derives exploration-mode enablement. |
| tracer/src/Datadog.Trace/Debugger/DebuggerManager.SnapshotExploration.cs | New snapshot exploration init path (test-host only) + native log polling for probe install readiness. |
| tracer/src/Datadog.Trace/Debugger/DebuggerManager.cs | Routes snapshot exploration mode into the specialized init path. |
| tracer/src/Datadog.Trace/Debugger/DebuggerFactory.cs | Wires exploration mode sinks/uploaders/poller and avoids double-opening the CSV writer. |
| tracer/src/Datadog.Trace/Configuration/supported-configurations.yaml | Documents new internal snapshot exploration root path configuration. |
| tracer/build/_build/BuildVariables.cs | Adds exploration-mode env var changes for offline snapshot runs. |
| tracer/build/_build/Build.SnapshotExplorationTest.cs | New Nuke implementation for generating probes, running tests, and analyzing results. |
| tracer/build/_build/Build.ExplorationTests.cs | Adds Nuke targets and shared test runner helper; updates log env vars for long runs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dudikeleti
force-pushed
the
dudik/snapshot-exploration-test-infra
branch
from
June 9, 2026 10:54
3f67c3c to
c4d370f
Compare
dudikeleti
added a commit
that referenced
this pull request
Jun 9, 2026
## Summary of changes
- Simplify `ProbeExpressionEvaluator.CompileAll` by dropping the
redundant `== null ? null :` ternaries; each `Compile*` helper already
null-checks its source and returns `null`.
- Use the nullable-aware `StringUtil.IsNullOrEmpty` instead of
`string.IsNullOrEmpty` in the `#nullable enable`
`ProbeExpressionEvaluator`.
- Harden `DebuggerManager.ShouldSkipDiUpdate` with a property-pattern
null check (`debuggerSettings is { DynamicInstrumentationCanBeEnabled:
false }`).
- Remove unused usings in `ProbeExpressionsProcessor` (`Newtonsoft.Json`
and `Debugger.Snapshots`).
- Add a clarifying comment in `DebuggerSnapshotCreator.EndEntry` about
why arg/local capture must not rely on heuristics.
- Fix a logging bug in `ProbeExpressionsProcessor.AddProbeProcessor` so
it no longer logs `"Successfully created probe processor"` after
creation throws.
## Reason for change
- These are low-risk cleanups split out of #8572 to keep that PR focused
on the snapshot-exploration feature.
- The `AddProbeProcessor` change fixes a real (debug-level) logging
inconsistency: a probe whose processor creation throws would log both a
failure and a success line.
## Implementation details
- `CompileAll` simplification is behavior-preserving:
`CompileTemplates`/`CompileCondition`/`CompileMetric`/`CompileDecorations`
each return `null` when their source is null. One benign difference:
when `SpanDecorations` is null, `CompileDecorations` now emits its
existing debug-level "is null" log.
- `StringUtil.IsNullOrEmpty` only adds value under `#nullable enable`
(it carries the `[NotNullWhen(false)]` annotation), so the swap was
limited to that file; other `string.IsNullOrEmpty` call sites in
non-nullable files were intentionally left unchanged.
- The `ShouldSkipDiUpdate` property pattern is equivalent for the normal
(non-null) case and additionally avoids a potential NRE if settings were
ever null.
## Test coverage
- No new tests added; changes are cleanups plus one debug-only logging
fix.
---------
Co-authored-by: Cursor <[email protected]>
dudikeleti
force-pushed
the
dudik/snapshot-exploration-test-infra
branch
2 times, most recently
from
June 9, 2026 16:43
5a7085a to
d25407f
Compare
dudikeleti
marked this pull request as ready for review
June 9, 2026 17:11
dudikeleti
enabled auto-merge (squash)
June 11, 2026 15:10
zacharycmontoya
pushed a commit
that referenced
this pull request
Jun 16, 2026
## Summary of changes
- Simplify `ProbeExpressionEvaluator.CompileAll` by dropping the
redundant `== null ? null :` ternaries; each `Compile*` helper already
null-checks its source and returns `null`.
- Use the nullable-aware `StringUtil.IsNullOrEmpty` instead of
`string.IsNullOrEmpty` in the `#nullable enable`
`ProbeExpressionEvaluator`.
- Harden `DebuggerManager.ShouldSkipDiUpdate` with a property-pattern
null check (`debuggerSettings is { DynamicInstrumentationCanBeEnabled:
false }`).
- Remove unused usings in `ProbeExpressionsProcessor` (`Newtonsoft.Json`
and `Debugger.Snapshots`).
- Add a clarifying comment in `DebuggerSnapshotCreator.EndEntry` about
why arg/local capture must not rely on heuristics.
- Fix a logging bug in `ProbeExpressionsProcessor.AddProbeProcessor` so
it no longer logs `"Successfully created probe processor"` after
creation throws.
## Reason for change
- These are low-risk cleanups split out of #8572 to keep that PR focused
on the snapshot-exploration feature.
- The `AddProbeProcessor` change fixes a real (debug-level) logging
inconsistency: a probe whose processor creation throws would log both a
failure and a success line.
## Implementation details
- `CompileAll` simplification is behavior-preserving:
`CompileTemplates`/`CompileCondition`/`CompileMetric`/`CompileDecorations`
each return `null` when their source is null. One benign difference:
when `SpanDecorations` is null, `CompileDecorations` now emits its
existing debug-level "is null" log.
- `StringUtil.IsNullOrEmpty` only adds value under `#nullable enable`
(it carries the `[NotNullWhen(false)]` annotation), so the swap was
limited to that file; other `string.IsNullOrEmpty` call sites in
non-nullable files were intentionally left unchanged.
- The `ShouldSkipDiUpdate` property pattern is equivalent for the normal
(non-null) case and additionally avoids a potential NRE if settings were
ever null.
## Test coverage
- No new tests added; changes are cleanups plus one debug-only logging
fix.
---------
Co-authored-by: Cursor <[email protected]>
Introduce a self-contained 'snapshot exploration' test mode that, when enabled, runs Dynamic Instrumentation against a list of probes (CSV) over a third-party test project, captures snapshots for every method called, and produces a CSV report that grades each probe as valid/invalid based on parsed managed and native log output. The runtime path is gated behind two internal config keys: * DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ENABLED * DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ROOT_PATH When the flag is off (production), this PR is a no-op aside from a small number of nullable-string signatures. Key pieces: * New Nuke targets SetUpSnapshotExplorationTests / RunSnapshotExplorationTests (Build.SnapshotExplorationTest.cs). * Per-process CSV report writer (SnapshotExplorationTestSink). * Mocked DiscoveryService and NoOp ProbeStatusPoller / SymbolUploader so the test mode never reaches an agent. * Per-probe rate-limit and snapshot-cap (ExplorationTestProbeTracker) and cheap perf metrics around hot paths (ExplorationTestMetrics). * DebuggerManager.SnapshotExploration.cs routes initialization through a test-host-only path that waits for the native side to receive probes. * DynamicInstrumentation.IsInitialized is replaced with a Task-based latch so callers can await initialization (used by the new init path). * SnapshotSink/SnapshotUploader switched to ISnapshotSink, accept nullable snapshots, ProbeStatusPoller factory returns IProbeStatusPoller. * Native logs in debugger_method_rewriter.cpp and debugger_rejit_preprocessor.cpp get NOTE comments documenting the strings parsed by the build-side analyzer. * New unit test SnapshotExplorationTestSinkTests verifies the sink writes the expected CSV row when the snapshot has a nested logger property. Does not include the unrelated bug fixes that were also on the original branch; those are in their own PRs. Co-authored-by: Cursor <[email protected]>
Restore DD_INTERNAL_DEBUGGER_INSTRUMENT_ALL for line-probe runs (native gating fix), revert the dead nullable-snapshot churn and silent null-drop, add sink unit tests (CSV escaping, dedup, invalid grading), and drop unrelated drive-by edits to keep the PR scoped.
Co-authored-by: Cursor <[email protected]>
Correct instance/static probe detection using extracted symbols, and reuse the quote-aware CSV parser for probe status reports so escaped fields don’t corrupt status analysis.
dudikeleti
force-pushed
the
dudik/snapshot-exploration-test-infra
branch
from
June 17, 2026 17:03
5b5a6c9 to
f20c7cf
Compare
andrewlock
reviewed
Jul 1, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
I have a few concerns with the huge extra complexity this seems to add to production code... It feels like the vast majority of this shouldn't be in Datadog.Trace. I would recommend considering if there's any way to get the same behaviour using a test agent that dumps things to file, if that's what you need. Ultimately it's up to you of course.
RE the build changes, I think we can simplify it a little, but mostly just move it out of being a Nuke partial Build class. Also, can we retire the old exploration tests now (as they're flaky and add little value IMO?) Separate PR if so.
Finally, the config needs to be registered in the config registry, we mustn't use #pragma etc to ignore the errors.
| .After(Clean, BuildTracerHome) | ||
| .Executes(() => | ||
| { | ||
| if (ExplorationTestUseCase != global::ExplorationTestUseCase.Debugger) |
Member
There was a problem hiding this comment.
It seems weird that you have to specify a ExplorationTestUseCase, but then this just bails out if it's not set to debugger? 🤔 Should we just make these debugger-specific anyway (they seem to be debugger specific already anyway, because of the whole "snapshot" part?)
Contributor
Author
There was a problem hiding this comment.
Correct. I'll fix it.
| }) | ||
| ; | ||
|
|
||
| Target SetUpSnapshotExplorationTests |
Member
There was a problem hiding this comment.
Are these exploration tests going to replace our existing exploration tests? I ask, because they currently flake a lot, they're fixed in time to (old) versions of libraries, and I'm not aware of them ever identifying a "real" issue... so should we just replace those old versions?
Contributor
Author
There was a problem hiding this comment.
At this point, they're indeed a bit less necessary (unless we add new IL rewriting capabilities). Early on, they helped uncover issues that we probably wouldn't have caught ourselves until they surfaced at a customer's site.
That said, you're right that we could probably replace them. The downside is figuring out when to run them. For the existing exploration tests it's easy, we should run them whenever there are changes in the CLR profiler files. But those new tests are end-to-end tests. 🤔
In any case, I think we'll need to clean this up. We could perhaps keep only the new tests, with a few different levels of test granularity.
Member
There was a problem hiding this comment.
nit: Instead of making this a partial class on Build, move it into its own sub-folder, and pass in the variables you need...
The problem with the current approach is that all of these "private" methods are available in every single Nuke target, which is confusing when working on those, and breaks encapsulation
Comment on lines
+126
to
+129
| if (settings.IsSnapshotExplorationTestEnabled) | ||
| { | ||
| return NoOpSymbolUploader.Instance; | ||
| } |
Member
There was a problem hiding this comment.
nit: Is it necessary to add all these special cases in? If we can just disable this functionality using env variables externally, that seems preferable to bloating the size of production code and adding complexity?
Contributor
Author
There was a problem hiding this comment.
I'm not sure I follow. I'll catch you offline to discuss it.
Comment on lines
+221
to
+227
| // Snapshot exploration test runs take a different initialization path (no agent | ||
| // discovery, no-op symbols/probe-status pollers, sink writes to CSV). | ||
| // Route here so that Instrumentation.cs stays free of debugger test plumbing. | ||
| // This intentionally runs and completes synchronously (returning an already-completed | ||
| // task): the test host must not start executing probed code until initialization and | ||
| // probe installation have finished. This path only runs inside a vstest test host. | ||
| if (settings.IsSnapshotExplorationTestEnabled) |
Member
There was a problem hiding this comment.
It seems like we're adding a lot of extra complexity here just to configure all this stuff for testing. Is there a good reason why we can't send all these snapshots to the test agent as usual, and just have the test agent dump them instead?
Contributor
Author
There was a problem hiding this comment.
Unfortunately, this would not be enough.
|
|
||
| ProbeFile = config.WithKeys(ConfigurationKeys.Debugger.DynamicInstrumentationProbeFile).AsString() ?? string.Empty; | ||
| // This is an internal test harness flag, not a registry-backed tracer configuration. | ||
| #pragma warning disable RS0030 |
Member
There was a problem hiding this comment.
You must not disable these. You must add new settings to the config registry
| /// setup tails these files to detect when the native side has accepted | ||
| /// the probe configuration. | ||
| /// </summary> | ||
| public const string NativeLogFilePattern = "dotnet-tracer-native-*.log"; |
Member
There was a problem hiding this comment.
This feels like a really nasty smell... this sort of thing shouldn't be in the Datadog.Trace binary we ship IMO 😟 This should all be external...
Contributor
Author
There was a problem hiding this comment.
ok lets talk offline
|
|
||
| namespace Datadog.Trace.Tests.Debugger; | ||
|
|
||
| [Collection(nameof(EnvironmentVariablesTestCollection))] |
Member
There was a problem hiding this comment.
This is a massive smell 🙁 We are trying to remove these. I don't see a reason we can't just read this from configuration like "normal" instead? 🤔
| "_DD_ROOT_DOTNET_SESSION_ID", | ||
| "DD_INTERNAL_TRACE_NATIVE_ENGINE_PATH", | ||
| "DD_INTERNAL_PROFILING_NATIVE_ENGINE_PATH", | ||
| "DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ROOT_PATH", |
Member
There was a problem hiding this comment.
You can't add more here 😉
Suggested change
| "DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ROOT_PATH", |
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
SetUpSnapshotExplorationTests/RunSnapshotExplorationTeststo generateSnapshotExplorationTestProbes.json, run the selected debugger exploration test, and analyze the generated reports/logs after the test completes.DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ROOT_PATHis present:SnapshotExplorationTestSink,SnapshotExplorationProbeStatusPoller+SnapshotExplorationProbeStatusReporter,SnapshotExplorationConstants,NoOpDebuggerUploader, and a snapshot-exploration init path onDebuggerManager.ISnapshotSinkinterface so normal debugger uploads continue to useSnapshotSink, while snapshot exploration can useSnapshotExplorationTestSinkto write report rows instead of buffering uploads for an agent.DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ROOT_PATHas an internal test-harness environment variable, not a generatedConfigurationKeys.Debuggersetting, and exclude it from configuration telemetry registration validation.SnapshotExplorationTestSinkreport writing, CSV escaping, duplicate probe handling, and invalid/partial snapshot grading.Reason for change
Implementation details
DD_INTERNAL_SNAPSHOT_EXPLORATION_TEST_ROOT_PATHis set and the current process is detected as a vstest host. Child processes may inherit the env var, but they keep the normal debugger path.SnapshotExplorationTestProbes.jsonwithLOG_PROBEdefinitions,captureSnapshot=true, and a deterministic sampledwhenexpression for some instance methods.Test coverage
SnapshotExplorationTestSinkTests(7 cases: report content, nested-logger handling, CSV escaping, probe-id dedup, and invalid/partial-snapshot grading).RunSnapshotExplorationTestsNuke target is intentionally not wired into CI in this PR because it is long-running and debugger-specific.Other details