Skip to content

fix(sampling): record rate limiter's effective rate on allow, not just drop - #2288

Open
MilanGarnier wants to merge 1 commit into
mainfrom
milan.garnier/otel-rate-limiter-effective-rate
Open

fix(sampling): record rate limiter's effective rate on allow, not just drop#2288
MilanGarnier wants to merge 1 commit into
mainfrom
milan.garnier/otel-rate-limiter-effective-rate

Conversation

@MilanGarnier

@MilanGarnier MilanGarnier commented Jul 28, 2026

Copy link
Copy Markdown

What

DatadogSampler::sample_root only recorded rl_effective_rate (the rate
limiter's effective accept rate, wired as _dd.limit_psr) when the rate
limiter dropped a trace. It should be recorded whenever the limiter is
consulted, regardless of outcome.

Why this doesn't follow other tracers

  • dd-trace-rb (rule_sampler.rb, apply_rule!): calls
    rate_limiter.allow?.tap { |allowed| ... set_limiter_metrics(trace, rate_limiter.effective_rate) ... } — the effective rate is set inside the
    .tap block unconditionally, for both allowed == true and false.
  • dd-trace-go (span.go): calls limiter.AllowOne(now) and then always
    calls setMetricLocked(keyRulesSamplerLimiterRate, limiterRate) right
    after, independent of the allow/drop outcome.

libdatadog: _dd.limit_psr was only emitted on
rate-limiter-drop, silently omitting it on allow. This matters for the
Datadog Agent's stats extrapolation (1/psr), which needs the tag whenever
the limiter was in play, not just when it rejected the trace.

Change

sample_root: once a rule matches and keeps the trace, always call
self.rate_limiter.effective_rate() after is_allowed(), whether or not
the limiter allowed it.

Tests

cargo test -p libdd-sampling: 104 lib + 2 doctests pass. fmt and clippy
clean.

@MilanGarnier
MilanGarnier requested a review from a team as a code owner July 28, 2026 16:18
@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

📚 Documentation Check Results

⚠️ 180 documentation warning(s) found

📦 libdd-sampling - 180 warning(s)


Updated: 2026-07-29 08:33:01 UTC | Commit: 901dcfa | missing-docs job results

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

🔒 Cargo Deny Results

⚠️ 2 issue(s) found, showing only errors (advisories, bans, sources)

📦 libdd-sampling - 2 error(s)

Show output
error[vulnerability]: Invalid pointer dereference in `fmt::Pointer` impl for `Atomic` and `Shared` when the underlying pointer is invalid
   ┌─ /home/runner/work/libdatadog/libdatadog/Cargo.lock:30:1
   │
30 │ crossbeam-epoch 0.9.18 registry+https://github.com/rust-lang/crates.io-index
   │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ security vulnerability detected
   │
   ├ ID: RUSTSEC-2026-0204
   ├ Advisory: https://rustsec.org/advisories/RUSTSEC-2026-0204
   ├ Affected versions of `fmt::Display` dereference the underlying pointer. This causes a invalid pointer dereference e.g., when a pointer created with `Atomic::null` or `Shared::null`. `fmt::Debug` impls and pre-0.9 `fmt::Display` impls, which do not dereference pointers, are not affected by this issue.
   ├ Announcement: https://github.com/crossbeam-rs/crossbeam/pull/1276
   ├ Solution: Upgrade to >=0.9.20 (try `cargo update -p crossbeam-epoch`)
   ├ crossbeam-epoch v0.9.18
     └── crossbeam-deque v0.8.5
         └── rayon-core v1.12.1
             └── rayon v1.10.0
                 └── criterion v0.5.1
                     ├── libdd-common v5.1.0
                     │   └── libdd-sampling v5.0.0
                     └── (dev) libdd-sampling v5.0.0 (*)

error[unsound]: Rand is unsound with a custom logger using `rand::rng()`
   ┌─ /home/runner/work/libdatadog/libdatadog/Cargo.lock:97:1
   │
97 │ rand 0.8.5 registry+https://github.com/rust-lang/crates.io-index
   │ ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ unsound advisory detected
   │
   ├ ID: RUSTSEC-2026-0097
   ├ Advisory: https://rustsec.org/advisories/RUSTSEC-2026-0097
   ├ It has been reported (by [@lopopolo](https://github.com/lopopolo)) that the `rand` library is [unsound](https://rust-lang.github.io/unsafe-code-guidelines/glossary.html#soundness-of-code--of-a-library) (i.e. that safe code using the public API can cause Undefined Behaviour) when all the following conditions are met:
     
     - The `log` and `thread_rng` features are enabled
     - A [custom logger](https://docs.rs/log/latest/log/#implementing-a-logger) is defined
     - The custom logger accesses `rand::rng()` (previously `rand::thread_rng()`) and calls any `TryRng` (previously `RngCore`) methods on `ThreadRng`
     - The `ThreadRng` (attempts to) reseed while called from the custom logger (this happens every 64 kB of generated data)
     - Trace-level logging is enabled or warn-level logging is enabled and the random source (the `getrandom` crate) is unable to provide a new seed
     
     `TryRng` (previously `RngCore`) methods for `ThreadRng` use `unsafe` code to cast `*mut BlockRng<ReseedingCore>` to `&mut BlockRng<ReseedingCore>`. When all the above conditions are met this results in an aliased mutable reference, violating the Stacked Borrows rules. Miri is able to detect this violation in sample code. Since construction of [aliased mutable references is Undefined Behaviour](https://doc.rust-lang.org/stable/nomicon/references.html), the behaviour of optimized builds is hard to predict.
   ├ Announcement: https://github.com/rust-random/rand/pull/1763
   ├ Solution: Upgrade to >=0.10.1 OR <0.10.0, >=0.9.3 OR <0.9.0, >=0.8.6 (try `cargo update -p rand`)
   ├ rand v0.8.5
     └── (dev) libdd-common v5.1.0
         └── libdd-sampling v5.0.0

advisories FAILED, bans ok, sources ok

Updated: 2026-07-29 08:34:55 UTC | Commit: 901dcfa | dependency-check job results

@MilanGarnier
MilanGarnier changed the base branch from milan.garnier/otel-consistent-sampling to main July 28, 2026 16:30
@MilanGarnier MilanGarnier added the AI Generated PR largely written by AI tools label Jul 28, 2026
@pr-commenter

pr-commenter Bot commented Jul 28, 2026

Copy link
Copy Markdown

Benchmarks

Comparison

Candidate

Candidate benchmark details

Baseline

Baseline benchmark details

@MilanGarnier
MilanGarnier changed the base branch from main to milan.garnier/otel-consistent-sampling July 28, 2026 16:32
…t drop

dd-trace-rb and dd-trace-go both stamp the rate limiter's effective rate
unconditionally once the limiter is consulted, regardless of allow/drop.
libdatadog only recorded it on drop, diverging from other tracers.
@MilanGarnier
MilanGarnier force-pushed the milan.garnier/otel-rate-limiter-effective-rate branch from 286da37 to c01edb8 Compare July 28, 2026 16:34
@MilanGarnier
MilanGarnier changed the base branch from milan.garnier/otel-consistent-sampling to main July 28, 2026 16:34
@datadog-prod-us1-3

datadog-prod-us1-3 Bot commented Jul 28, 2026

Copy link
Copy Markdown

Pipelines  Tests

⚠️ Warnings

🚦 2 Pipeline jobs failed

DataDog/apm-reliability/libdatadog | benchmarks 1/2   View in Datadog   GitLab

DataDog/apm-reliability/libdatadog | compute_impacted_crates   View in Datadog   GitLab

ℹ️ Info

No other issues found (see more)

🧪 All tests passed
❄️ No new flaky tests detected

🎯 Code Coverage (details)
Patch Coverage: 60.00%
Overall Coverage: 74.65% (+0.02%)

Useful? React with 👍 / 👎

This comment will be updated automatically if new data arrives.
🔗 Commit SHA: c01edb8 | Docs | Datadog PR Page | Give us feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants