Repository navigation
Conversation
Typing analysisThis PR does not change typing compared to the base branch. |
|
✅ All CI checks and tests passed. 🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: c2323a2 | Docs | View more details | Give us feedback! |
BenchmarksBenchmark execution time: 2026-09-04 18:25:06 Comparing candidate commit 5abe415 in PR branch Found 1 performance improvements and 0 performance regressions! Performance is the same for 50 metrics, 1 unstable metrics.
|
91c7867 to
249ff7d
Compare
…ing gate Coordinated sampling for Live Debugger snapshots: one emit/drop decision per execution unit (active APM trace, or a task-scoped correlation id read from fiber-local storage), shared across every probe in the unit so related snapshots stop fragmenting under independent per-probe sampling. Within an emitting unit a per-probe-per-span cap bounds each probe to one snapshot. The decision reuses the probe's existing rate limiter (first probe in a unit decides, siblings inherit), introducing no new sampling-rate wire or config contract. Decision and cap state live in bounded LRU maps. Includes RBS signatures and unit tests.
…apshots
Gap 1 (process identity): emit the per-process runtimeId in the snapshot
envelope, distinguishing snapshots from before and after a restart inside the
same container. Same value already sent in probe status diagnostics.
envelope-source: emit trace_id_source ("apm" | "task" | "none") so a consumer
knows when the trace id in the envelope is a valid join key to APM. Mirrors the
correlation gate's tier ordering using in-process reads only.
Wire Correlation into Component and Instrumenter. Replace the two per-probe rate-limiter checks (method-probe and line-probe paths) with a single emit? gate that delegates to Correlation#gate, so probes in one execution unit share the emit/drop decision and a per-probe-per-span cap bounds each probe. emit? fails open: when correlation is absent or the gate raises (outside propagate_all_exceptions), it falls back to the probe's own rate limiter so a correlation bug cannot silence all snapshots. Adds an integration test covering tier-1 coordination, per-span cap, tier-2 task units, runtime id on the wire, and fail-open.
Extract execution-unit identity (APM trace / task boundary / individual hit) into Datadog::DI::ExecutionUnit, the single source of truth for tiering used by both the sampler and the snapshot envelope. Correlation becomes a sampler with one responsibility: emit?(probe, unit) decides once per unit, shares the decision across sibling probes, and caps each probe once per scope. Drops the unused decisions_made/last_decision_at counters.
Correlation used none of settings/logger/telemetry; remove them and their attr_readers rather than document dead surface. Constructor now takes only max_entries. Add brief describing docstrings to the constructors.
The requirements decision permits only existing tracer context and prohibits new context mechanisms; with no active context the hit is not correlated. Remove the task tier from ExecutionUnit: TASK_KEY, the task branch in .current, and the .bracket/.open/.close boundary API (a new fiber-local context mechanism). Units now resolve to :apm (active trace) or :none. Correlation and the notification builder need no change: Correlation is unit-agnostic (nil key -> per-probe, i.e. not correlated), and trace_id_source serializes the unit source, which now yields only "apm" or "none". Drop the tier-2 tests and RBS entries.
Replace untyped in the ExecutionUnit and Correlation signatures with concrete types: unit key/scope are Integer? (trace/span ids); the decision and cap maps are Hash[Integer, bool] and Hash[Integer, Set[String]]; probe ids are String; evict is generic over [K, V]. Bind unit.key/unit.scope to locals in emit? so the nil-guard narrows them to Integer for the map operations. Steep clean, DI correlation specs green.
Rename the unit that groups related probe hits from 'execution unit' to 'sampling unit': class ExecutionUnit -> SamplingUnit (file, sig, spec), the Correlation param unit -> sampling_unit, the @unit_decisions map and unit_decision method -> @sampling_unit_decisions / sampling_unit_decision, and the prose throughout. Also drop a stale 'task' mention left in the trace_id_source envelope comment. Behavior unchanged. Steep clean, DI correlation specs green.
Non-capturing (log-only) probes bypass the correlation gate and keep
their own per-probe rate limit; only snapshot/expression-capturing
probes share a sampling-unit decision and the per-span cap. Matches the
current RFC scope ("every capturing/snapshot probe").
Adds Probe#capturing? and gates Instrumenter#emit? on it.
Drops the author-facing trace_id_source field ("apm"|"none") and its
builder method; runtimeId and dd.trace_id/dd.span_id are unchanged. The
require of sampling_unit is dropped from the builder (SamplingUnit is
still used by Instrumenter).
A token bucket that permits consumption below zero and refills the deficit over time at its configured rate. Coordinated snapshot sampling uses it as the process-wide GLOBAL budget: a trace that has started emitting keeps emitting after the per-second budget is spent, and new traces are held off until the balance recovers. rate and max_tokens are constructor parameters.
Replace the inherited-decision + per-span cap model with the mechanism in the Casual Correlation requirements: - SamplingUnit carries only the trace key (drop span scope and source). - Correlation gates the first capturing probe in a trace (the top probe) on a process-wide TOP rate limit (10/s, non-borrowing) and GLOBAL rate limit (20/s, borrowing); on pass it emits and seeds per-trace counters. - Correlated probes are bounded by per-trace per-probe (5) and all (20) emission counters and consume GLOBAL on emit. - A top probe that fails GLOBAL or TOP marks the trace starved so every correlated probe in it drops. - No active trace falls back to the probe's own rate limiter. - Rates and budgets are constructor-overridable for tests.
Cover the wired behaviors under production limits: a nested capturing chain emits together sharing the trace id; one probe is bounded to the per-probe counter (5) within a trace; non-capturing probes bypass coordination; a capturing probe with no active trace keeps its own rate limit; runtimeId rides the snapshot; the gate fails open. TOP/GLOBAL limits and starvation are covered in the unit spec where the limits are constructed small.
BorrowingTokenBucket#rate multiplies by elapsed time, so its rate (and the GLOBAL rate Correlation forwards to it) must be Float | Integer, matching TokenBucket; Numeric has no multiplication in the core RBS. Clears the rake typecheck error.
Drop the private attr_reader comments that restated the RBS and names, the editorial phrasing in the class and constant docstrings, and the usage narrative on BorrowingTokenBucket.
The class decides whether a capturing Live Debugger probe hit emits a snapshot (a coordinated sampler). "Correlation" named the domain concept, not the responsibility, and collided with the pre-existing Datadog::Tracing::Correlation (trace-log correlation identifiers), which would lead a reader to expect the same meaning here. The DI sibling classes are agent nouns (Redactor, Serializer, Instrumenter); this one now follows that pattern. Renames the class, its file, sig, and unit spec, and the wiring accessor/keyword/ivar `correlation` to `correlation_sampler` for accessor consistency with the new class name. Verified: rspec (correlation_sampler_spec, sampling_unit_spec) green; rubocop, standard, rbs:stale, rbs:missing, steep:check clean under Ruby 4.0.6. The correlation_integration_spec was not run locally: the libdatadog_api native extension fails to compile against the vendored libdatadog-38.0.0 headers (trace_exporter.c), a pre-existing build issue unrelated to this rename; the two edited identifiers there are covered by steep and rubocop.
The three private helpers dispatched from #emit? had names that did not convey purpose from their signatures: `per_probe`, `top`, and `correlated` each returned a Boolean emit decision but read as nouns/adjectives. Rename them to predicates naming the case they decide: per_probe -> emit_uncorrelated? (hit with no sampling unit) top -> emit_first_in_unit? (first capturing probe in the unit) correlated -> emit_correlated? (subsequent probe in an established unit) Updates call sites in #emit? and the sig file. Verified: rspec (correlation_sampler_spec, sampling_unit_spec) green; rubocop, rbs:stale, steep:check clean under Ruby 4.0.6.
The initializer parameter `all` and the reader `all` were bare determiners that did not convey their roles from the signature: the parameter is a starting budget, the reader is the live remainder. Rename the parameter to `all_budget`, the ivar and reader to `all_remaining`, so each name states what it holds. Updates the sig file and unit spec readouts. Verified: rspec (correlation_sampler_spec) green; rubocop, rbs:stale, steep:check clean under Ruby 4.0.6.
Add a docstring to the NONE sentinel constant. Replace the prose arrow undefined->defined with words in the per-call defined? rationale.
…rate Document that available_tokens returns the balance as of the last refill and does not refill first. Add a context covering the negative-rate guard in BorrowingTokenBucket#initialize.
Make correlation_sampler a required keyword argument of Instrumenter (Component always injects it) and pass it explicitly in instrumenter_spec. Add @param/@return YARD tags to emit? and state the capturing-probe scope of the cross-tracer RFC's snapshot correlation.
Move CorrelationIntegrationTestClass into its own file with stable line numbers and add a capturing line-probe context that drives the emit? gate through line_trace_point_callback with a live sampler, asserting the per-probe counter bounds snapshots within a trace.
Instrumenter#initialize now requires correlation_sampler, so the method-probe dispatch, circuit-breaker, and remote specs pass it explicitly as nil to preserve their uncoordinated behavior.
Instrumenter#initialize now requires correlation_sampler. The di_instrument benchmark (exercised by validate_benchmarks_spec) and the dead instrumenter let in probe_notification_builder_spec both construct Instrumenter without it; pass correlation_sampler: nil to preserve their uncoordinated behavior. The notification-builder let also gains the serializer and logger positionals it was already missing.
The standard/lint CI job (bundle exec rubocop -D) reported two autocorrectable offenses introduced by this PR. lib/datadog/di/instrumenter.rb: Style/KeywordParametersOrder — the required keyword `correlation_sampler:` preceded the optional `code_tracker: nil` in Instrumenter#initialize. Reorder so required keyword parameters come before optional ones. Callers pass these keyword arguments by name, so the reorder is behavior-preserving. The @PARAM tags in the docstring are reordered to match the new signature order. spec/datadog/di/integration/correlation_integration_spec.rb: Style/RescueModifier — the `Object.send(:remove_const, ...) rescue nil` idiom was copied from instrumentation_spec.rb without that file's `# rubocop:disable Style/RescueModifier` directive. Add a targeted disable/enable pair around the line, matching the convention used in the sibling spec. Verified locally on Ruby 4.0 with gemfiles/ruby-4.0.gemfile: `bundle exec rubocop -D` reports 0 offenses across 1966 files and `bundle exec rake standard` passes.
|
@codex review |
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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved sampling-limit issues and required changelog and assertion updates remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (1)
What changed in this PR
Adds coordinated Live Debugger snapshot sampling by APM trace, with bounded budgets, global borrowing, and runtime ID validation.
Changes:
- Introduces sampling-unit and correlation-sampler implementations.
- Wires coordinated sampling and borrowing token buckets into DI instrumentation.
- Adds unit, integration, RBS, benchmark, and UUID assertion coverage.
| File | Summary | Review notes |
|---|---|---|
spec/datadog/di/sampling_unit_spec.rb |
Sampling-unit tests | — |
spec/datadog/di/remote_spec.rb |
Instrumenter setup updates | — |
spec/datadog/di/method_probe_dispatch_semantics_spec.rb |
Instrumenter setup updates | — |
spec/datadog/di/integration/probe_notification_builder_spec.rb |
Runtime ID payload coverage | — |
spec/datadog/di/integration/everything_from_remote_config_spec.rb |
Runtime UUID assertions | Nit (1): tighten the remaining runtimeId assertion. |
spec/datadog/di/integration/correlation_integration_test_class.rb |
Correlation integration fixtures | — |
spec/datadog/di/integration/correlation_integration_spec.rb |
End-to-end correlation tests | — |
spec/datadog/di/instrumenter_spec.rb |
Instrumenter sampling coverage | — |
spec/datadog/di/instrumenter_circuit_breaker_spec.rb |
Constructor updates | — |
spec/datadog/di/correlation_sampler_spec.rb |
Sampler budget tests | — |
spec/datadog/core/rate_limiter_spec.rb |
Borrowing bucket tests | — |
sig/datadog/di/sampling_unit.rbs |
Sampling-unit signatures | — |
sig/datadog/di/instrumenter.rbs |
Instrumenter signatures | — |
sig/datadog/di/correlation_sampler.rbs |
Sampler signatures | — |
sig/datadog/di/component.rbs |
Component signatures | — |
sig/datadog/core/rate_limiter.rbs |
Rate-limiter signatures | — |
lib/datadog/di/sampling_unit.rb |
Resolves trace-based sampling units | — |
lib/datadog/di/probe.rb |
Capturing documentation updates | — |
lib/datadog/di/probe_notification_builder.rb |
Adds runtime identity to snapshots | — |
lib/datadog/di/instrumenter.rb |
Applies coordinated sampling gates | Moderate (4): prevent the legacy limiter from fragmenting correlated snapshots. |
lib/datadog/di/correlation_sampler.rb |
Coordinates trace-level sampling | Moderate (2): honor first-probe budget admission; Nit (1): add the required changelog fragment. |
lib/datadog/di/component.rb |
Wires the sampler into DI | — |
lib/datadog/core/rate_limiter.rb |
Adds borrowing token bucket | Moderate (1): require at least one token before admitting a new top probe. |
benchmarks/di_instrument.rb |
Updates benchmark wiring | — |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| budget = TraceBudget.new(per_probe_budget: per_probe_budget, all_budget: all_budget) | ||
| budget.admit(probe.id) | ||
| store(key, budget) | ||
| true |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 02584ca08e
ℹ️ 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".
|
|
||
| rate_limiter = probe.rate_limiter | ||
| admitted = continue && (rate_limiter.nil? || rate_limiter.allow?) | ||
| admitted = continue && emit?(probe) |
There was a problem hiding this comment.
Remove the legacy limiter from correlated snapshots
When a capturing probe fires inside an active trace, emit? already consumes the correlation sampler's borrowing GLOBAL budget, but the immediately following global_snapshot_rate_limiter check applies a second, non-borrowing 20/s gate. After earlier correlated or uncorrelated snapshots exhaust that legacy bucket, an admitted trace can emit its first snapshot while sibling snapshots are dropped—or have its top snapshot dropped after its correlation budget was established—defeating the promised all-or-nothing correlation and consuming correlation counters for snapshots never sent.
Useful? React with 👍 / 👎.
| # Sampling gate that coordinates capturing Live Debugger probe hits within | ||
| # one trace and bounds total snapshot volume with process-wide budgets. | ||
| # |
There was a problem hiding this comment.
Add the customer-facing changelog fragment
This introduces a customer-visible change to which Live Debugger snapshots are emitted, but the commit adds no non-example fragment under unreleased/, so the release tooling cannot include the change in customer release notes. Add the required fragment describing coordinated snapshot sampling.
AGENTS.md reference: AGENTS.md:L169-L169
Useful? React with 👍 / 👎.
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: 02584ca08e
ℹ️ 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.
| correlation_sampler = self.correlation_sampler | ||
| if correlation_sampler && probe.capturing? | ||
| begin | ||
| return correlation_sampler.emit?(probe, SamplingUnit.current) |
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Security: Enforce per-probe limits for traced captures
When DI is enabled with a capturing probe on attacker-reachable code, keep enforcing that probe's configured rate on traced requests. This early return skips the probe limiter below, so fresh request traces are admitted at the TOP rate (10/s) even when snapshotsPerSecond is 0 or at its documented 1/s default; a caller can force up to 10× more snapshot serialization in the host process. The later 20/s process cap, capture/queue bounds, and 0.5s per-execution circuit breaker limit but do not remove this bounded DoS amplification. Combine correlation admission with the per-probe limit.
Useful? React with 👍 / 👎.
…cket TokenBucket and BorrowingTokenBucket duplicated constructor validation, state initialization, and refill math. Both now include a RefillableBucket module holding the shared pieces; each class keeps only its admission policy. Field-specific ArgumentError matchers replace the generic /bad/ matchers and redundant clock re-stubs are dropped.
Instrumenter#emit? and CorrelationSampler#emit_uncorrelated? both implemented the nil-means-allow consult of the probe's rate limiter. Probe#own_rate_limit_allows? is the single implementation; both sites call it. The sampler spec doubles stub the predicate.
…e seeded budget The comment claimed dd-trace-java budgets only per-probe-per-trace with no process-wide gates; dd-trace-java coordinates through a debug session with per-probe-per-trace budgets and a process-wide global sampler. The comment now states the actual divergence. emit_top? returns TraceBudget#admit's value and stores the seeded budget first, so a constructed all_budget of zero reports the drop it records. The TraceBudget initialize signature order now matches the .rb order.
Instrumenter#emit? admitted correlated chains past the borrowing GLOBAL budget and then run_method_probe and line_trace_point_callback applied the hard 20/s GLOBAL_SNAPSHOT_RATE_LIMIT through probe_global_rate_limiter, dropping exactly the borrowed snapshots that keep a chain intact. The hard limiter now lives in emit? behind global_rate_limit_allows? and applies to uncorrelated, non-capturing, coordination-disabled, and fail-open hits; correlated hits are bounded by the sampler's borrowing GLOBAL budget alone. The Instrumenter initialize signature order now matches the .rb order.
The line-probe context removed the reloaded test class with a rescue modifier that swallowed every exception class. The guard now states the intended condition.
…on branches The class and per-call docstrings described the unit by what it lacks; they now state the NONE-sentinel resolution and the load-order reason. Tests cover the no-id active trace and the undefined Datadog::Tracing branches.
* origin/master: (177 commits) Fix flaky: native transport fork drain spec (#6380) Fix symbol extraction for instrumented methods (#6339) Mark trace transport provenance at export [🤖] Lock Dependency: https://github.com/DataDog/dd-trace-rb/actions/runs/36876578516 Make standardrb happy Add changelog entry [PROF-16128] Profiling: Fix native extension build breaking when trying to log % Dependency inject args into configure_libdatadog docs: add instructions for updating system-tests commit SHA in CI configurations (#6405) ci: adopt LLM validation gate for AGENTS.md Add stable OTel environment attribute mapping (#6261) Revert OpenFeature provider activation and dependent lifecycle changes (#6403) Declare FLAKY_BENCHMARKS_REGEX and document benchmarks CI (#6369) feat(openfeature): harden provider lifecycle and delivery recovery (#6323) feat(openfeature): activate delivery during provider initialization (#6295) feat(openfeature): add agentless configuration delivery (#6294) feat(openfeature): track configuration delivery readiness (#6292) feat(openfeature): add agentless configuration and source resolution (#6291) adding TODO comment for otlp export Change TAG_SDK_OTLP_EXPORT type from ::String to String ...
The coordinated-sampling test plan requires TOP exhaustion / starved trace coverage at the integration tier: after TOP_RATE traces each establish an emitting unit, the next trace's capturing probes all drop. The correlation integration spec covered the coordinated chain, the per-probe counter, and the hard-limit bypass, but not TOP exhaustion, which was only covered at unit level. The test drives one capturing-probe hit per trace for TOP_RATE traces (each hit is its trace's top probe and consumes one TOP token; one emit per trace also keeps GLOBAL positive, so TOP is the gate the starved trace fails), then drives one more trace through a nested call and asserts it produced no snapshots. The time provider is frozen so the process-wide buckets do not refill while the traces are driven.
* origin/master: (67 commits) Add missing backtick in failed expectation message Fix semantic conflict in collectors_cpu_and_wall_time_worker.c Avoid raising due to monotonic clock failure during `on_thread_begin_event` Minor: Add note about `during_gc` still being set Register .agents/ in CODEOWNERS under ruby-guild Gather stats after `prepare_serialize` Minor: Small adjustments to profiling_gc Skip `prepare_serialize` spec when inside valgrind Add changelog entry for #6377 [NO-TICKET] Profiling: Ensure during_sample is respected when writing samples (2/n) Run document lint in the engine container Install document lint gem into a writable GEM_HOME Restore dalli ignore entries now that audit output is validated Rename lint job to document-lint and extract its script [🤖] Update datadog gem version to 2.45.0.dev Add simple-english lint job for dependency audit doc Rewrite dependency audit doc in plain English Extract dependency audit guide to its own doc Document audit job skip behavior and handle-cve skill in dev guide Document standalone-only run of dependency audit spec ...
…hanges The changelog/check job failed on this PR with "This pull request modifies CHANGELOG.md directly" although the branch never touches CHANGELOG.md. Root cause: on pull_request events actions/checkout checks out GitHub's test-merge commit, whose tree carries the base branch's current CHANGELOG.md, and github.event.pull_request.base.sha can lag the base branch tip by days (observed here: the event carried a 2026-10-02 base SHA while the run merged into a 2026-10-07 master tip). The script diffed from the recorded base SHA to the merge checkout, so the 2.44.0 release regenerating CHANGELOG.md on master was reported as a PR change. Any PR open across a release is falsely rejected, and the branch has no remedy: nothing on the branch touches CHANGELOG.md. Fix: pass the base ref and the PR head SHA from the workflow and diff origin/<base_ref>...<PR head SHA>, which counts only the PR's own changes (the merge base of base tip and PR head is the PR's fork point) and stays correct when the branch merges master after a release, when the base SHA is stale, and when CI checks out the PR head because the test merge conflicts. Task: fix the defect reported by failing CI on PR #6118 (this check is the failing job). Passing the base ref and PR head SHA from the workflow instead of relying on the test-merge commit's parent order is my design decision. No changelog fragment: internal CI tooling. Verified by reproducing the CI state locally (test merge of the PR head into the current master tip, with the run's recorded base SHA and branch name): the old script exits 1 (reproduces the failure), the fixed script exits 0. A simulated branch that modifies CHANGELOG.md is still rejected; a bump_to_version_* branch is rejected when it does not modify CHANGELOG.md and accepted when it does; a changelog_fix_* branch is accepted; a branch that merged post-release master is accepted; missing BASE_REF or PR_HEAD_SHA errors. shellcheck and yamllint clean. actionlint is not available in this environment and was not run.

What does this PR do?
Adds coordinated sampling for Live Debugger snapshots:
/ Capturing probes firing within the same APM trace now share one sampling decision - either all are captured or none are. This makes it possible to follow a single logical request/operation over time through the probes.
/ Rate limits are reworked to keep approximately the same overall load on customer applications.
Motivation:
Previously there was no way to guarantee capture of probes that execute within e.g. one HTTP request to an application. This complicated debugging especially when used by LLMs.
Change log entry
Yes. Live Debugger snapshots now capture all or no probes within one APM trace.
Additional Notes:
How to test the change?