DI: coordinated sampling for Live Debugger snapshots (runtime_id, trace_id_source) - #6118
Draft
p-datadog wants to merge 30 commits into
Draft
DI: coordinated sampling for Live Debugger snapshots (runtime_id, trace_id_source)#6118p-datadog wants to merge 30 commits into
p-datadog wants to merge 30 commits into
Conversation
…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.
p-datadog
added
the
AI Generated
dd-octo-sts
Bot
added
the
debugger
Contributor
Typing analysisIgnored filesThis PR clears 4 ignored files. It increases the percentage of typed files from 49.84% to 50.57% (+0.73%). Ignored files (+0-4)✅ Cleared:Note: Ignored files are excluded from the next sections.
|
|
BenchmarksBenchmark execution time: 2026-08-17 14:13:51 Comparing candidate commit caa8beb in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
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.
Fixes: 1. Add @return [Boolean] YARD tag to Probe#capturing? 2. Replace double() with instance_double() in test specs for interface verification (Probe, RateLimiter, transports, trace, span) 3. Convert BorrowingTokenBucket defaulted positional args to keyword args (max_tokens: rate, consume(size: 1)) 4. Add :: prefix to all core types in correlation_sampler.rbs and sampling_unit.rbs RBS signatures 5. Add explicit nil return to CorrelationSampler#store (void method) 6. Add frozen NONE constant to SamplingUnit to avoid allocation on the no-trace hot path 7. Trim redundant comment in probe_notification_builder.rb Not verified: tests not run (no bundler environment in this context). Proposed fix in: changes address all review findings.
Reverts the frozen NONE constant added to avoid allocation on the no-trace path. The constant name is too terse for the diff.
This reverts commit 7dcf0f0.
NONE = new(nil) sat at the top of the SamplingUnit class body, before def initialize(key). During class-body evaluation the custom initialize is not yet defined, so new(nil) dispatched to the inherited zero-arg initialize and raised: ArgumentError: wrong number of arguments (given 1, expected 0) lib/datadog/di/sampling_unit.rb:12 This ran whenever datadog is required (datadog.rb -> di/boot.rb -> instrumenter.rb -> sampling_unit.rb), so spec_helper failed to load and every test batch aborted on every Ruby that loads DI (2.6+). Ruby 2.5 was spared because DI does not load there. The placement was restored by the revert in 3928129 (Revert "Remove NONE constant from SamplingUnit"). Move NONE = new(nil) to the end of the class body, after initialize is defined. self.current references NONE at call time, so relocation keeps it resolvable. Verified on Ruby 3.2.3: require "datadog" loads, sampling_unit_spec, correlation_sampler_spec, probe_notification_builder_spec, and integration/correlation_integration_spec pass.
The BorrowingTokenBucket subject called described_class.new(rate, max_tokens) with max_tokens positional, but the constructor takes it as a keyword: def initialize(rate, max_tokens: rate). This matches the RBS signature (?max_tokens: ::Numeric) and the production call site CorrelationSampler#initialize -> Core::BorrowingTokenBucket.new(global_rate). Passing two positional arguments raised: ArgumentError: wrong number of arguments (given 2, expected 1) failing all 10 BorrowingTokenBucket examples. On Ruby 2.6+ this surfaced only after the SamplingUnit load-time crash was fixed; on Ruby 2.5 it was already the sole failing batch. Pass max_tokens: max_tokens. Verified on Ruby 3.2.3: rate_limiter_spec passes (both TokenBucket and BorrowingTokenBucket).
sig/datadog/di/sampling_unit.rbs did not declare the NONE constant, so steep reported Ruby::UnknownConstant twice in sampling_unit.rb: the reference returned by self.current and the NONE = new(nil) assignment. This failed the steep/typecheck CI job. Declare NONE: SamplingUnit, matching the singleton instance returned on the no-trace path. Verified on Ruby 4.0.6 with gemfiles/ruby-4.0.gemfile: rake rbs:stale, rbs:missing, and steep:check all pass (No type error detected).
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
2 participants
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.
What does this PR do?
Adds coordinated sampling for Live Debugger snapshots, plus a
runtimeIdfield in the snapshot envelope.runtimeIddistinguishes snapshots emitted before and after a process restart inside the same container.Motivation:
Independent per-probe sampling fragments chains of related snapshots: the system can emit one probe's snapshot while dropping a sibling, parent, or child probe that would explain it, with no signal to the user.
Change log entry
Yes. Live Debugger snapshots from probes in the same trace now share one sampling decision so related snapshots arrive together instead of fragmenting, and each snapshot carries the process runtime id.
Additional Notes:
Probes firing outside an active trace are not correlated.
How to test the change?
Unit and integration tests added