[Debugger] Add global rate limiter#8480
Merged
Merged
Conversation
Execution-Time Benchmarks Report ⏱️Execution-time results for samples comparing This PR (8480) 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 (8480) - mean (73ms) : 70, 76
master - mean (73ms) : 70, 75
section Bailout
This PR (8480) - mean (77ms) : 74, 79
master - mean (79ms) : 75, 82
section CallTarget+Inlining+NGEN
This PR (8480) - mean (1,114ms) : 1055, 1173
master - mean (1,111ms) : 1049, 1173
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 (8480) - mean (114ms) : 110, 117
master - mean (116ms) : 110, 122
section Bailout
This PR (8480) - mean (118ms) : 112, 124
master - mean (117ms) : 112, 122
section CallTarget+Inlining+NGEN
This PR (8480) - mean (793ms) : 773, 814
master - mean (788ms) : 761, 814
FakeDbCommand (.NET 6)gantt
title Execution time (ms) FakeDbCommand (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8480) - mean (103ms) : 97, 109
master - mean (102ms) : 97, 107
section Bailout
This PR (8480) - mean (104ms) : 100, 109
master - mean (106ms) : 99, 113
section CallTarget+Inlining+NGEN
This PR (8480) - mean (960ms) : 923, 997
master - mean (948ms) : 909, 986
FakeDbCommand (.NET 8)gantt
title Execution time (ms) FakeDbCommand (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8480) - mean (100ms) : 96, 103
master - mean (100ms) : 95, 104
section Bailout
This PR (8480) - mean (104ms) : 99, 110
master - mean (102ms) : 97, 107
section CallTarget+Inlining+NGEN
This PR (8480) - mean (820ms) : 784, 857
master - mean (824ms) : 787, 861
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 (8480) - mean (197ms) : 191, 203
master - mean (196ms) : 191, 201
section Bailout
This PR (8480) - mean (200ms) : 196, 204
master - mean (200ms) : 195, 206
section CallTarget+Inlining+NGEN
This PR (8480) - mean (1,189ms) : 1142, 1237
master - mean (1,192ms) : 1156, 1229
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 (8480) - mean (282ms) : 273, 291
master - mean (286ms) : 280, 291
section Bailout
This PR (8480) - mean (283ms) : 277, 289
master - mean (286ms) : 279, 292
section CallTarget+Inlining+NGEN
This PR (8480) - mean (955ms) : 937, 974
master - mean (954ms) : 928, 981
HttpMessageHandler (.NET 6)gantt
title Execution time (ms) HttpMessageHandler (.NET 6)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8480) - mean (276ms) : 270, 282
master - mean (275ms) : 268, 283
section Bailout
This PR (8480) - mean (276ms) : 269, 282
master - mean (275ms) : 268, 283
section CallTarget+Inlining+NGEN
This PR (8480) - mean (1,153ms) : 1113, 1193
master - mean (1,150ms) : 1111, 1189
HttpMessageHandler (.NET 8)gantt
title Execution time (ms) HttpMessageHandler (.NET 8)
dateFormat x
axisFormat %Q
todayMarker off
section Baseline
This PR (8480) - mean (274ms) : 266, 281
master - mean (275ms) : 268, 282
section Bailout
This PR (8480) - mean (275ms) : 269, 281
master - mean (275ms) : 269, 281
section CallTarget+Inlining+NGEN
This PR (8480) - mean (1,032ms) : 1000, 1063
master - mean (1,034ms) : 994, 1073
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
BenchmarksBenchmark execution time: 2026-05-26 09:23:41 Comparing candidate commit 9b2dc20 in PR branch Found 0 performance improvements and 1 performance regressions! Performance is the same for 71 metrics, 0 unstable metrics, 62 known flaky benchmarks, 64 flaky benchmarks without significant changes.
|
dudikeleti
force-pushed
the
dudik/probes-global-rate-limit
branch
from
May 20, 2026 11:02
caecce0 to
bce4414
Compare
This comment has been minimized.
This comment has been minimized.
dudikeleti
force-pushed
the
dudik/probes-global-rate-limit
branch
from
May 20, 2026 16:46
bce4414 to
b50b759
Compare
dudikeleti
force-pushed
the
dudik/probes-global-rate-limit
branch
from
May 22, 2026 10:55
b50b759 to
5a3585a
Compare
dudikeleti
marked this pull request as ready for review
May 22, 2026 10:55
dudikeleti
changed the title
![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: 5a3585af3a
ℹ️ 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".
jpbempel
reviewed
May 22, 2026
Comment on lines
+121
to
+122
| return sampler.Sample() | ||
| && (probeInfo.ProbeType != ProbeType.Snapshot || _globalRateLimiter.ShouldSampleSnapshot(probeInfo.ProbeId)); |
Member
There was a problem hiding this comment.
beware here on the order you invoke the samplers because you can have issues as the global will not be invoked as long as the per-probe returns true (short circuit of the condition). but once the sampler per-probe failed you start invoking the global one, but the total of sample starts at different point in time.
see the fix I made:
DataDog/dd-trace-java#5332
Contributor
Author
There was a problem hiding this comment.
I did it in this order to avoid a case where one probe with a lot of hits would affect another probe with only a single hit if we applied the global sampler first. Since the per-probe sampler already filters out some snapshots anyway, applying it first means the global sampler only sees snapshots that would actually be emitted in the end, which helps preserve fairness between probes.
But you’re right about the correctness of the global adaptive sampler. (I’m not sure what makes more sense from a product perspective). Thanks for pointing it out, fixed in b95673e
| // Avoid ConcurrentDictionary.GetOrAdd(factory): its factory can run more than once | ||
| // under contention, leaking the losing sampler's Timer (rooted by the runtime). | ||
| if (_samplers.TryGetValue(probeId, out var sampler)) | ||
| while (true) |
Member
There was a problem hiding this comment.
I am concern that under high contention you make no progress on the thread, plus you are hammering the dict. will be good to have some backoff or a way to break this loop after a certain number of retries
Contributor
Author
There was a problem hiding this comment.
Good point. In practice the loop bounded at ~2 iterations (only spins if ResetRate races for the same probeId), but you're right it's theoretically not bounded. Switched to GetOrAdd(key, value) to drop the loop entirely.
7180f83
dudikeleti
force-pushed
the
dudik/probes-global-rate-limit
branch
2 times, most recently
from
May 26, 2026 08:22
b95673e to
7180f83
Compare
jpbempel
approved these changes
May 26, 2026
…im rate-limit overhead
dudikeleti
force-pushed
the
dudik/probes-global-rate-limit
branch
from
May 26, 2026 08:34
1a0e46c to
9b2dc20
Compare
dudikeleti
enabled auto-merge (squash)
May 26, 2026 08:35
andrewlock
approved these changes
May 26, 2026
andrewlock
left a comment
andrewlock
left a comment
Member
There was a problem hiding this comment.
Just approving the nullability files, but took a glance at the other code, and I'd suggest removing implicit shared static state wherever you can, as it adds complexity, especially for testing
Comment on lines
+46
to
+52
| internal static void TryDisposeInstance() | ||
| { | ||
| if (InstanceLazy.IsValueCreated) | ||
| { | ||
| InstanceLazy.Value.Dispose(); | ||
| } | ||
| } |
Member
There was a problem hiding this comment.
Oh my, what's going on here 😅 Disposal shouldn't be this complicated 🙈
|
|
||
| namespace Datadog.Trace.Debugger.RateLimiting | ||
| { | ||
| internal interface IDebuggerGlobalRateLimiter : IDisposable |
Member
There was a problem hiding this comment.
nit: if you only have one implementation, you probably don't need an interface 🤷♂️ And if the reason you have an interface is because of the static state in your implementation, that's the root cause 😉
Contributor
Author
Yes, it’s complicated because of existing code debt. The issue is the relationship between the global rate limiter, which is supposedly process-wide and the sharing across different debugger products, and the way |
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
adaptive global sampler observes the full snapshot traffic and stays
correctly calibrated.
ConfigurationUpdater, andDynamicInstrumentation.Reason for change
AdaptiveSamplerinstances could stay rooted after replacement/removal.Implementation details
DebuggerGlobalRateLimiterowns one adaptive sampler for snapshot probes.100/s.ServiceConfiguration.Sampling.SnapshotsPerSecondupdates the snapshot global limiter. When no service sampling config is present, the limiter falls back to its default.ProbeProcessorsamples snapshot probes in this order:AdaptiveSamplerLifetimecentralizes sampler creation, replacement, and disposal so losing add races and removed probes do not leak timers.Test coverage
DebuggerGlobalRateLimiterTests|ConfigurationUpdaterTests|ProbeRateLimiterTests