[APMSVLS-469] feat(traces): rescue errored traces via agent-side error sampler - #1320
Open
lucaspimentel wants to merge 13 commits into
Open
[APMSVLS-469] feat(traces): rescue errored traces via agent-side error sampler#1320lucaspimentel wants to merge 13 commits into
lucaspimentel wants to merge 13 commits into
Conversation
|
Member
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds an agent-side error sampler to the Bottlecap trace processing pipeline so that errored traces that were auto-dropped (P0/AutoDrop) on the lambda_extension_compute_stats path can be “rescued” up to a configurable TPS budget, improving error visibility while still keeping non-errored P0 traces dropped and honoring explicit user drops.
Changes:
- Introduces an
ErrorsSamplerintoServerlessTraceProcessorand uses it to selectively retain erroredAutoDropchunks, stamping_dd.errors_sron rescued roots. - Adds config surface area for
DD_APM_ERROR_TPSandDD_APM_EXTRA_SAMPLE_RATE(defaults + env/YAML tests). - Wires the sampler into the runtime entrypoint and updates unit/integration tests and third-party licensing/deps to include the new shared sampler crate.
Reviewed changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| bottlecap/src/traces/trace_processor.rs | Implements the rescue decision path for errored AutoDrop chunks and adds unit tests for the new behavior. |
| bottlecap/src/config/mod.rs | Adds apm_error_tps / apm_extra_sample_rate configuration fields and parsing/tests. |
| bottlecap/src/bin/bottlecap/main.rs | Constructs and injects the error sampler into the trace processor using config values. |
| bottlecap/tests/apm_integration_test.rs | Updates the integration pipeline wiring to include the new sampler field. |
| bottlecap/src/lifecycle/invocation/processor.rs | Updates test constructions of ServerlessTraceProcessor to provide an error sampler. |
| bottlecap/Cargo.toml | Adds the datadog-agent-trace-sampler dependency and updates serverless-components rev pins. |
| bottlecap/Cargo.lock | Locks the new sampler crate and updates the serverless-components sources to the new rev. |
| bottlecap/LICENSE-3rdparty.csv | Adds third-party license metadata for datadog-agent-trace-sampler. |
On the lambda_extension_compute_stats path, the extension drops every trace marked P0 (priority <= 0) after computing its stats. This adds an error sampler that gives those dropped traces a second look: errored traces are kept (rescued) up to DD_APM_ERROR_TPS traces/sec (default 10), distributed fairly across trace signatures, with _dd.errors_sr stamped on the rescued root span. Non-errored P0 traces are still dropped, and stats still count all traces. This guarantees error visibility even under aggressive sampling. Ports the Go trace agent's ScoreSampler/ErrorTPS behavior via the shared, dependency-free datadog-agent-trace-sampler crate. New config: - DD_APM_ERROR_TPS (default 10.0; 0 disables the rescue) - DD_APM_EXTRA_SAMPLE_RATE (default 1.0) 🤖
Errored traces were only rescued from a drop decision when the root span itself carried the error, so a trace whose failure happened deeper (for example a failed downstream call that the handler caught) was still dropped. Now an error anywhere in the trace makes it a rescue candidate, matching the Datadog Agent. 🤖
Traces dropped on purpose, either by a tracer sampling rule or by an explicit MANUAL_DROP, were being fed to the error sampler and could be sent to Datadog anyway when they contained an error. Only traces dropped by automatic sampling are now rescue candidates, matching the Datadog Agent. 🤖
The error sampler's per-signature rate limits were keyed on the extension's own DD_ENV, so when that was unset every trace shared one empty env and distinct services competed for the same budget. The env the tracer reported with the trace is now used instead, matching the Datadog Agent. 🤖
A panic while the error sampler's lock was held poisoned the mutex, which silently disabled error-trace rescue for the rest of the sandbox's life. Recover through poisoning instead: the sampler holds only rolling-window counters, so a partially updated bucket costs far less than losing the feature entirely. 🤖
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
The disabled check re-derived "sampler is off" from apm_error_tps at the call site, a second definition of a condition the sampler already knows. It now asks the sampler via the new is_disabled(), hoisted above the payload loop so it costs one lock per flush instead of a check per chunk. Repins the four serverless-components deps to pick up is_disabled(), which also brings in non-finite client sample rate handling. 🤖
lucaspimentel
force-pushed
the
lpimentel/add-trace-error-sampler
branch
from
August 6, 2026 21:57
7a0f64f to
328cf05
Compare
Bump the serverless-components pin to 54e570ae, which adds the dual-mode ErrorSamplerMode (AlwaysKeep | RateLimited) to datadog-agent-trace-sampler and makes `mode` a required field on ErrorSamplerConfig. The rev also carries a fix for non-finite client sample rates on the error sample rate path. Hardcode the production sampler to AlwaysKeep: Lambda's per-invocation trace volume is low, so the RateLimited budget rarely binds, and freeze/thaw breaks its 30s wall-clock window. Wiring the mode through config is deferred to a follow-up. Correct the doc comments on apm_error_tps, apm_extra_sample_rate, and ServerlessTraceProcessor::error_sampler, which described RateLimited behavior that no longer runs in production. Refs APMSVLS-469
Replace apm_error_tps and apm_extra_sample_rate with a single apm_error_sampler_enabled toggle (DD_APM_ERROR_SAMPLER_ENABLED). Both replaced knobs were introduced earlier on this branch and never released, so there is no compatibility constraint. Neither carries its advertised meaning in AlwaysKeep mode: extra_sample_rate is ignored outright, and error_tps degrades to an on/off switch, so DD_APM_ERROR_TPS=25 and =1 behave identically. Exposing a rate cap that is not enforced is worse than not exposing one. Both return, with their real semantics, when RateLimited is wired up. Default false while the feature rolls out as opt-in; the plan is to flip it once it has soaked. AlwaysKeep derives its disabled flag from target_tps <= 0.0, so the boolean maps onto 1.0 / 0.0 and the disabled path short-circuits before any SpanView is built. Refs APMSVLS-469
Build the shipped error sampler (AlwaysKeep, enabled by apm_error_sampler_enabled) in one place so tests exercise the same configuration that ships, and cover the disabled default with a test. A failed clock read no longer aborts the extension, and the clock is only read when error rescue is enabled. In AlwaysKeep mode the sampler ignores span contents, so only the root span view is built instead of one per span. 🤖
The float_cmp allow no longer matches any comparison in the test module and would mask a real one. 🤖
Condense the doc and inline comments added with the error sampler: drop roadmap notes, references to prior behavior, and comments that restate the code they sit above. 🤖
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.
Overview
On the
lambda_extension_compute_statspath, the extension drops every trace marked P0 (priority <= 0) after computing its stats. That means an errored trace sampled away by the tracer is invisible in the UI, even though its stats are counted.This adds an agent-side error sampler that gives those dropped traces a second look: errored traces are rescued and
_dd.errors_sris stamped on the rescued root span, so errors stay visible even under aggressive sampling, matching the Go trace agent'sScoreSampler/ErrorTPSbehavior.Which traces are candidates, matching the Go agent:
MANUAL_DROP, are honored and never rescued.The sampling logic itself lives in the shared, dependency-free
datadog-agent-trace-samplercrate added in DataDog/serverless-components#141. It takes primitives in (SpanView/TraceView) and returns aSampleDecision, exposing no protobufSpantype, so consumers pinning different libdatadog revisions can share it.Configuration
DD_APM_ERROR_SAMPLER_ENABLEDfalseFlat env/YAML key (
apm_error_sampler_enabled), unlike the Go agent's nestedapm_config.*form.The crate offers two rescue strategies. This PR ships
AlwaysKeep, which rescues every errored auto-dropped trace: Lambda's per-invocation trace volume is low, and freeze/thaw breaks theRateLimitedstrategy's 30s wall-clock window.RateLimited(a traces/sec budget spread fairly across trace signatures, the Go agent'serrors_per_second) stays unwired, along with theDD_APM_ERROR_TPSandDD_APM_EXTRA_SAMPLE_RATEsettings that only become meaningful with it.When the sampler is disabled, the rescue path is skipped entirely: no span views built, no clock read, chunks drop exactly as they did before this PR.
Blocked on
DataDog/serverless-components#141 must merge first. The four serverless-components pins in
bottlecap/Cargo.tomlcurrently point at that PR's branch rev (54e570a) and need to be repinned to the mergedmainrev before this can land. Draft until then.Testing
New unit tests in
traces/trace_processor.rs:test_error_sampler_rescues_errored_p0_chunks— an errored auto-dropped chunk is rescued and stamped with_dd.errors_sr; a non-errored one is still dropped; an errored chunk marked as an explicit user drop is not rescued.test_error_sampler_rescues_chunk_with_errored_child_span— a trace whose root is fine but whose child span errored is still rescued.test_disabled_error_sampler_drops_errored_p0_chunks— with the shipping default (disabled), an errored P0 trace stays dropped.Also:
apm_error_sampler_enabled.apm_integration_test.rswires the sampler into the processor pipeline, so the existing end-to-end APM assertions run with it enabled.new_error_sampler), so the tests exercise the configuration that ships.cargo test— 567 passed, 0 failed.cargo clippy --all-targetsandcargo fmt --checkclean.🤖