Repository navigation
[Debugger] DEBUG-5828 Coordinate snapshot sampling per trace - #9266
Conversation
BenchmarksBenchmark execution time: 2026-09-25 15:19:59 Comparing candidate commit d9f50fc in PR branch Found 0 performance improvements and 11 performance regressions! Performance is the same for 61 metrics, 0 unstable metrics, 73 known flaky benchmarks, 53 flaky benchmarks without significant changes.
|
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (9266) and master. ✅ No regressions detected |
There was a problem hiding this comment.
🔵 Needs a closer look
It changes concurrent sampling behavior on a customer-process hot path and warrants final human validation.
Pull request overview
Coordinates Live Debugger snapshot sampling per local trace while preserving independent sampling for excluded probe types.
Changes:
- Adds trace-scoped sampling state with concurrency-safe per-probe emission caps.
- Captures entry-time span context for stable snapshot correlation.
- Adds comprehensive unit and integration coverage.
File summaries
| File | Description |
|---|---|
CoordinatedSamplingTest.cs |
Adds an end-to-end sampling fixture. |
CoordinatedSamplingTests.cs |
Tests coordination, concurrency, caps, and correlation. |
ProbesTests.cs |
Verifies complete chains and line-probe caps. |
TraceContext.cs |
Stores trace-scoped debugger sampling state. |
DebuggerSnapshotCreator.cs |
Preserves entry-time span context. |
IDebuggerSamplingDecisionProvider.cs |
Defines allocation-free decision abstraction. |
DebuggerSamplingCoordinator.cs |
Implements synchronized trace-level decisions. |
ProbeProcessor.cs |
Integrates coordinated sampling into probe processing. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
/codex review |
There was a problem hiding this comment.
🟢 Approval recommended
The coordination lifecycle, failure handling, entry-time correlation, and per-probe limits are coherent and comprehensively tested.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
Security findingsAdvisory findings (1)ℹ️ 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. |
b645573 to
cc74db2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6455734d6
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Here are some automated security review suggestions for this pull request.
Reviewed commit: b6455734d6
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
38e161d to
6493c0b
Compare
6493c0b to
0cba76a
Compare
andrewlock
left a comment
There was a problem hiding this comment.
TraceContext addition looks fine to me. It'll impact the microbenchmarks due to the extra pointer-size allocation but I don't think there's any way around that
Share the first sampling decision across capturing probes and cap each probe to one snapshot per local trace. Preserve independent log and Exception Replay behavior, and stamp correlation IDs at capture start.
Release unused capture-expression reservations and retain rate-limit reasons for coordinated skip telemetry.
…empty Releasing the slot let every later hit in a kept trace capture and evaluate again without any sampler throttling, and it gained nothing: an empty result depends on the probe definition, so later hits are empty too. A probe updated mid-trace won't emit until the next trace.
TraceContext now only lazily creates the per-trace DebuggerSamplingCoordinator, like the AppSec, IAST and feature-flag state. Sampling logic stays in the debugger. The coordinator becomes the per-trace state class instead of a static wrapper around a nested State.
Store DebuggerSamplingDecision directly in the coordinator instead of a separate private state enum, with Undecided as the zero value so an unassigned decision never means Keep. Nested calls on the deciding thread are caught with Monitor.IsEntered rather than a Creating state. The decision is only published after Sample() returns, so a throwing sampler leaves the trace undecided without a reset.
0cba76a to
d9f50fc
Compare
Summary of changes
Reason for change
Implementation details
Which probes participate
ShouldCoordinateSampling).Flow at probe entry (
ProbeProcessor.TryBeginProcess)flowchart TD A[Probe hit] --> B{Has condition?} B -- yes --> C[No sampling yet.<br/>Capture span context, create snapshot creator] B -- no --> D{Coordinated probe<br/>and active trace?} D -- no --> E[Independent sampling:<br/>global limiter, then per-probe sampler] D -- yes --> F["traceContext.GetOrCreateDebuggerSamplingCoordinator()<br/>.TrySample(probeId, provider)"] F --> G{Decision} G -- Keep --> H[Capture span context,<br/>create snapshot creator, capture] G -- DropGlobal / DropProbe --> I[Skip before any capture work,<br/>record events.skipped] C --> J[Evaluate condition] J -- false --> K[Stop] J -- true --> Fdd.trace_id/dd.span_idand for the trace lookup, so a snapshot finalized after its span closed still correlates correctly.Per-trace coordinator (
DebuggerSamplingCoordinator)stateDiagram-v2 [*] --> Undecided Undecided --> Keep: first hit's samplers accept (its probe ID claims a slot) Undecided --> DropGlobal: global limiter rejects Undecided --> DropProbe: per-probe sampler rejects Undecided --> Undecided: sampler threw (nothing published, waiting callers decide) Keep --> Keep: later hit claims its probe's slot once, further hits get DropProbeTraceContextgains one nullable field and a lazyGetOrCreateDebuggerSamplingCoordinator()accessor, the same pattern as the AppSec, IAST and feature-flag per-trace state. All sampling logic lives in the debugger.DebuggerSamplingDecisiondirectly.Undecidedis the enum's zero value, so a new coordinator starts undecided and an unassigned decision never meansKeep.HashSet.Monitor.IsEntereddetects that nested call and returnsDropProbeinstead of re-entering the samplers, which could otherwise recurse without bound.IDebuggerSamplingDecisionProvider) avoids delegate allocation and boxing on the sampling path.A claimed slot is never released
Test coverage
CoordinatedSamplingTests): shared Keep/Drop decisions across snapshot and capture-expression probes, per-probe caps, conditions, independent sampling for template logs, decisions local to each trace, concurrent first hits, reentrancy, sampler exception recovery, empty capture expressions keeping the slot (including a mid-trace probe update), skipped-event telemetry, and entry-time span correlation.ProbesTests):CoordinatedSnapshotProbesEmitCompleteChains(a complete three-probe chain across sampling windows) andCoordinatedLineProbesEmitOncePerTrace(one snapshot per line probe per trace).Other details
TraceContext.reason:rateLimitProbeinevents.skipped.dd.trace_idremains the existing 64-bit projection.