[Debugger] Fix Timer leaks and missing rate updates in probe rate limiter#8619
Merged
Conversation
![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: aa2e1f5eff
ℹ️ 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".
BenchmarksBenchmark execution time: 2026-05-15 14:50:50 Comparing candidate commit 7cccc0a in PR branch Some scenarios are present only in baseline or only in candidate runs. If you didn't create or remove some scenarios in your branch, this maybe a sign of crashed benchmarks 💥💥💥 Scenarios present only in baseline:
Found 4 performance improvements and 1 performance regressions! Performance is the same for 49 metrics, 18 unstable metrics, 92 known flaky benchmarks, 34 flaky benchmarks without significant changes.
|
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes resource leaks and a rate-update bug in the debugger probe rate limiting pipeline by ensuring AdaptiveSampler timers are disposed when samplers are dropped, and by applying updated RCM rates to existing samplers rather than ignoring them.
Changes:
- Make
IAdaptiveSamplerdisposable and disposeAdaptiveSampler’s internalTimerwhen samplers are removed or lose insertion races. - Update
ProbeRateLimiterto (a) avoidConcurrentDictionary.GetOrAddfactory double-invocation leaks and (b) apply rate updates in-place on existing samplers. - Close an instrumentation race by registering processors/samplers before activating IL for newly-bound probes.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tracer/src/Datadog.Trace/Debugger/RateLimiting/IAdaptiveSampler.cs | Extends the sampler contract to IDisposable so implementations can release resources. |
| tracer/src/Datadog.Trace/Debugger/RateLimiting/AdaptiveSampler.cs | Adds Dispose() for timer cleanup and SetRate() to mutate rate-related fields in place. |
| tracer/src/Datadog.Trace/Debugger/RateLimiting/NopAdaptiveSampler.cs | Implements no-op Dispose() to satisfy IAdaptiveSampler : IDisposable. |
| tracer/src/Datadog.Trace/Debugger/RateLimiting/ProbeRateLimiter.cs | Reworks sampler add/update/remove logic to avoid leaks and ensure rate updates are applied. |
| tracer/src/Datadog.Trace/Debugger/DynamicInstrumentation.cs | Adjusts ordering in CheckUnboundProbes to prevent default-rate sampler races. |
| tracer/test/Datadog.Trace.Tests/Debugger/AdaptiveSamplerTests.cs | Adds tests for SetRate behavior and idempotent Dispose. |
| tracer/test/Datadog.Trace.Tests/Debugger/ProbeRateLimiterTests.cs | Adds new tests covering rate updates, disposal on removal, and GerOrAddSampler behavior. |
| tracer/test/Datadog.Trace.Tests/Debugger/ProbeProcessorTests.cs | Updates test sampler stub to implement Dispose() per new interface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
dudikeleti
force-pushed
the
dudik/fix-probe-rate-limiter
branch
from
May 13, 2026 10:35
aa2e1f5 to
40544e0
Compare
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8619) 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 (8619) - mean (73ms) : 70, 76
master - mean (73ms) : 68, 77
section Bailout
This PR (8619) - mean (77ms) : 74, 80
master - mean (77ms) : 74, 80
section CallTarget+Inlining+NGEN
This PR (8619) - mean (1,111ms) : 1038, 1184
master - mean (1,106ms) : 1059, 1153
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 (8619) - mean (113ms) : 110, 116
master - mean (113ms) : 109, 116
section Bailout
This PR (8619) - mean (118ms) : 112, 124
master - mean (118ms) : 110, 126
section CallTarget+Inlining+NGEN
This PR (8619) - mean (785ms) : 758, 812
master - mean (785ms) : 758, 812
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8619) - mean (103ms) : 97, 110
master - mean (104ms) : 98, 110
section Bailout
This PR (8619) - mean (102ms) : 99, 104
master - mean (103ms) : 99, 107
section CallTarget+Inlining+NGEN
This PR (8619) - mean (941ms) : 886, 997
master - mean (944ms) : 901, 987
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8619) - mean (100ms) : 95, 106
master - mean (99ms) : 96, 102
section Bailout
This PR (8619) - mean (103ms) : 98, 109
master - mean (104ms) : 98, 110
section CallTarget+Inlining+NGEN
This PR (8619) - mean (825ms) : 778, 871
master - mean (821ms) : 786, 856
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 (8619) - mean (200ms) : 193, 207
master - mean (199ms) : 192, 205
section Bailout
This PR (8619) - mean (203ms) : 195, 210
master - mean (203ms) : 198, 207
section CallTarget+Inlining+NGEN
This PR (8619) - mean (1,212ms) : 1131, 1294
master - mean (1,199ms) : 1157, 1241
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 (8619) - mean (288ms) : 281, 295
master - mean (289ms) : 282, 296
section Bailout
This PR (8619) - mean (288ms) : 279, 297
master - mean (289ms) : 282, 296
section CallTarget+Inlining+NGEN
This PR (8619) - mean (957ms) : 935, 979
master - mean (964ms) : 940, 988
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8619) - mean (280ms) : 269, 290
master - mean (280ms) : 273, 288
section Bailout
This PR (8619) - mean (278ms) : 272, 284
master - mean (281ms) : 276, 286
section CallTarget+Inlining+NGEN
This PR (8619) - mean (1,156ms) : 1116, 1196
master - mean (1,156ms) : 1120, 1193
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8619) - mean (279ms) : 271, 287
master - mean (279ms) : 273, 285
section Bailout
This PR (8619) - mean (280ms) : 275, 284
master - mean (279ms) : 271, 286
section CallTarget+Inlining+NGEN
This PR (8619) - mean (1,036ms) : 999, 1073
master - mean (1,039ms) : 1003, 1076
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
dudikeleti
enabled auto-merge (squash)
May 13, 2026 15:13
jpbempel
reviewed
May 15, 2026
Comment on lines
+153
to
+156
| // Plain writes match the rest of the class's relaxed memory model; rate change is | ||
| // visible "eventually" and self-corrects on the next window roll. | ||
| _samplesPerWindow = samplesPerWindow; | ||
| _samplesBudget = samplesPerWindow + ((long)_budgetLookback * samplesPerWindow); |
Member
There was a problem hiding this comment.
I haven't look deep into the algorithm, but I am scared that it may happen to have incoherent values between the 2 fields while processing the sampling leading to errors like exceptions.
Why not recreating an instance of AdaptiveSampler when changing the rate? should not be that frequent.
Contributor
Author
There was a problem hiding this comment.
I don't replace because ProbeData caches the sampler reference, so replacing the dictionary entry would not update existing probe data. I have a task in the pipeline to fix this, and then we’ll be able to handle it correctly.
For now, I thought it would be better to keep it relaxed temporarily, but you’re right about the concern. I added synchronization around the rare SetRate/RollWindow paths and atomic budget access while keeping Sample() lock-free.
…iter AdaptiveSampler owns a System.Threading.Timer that the runtime roots in its global timer queue until disposed. Three paths in the dynamic-instrumentation rate limiter dropped samplers without disposing them, so over the lifetime of a process every probe removal, every RCM-driven rate-update race, and every GerOrAddSampler race against an RCM update would leak a Timer plus the captured callback graph. This commit: - Makes IAdaptiveSampler implement IDisposable so callers can dispose any sampler implementation uniformly (NopAdaptiveSampler's Dispose is a no-op since it is a singleton). - Implements AdaptiveSampler.Dispose to release the timer. - Adds AdaptiveSampler.SetRate to mutate _samplesPerWindow / _samplesBudget in place, preserving the running EMAs and probability. Re-creating the sampler on rate updates (the previously suggested fix) would have wiped history and caused a transient sampling spike on every RCM rate change. Memory ordering matches the rest of the class, which is intentionally relaxed on the hot path. - ProbeRateLimiter.GerOrAddSampler now uses TryGetValue + TryAdd + Dispose on race-loss instead of ConcurrentDictionary.GetOrAdd, which can invoke its factory multiple times and silently drop the losing AdaptiveSampler (and its Timer). - ProbeRateLimiter.SetRate now applies the new rate to the existing sampler via AdaptiveSampler.SetRate instead of logging "already exist" and ignoring the update; also disposes the candidate sampler when it loses the insert race. - ProbeRateLimiter.ResetRate now disposes the removed sampler, fixing the leak that was triggered on every RCM probe removal. Adds AdaptiveSamplerTests for SetRate (budget update + history preservation) and Dispose (idempotent), plus a new ProbeRateLimiterTests suite covering SetRate in-place update, ResetRate disposal, and GerOrAddSampler no-leak behavior. Co-authored-by: Cursor <[email protected]>
When a previously-unbound line probe is resolved, DynamicInstrumentation. CheckUnboundProbes was calling DebuggerNativeMethods.InstrumentProbes before AddProbeProcessor / SetRateLimit. A probe hit in the window between those two calls would land in GerOrAddSampler, which inserts a default-rate sampler; the subsequent SetRateLimit would then see the existing entry and the user-configured rate would never take effect for that probe. Mirror the ordering used by InstrumentAddedProbes: register the processor and rate limiter for each newly-bound probe first, then make the IL live. Also tighten the inline comments that came in with the previous commit. Co-authored-by: Cursor <[email protected]>
dudikeleti
force-pushed
the
dudik/fix-probe-rate-limiter
branch
from
May 15, 2026 13:45
40544e0 to
7cccc0a
Compare
jpbempel
approved these changes
May 15, 2026
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
Fixes Timer/sampler leaks and a latent rate-update bug in
ProbeRateLimiterandAdaptiveSampler.AdaptiveSamplerallocates aSystem.Threading.Timerthat the runtime roots until disposed; three code paths previously dropped samplers without disposing them, leaking one Timer (plus captured state) on every probe removal and on every dictionary insert race. Separately, RCM-driven rate updates were silently ignored when a sampler already existed.Reason for change
Timerinstances on every RCM probe rotation — unbounded growth over the process lifetime.CheckUnboundProbesregistered the sampler after making the IL live, opening a race window where the first probe hit inserts a default-rate sampler and the configured rate is then dropped bySetRate.Implementation details
IAdaptiveSamplerextendsIDisposable;AdaptiveSampler.Disposereleases the internalTimer;NopAdaptiveSampler.Disposeis a no-op (singleton).AdaptiveSampler.SetRatemutates_samplesPerWindow/_samplesBudgetin place. Uses the same relaxed memory semantics as the rest of the class.ProbeRateLimiter.GerOrAddSamplerusesTryGetValue→TryAdd→Dispose-on-loseinstead ofConcurrentDictionary.GetOrAdd, which can invoke its factory multiple times.ProbeRateLimiter.SetRateapplies the new rate to the existing sampler instead of logging and returning; disposes the candidate if it loses the insert race.ProbeRateLimiter.ResetRatedisposes the removed sampler.DynamicInstrumentation.CheckUnboundProbesnow registers samplers and processors before callingDebuggerNativeMethods.InstrumentProbes, matching the ordering inInstrumentAddedProbesand closing the race at its source.Test coverage
AdaptiveSamplerTests—SetRateupdates budget and preserves running stats;Disposeis idempotent.ProbeRateLimiterTests(new) —SetRateupdates the existing sampler in place;ResetRatedisposes the removed sampler;GerOrAddSamplerdoes not dispose existing entries.