From 9ae11172b0abdd0714c7cb85e4ac3637c18267e2 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Mon, 2 Nov 2026 15:53:34 -0500 Subject: [PATCH 01/15] docs: add temporary plan for DD_APM_ERROR_TPS error rescue budget Planning notes for rate-limited error trace rescue and the deprecation of DD_SERVERLESS_ERROR_SAMPLER_ENABLED. To be deleted before the PR merges. --- PLAN-error-sampler-tps.md | 251 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 251 insertions(+) create mode 100644 PLAN-error-sampler-tps.md diff --git a/PLAN-error-sampler-tps.md b/PLAN-error-sampler-tps.md new file mode 100644 index 000000000..ac1f98a28 --- /dev/null +++ b/PLAN-error-sampler-tps.md @@ -0,0 +1,251 @@ +# Plan: rate-limit Lambda error trace rescue with `DD_APM_ERROR_TPS` and deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` + +> Temporary planning file (untracked). Delete before merging the PR. This file plans **PR 1 of 3**; PRs 2 and 3 are listed under "Follow-up PRs" and are out of scope here. + +## PR series +| PR | Scope | Base | +|---|---|---| +| **1 (this plan)** | `DD_APM_ERROR_TPS` budget, `RateLimited` mode, deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. Rescue gating unchanged: only when the extension computes stats. | `main` | +| 2 | Also run rescue when the tracer computes stats (`Datadog-Client-Computed-Stats: true`). On that path rescue only stamps `_dd.errors_sr` on kept errored P0 chunks; every chunk is still forwarded, so the backend's P0 drop keeps the stamped ones (same model as SCL, APMSVLS-472). | PR 1 | +| 3 | DataDog/datadog-lambda-extension#1358, rebased onto PR 2 and trimmed to filtering sampled-out chunks when the tracer computes stats. | PR 2 | + +Order matters: filtering on the tracer path before rescue exists there would drop errored P0 chunks unrescued. Today tracers still send P0 chunks because `/info` reports `client_drop_p0s: false` (`bottlecap/src/traces/trace_agent.rs:653`). + +## Goal +Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescue always uses the shared `ErrorSamplerMode::RateLimited` sampler with Go agent / SCL trace-agent defaults: rescue is on at 10 TPS when nothing is set, and `<= 0` turns it off. `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` stays parsed but deprecated. + +## Context (verified by reading the code) +- Repo: DataDog/datadog-lambda-extension. Worktree: `/home/lucas/source/datadog/datadog-lambda-extension--lpimentel-error-sampler-rate-limited`. Branch: `lpimentel/error-sampler-rate-limited` at `aa9d3f54`, 1 commit behind `origin/main` (`e36a4709`, #1424, `AGENTS.md` only). Fast-forward to `origin/main` before starting; no line references below change. All Rust code is under `bottlecap/`. Toolchain is 1.93.1. +- `AGENTS.md` (after #1424) says to check `serverless-components` before adding functionality. This plan adds no new shared logic: the sampler is already the shared crate, and the new code is bottlecap's `LambdaConfig` extension field plus its resolution (bottlecap-specific because of the deprecated flag). SCL's `parse_error_sampler_config` lives in `datadog-trace-agent`, not in a shared crate, so there is nothing to reuse. +- Sampler crate `datadog-agent-trace-sampler` is pinned at serverless-components rev `e462105e918904e6b1ad2ebc153ae561cebe5a34` (`bottlecap/Cargo.toml:100`). Do not bump it. Its source is at `~/.cargo/git/checkouts/serverless-components-d7345b4db0f8d2aa/e462105/crates/datadog-agent-trace-sampler/src/{lib.rs,score_sampler.rs}`. What it provides: + - `ErrorSamplerConfig { mode, target_tps, extra_sample_rate }`. `Default` is `RateLimited`, `10.0`, `1.0`. + - `ErrorsSampler` is a public enum with variants `AlwaysKeep { disabled }` and `RateLimited(Box)`. Tests can `matches!` on it. + - `is_disabled()` returns `target_tps <= 0.0`. + - `sample(now_unix_secs, &TraceView)` rotates its buckets only from the `now_unix_secs` argument. `bucket_index` uses `rem_euclid`. A backward clock step (`new_bucket < prev`) zeroes nothing and does not panic. A gap longer than 30s zeroes every bucket. +- Config loading: `datadog-agent-config` builds the YAML source and then the env source. For each source it runs `figment.extract::()` and calls `LambdaConfig::merge_from`. + - Env provider: `Env::prefixed("DD_")`. A source field named `apm_error_tps` therefore maps to `DD_APM_ERROR_TPS` and to the flat YAML key `apm_error_tps`. + - The core `Config` has no `error_tps` field, so the new field does not collide with anything. + - The `merge_fields!` `option:` group copies the value only when the source value is `Some`. +- Figment parses env values as follows (`figment-0.10.19/src/value/parse.rs`): + - `"5"` and `"-1"` become integers, `"2.5"` becomes f64, and `" 7 "` is trimmed. + - `""`, `"abc"`, `"inf"`, `"nan"` and `"1e400"` stay strings. `deserialize_option_lossless::` then warns and returns `None`. + - `"1.0e999"` contains a `.`, so it parses to f64 `inf` and passes `deserialize_option_lossless`. + - YAML `.inf` and `.nan` also arrive as non-finite f64. + - **A finiteness filter is therefore required after deserialization.** +- Logging is initialized before config load (`bottlecap/src/bin/bottlecap/main.rs:116` vs `:164`), so warnings from the deserializer are visible. +- Lints: `bottlecap/src/lib.rs:2-10` denies `clippy::all`, `clippy::pedantic`, `clippy::unwrap_used`, `unreachable_pub` and `missing_copy_implementations`. `clippy::float_cmp` is pedantic; the repo convention is `#[allow(clippy::float_cmp)]` on the test (see `src/metrics/enhanced/lambda.rs:876`). Pedantic also requires `#[must_use]` on value-returning pub fns and backticks around identifiers in doc comments. +- SCL reference: DataDog/serverless-components branch `lpimentel/add-error-sampler`, `crates/datadog-trace-agent/src/config.rs`, function `parse_error_sampler_config`. It starts from `ErrorSamplerConfig::default()` and only overrides `target_tps`. Blank, unparsable and non-finite values warn and fall back to the default. + +## Settled semantics (do not reopen) +1. `DD_APM_ERROR_TPS` (f64, YAML `apm_error_tps`) is the budget. Mode is always `RateLimited` and `extra_sample_rate` is always `1.0`. Bottlecap can never select `AlwaysKeep`. +2. With nothing set, the target is `10.0`, which equals `ErrorSamplerConfig::default().target_tps`. A target `<= 0` disables rescue. +3. `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` is deprecated but still parsed. Warn whenever it is explicitly set. `true` is redundant (target 10). `false` with no usable TPS is a kill switch (target 0). +4. If both are set, TPS wins, and the warning names both values. This holds regardless of which source (YAML or env) each came from. +5. An invalid, blank or non-finite TPS warns and is ignored for that source only. Resolution then uses any remaining valid TPS from a lower-priority source (YAML under env), then the legacy flag, then 10.0. Example: YAML `apm_error_tps: 5` + env `DD_APM_ERROR_TPS=abc` + `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` resolves to 5. +6. Large finite values (for example `1000000000`) are accepted as-is, no cap and no warning (Go/SCL parity). +7. Configuration keys: env `DD_APM_ERROR_TPS` (parity with the Go trace-agent's env var) and flat YAML `apm_error_tps`. Go's YAML key `apm_config.errors_per_second` is intentionally not supported: parity target is env vars, not YAML files. +8. Deprecation warnings do not promise removal (no "will be removed" wording). +9. No "setting has no effect" warning when compute stats is off: whether the tracer computes stats is known only per request, and PR 2 makes rescue apply there too. + +## Constraints +- TDD: write each step's tests first, watch them fail (or fail to compile), then implement. +- Every step must leave the whole crate compiling (`cargo test --lib` builds all lib tests regardless of name filter). Each step therefore updates every caller its type or signature change breaks; do not defer callers to a later step. +- Keep `lambda_extension_compute_stats` gating unchanged (`trace_processor.rs:638`). +- Keep comments concise and leave history out of them. No em dashes in code comments or strings. +- Non-goals: removing `AlwaysKeep` from the shared crate, stats-payload `errorTPS`, changing compute-stats gating (PR 2), tracer-path filtering (PR 3), public docs. The repo has no changelog and no doc mentions `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. +- Do not commit or push unless the user asks. When asked, make two commits (step 1 alone, then everything else). Commit messages must be user-facing. +- The PR description must not include test counts. Use `.github/pull_request_template.md` (`## Overview`, `## Testing`). No Jira ticket in the PR title; mention APMSVLS-469 in the description. + +## Plan + +### Step 1: stale `DD_APM_FEATURES` comment (separate commit) +1. Test first: in `bottlecap/src/config/mod.rs` `lambda_config_tests` (next to `apm_features_comma_separated_list_from_env`, ~:767), add `apm_features_space_separated_list_from_env`. + - Set `DD_APM_FEATURES="some_other_feature error_rare_sample_tracer_drop"`. + - Assert `config.apm_features == ["some_other_feature", "error_rare_sample_tracer_drop"]` and `error_rare_sample_tracer_drop_enabled(&config)`. + - It should pass immediately: it is a regression test, because the pinned crate uses `deserialize_comma_then_space_separated_string` (`datadog-agent-config/src/sources/env.rs:237`). +2. In `bottlecap/src/traces/trace_processor.rs:81-84`, replace "parsed upstream as a comma-separated list ... space-separated values are not supported" with this: `DD_APM_FEATURES` is split on commas when one is present, otherwise on spaces, and matching is exact per entry. +3. Verify: run `cd bottlecap && cargo test --lib apm_features`. All `apm_features*` tests pass. + +### Step 2: config resolution in `bottlecap/src/config/mod.rs` +1. Tests first, in `lambda_config_tests`: + - Replace the three tests at ~:718-742 (`serverless_error_sampler_enabled_*`) with table-driven tests. Each case calls `load(|jail| ...)`, then asserts `config.ext.error_sampler_target_tps()` and whether `config.ext.legacy_error_sampler_warning()` is `Some`. + - Put `#[allow(clippy::float_cmp)]` on the tests, and include the case input in every assert message. + + | Case | Setup | Target | Warning | + |---|---|---|---| + | default | nothing set (also assert `apm_error_tps == None`, `serverless_error_sampler_enabled == None`) | 10.0 | None | + | env int | `DD_APM_ERROR_TPS=5` | 5.0 | None | + | env float | `DD_APM_ERROR_TPS=2.5` | 2.5 | None | + | env padded | `DD_APM_ERROR_TPS=" 7 "` | 7.0 | None | + | YAML | `apm_error_tps: 3` | 3.0 | None | + | env over YAML | YAML `apm_error_tps: 3` + env `DD_APM_ERROR_TPS=4` | 4.0 | None | + | zero | `DD_APM_ERROR_TPS=0` | 0.0 | None | + | negative | `DD_APM_ERROR_TPS=-1` | -1.0 | None | + | large | `DD_APM_ERROR_TPS=1000000000` | 1e9 | None | + | invalid env | `DD_APM_ERROR_TPS` = `""`, `"abc"`, `"inf"`, `"nan"`, `"1.0e999"` (assert `apm_error_tps == None`) | 10.0 | None | + | invalid YAML | `apm_error_tps: .inf`, `apm_error_tps: .nan` (assert `apm_error_tps == None`) | 10.0 | None | + | legacy true | `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true` | 10.0 | Some, contains `deprecated` | + | legacy false | `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` | 0.0 | Some, contains `DD_APM_ERROR_TPS=0` (migration hint) | + | legacy YAML false | `serverless_error_sampler_enabled: false` | 0.0 | Some | + | both, TPS 5 + legacy false | env both | 5.0 | Some, contains `DD_APM_ERROR_TPS=5` and `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` | + | both, TPS 0 + legacy true | env both | 0.0 | Some, contains `DD_APM_ERROR_TPS=0` and `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true` | + | cross-source | YAML `apm_error_tps: 5` + env `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` | 5.0 | Some, names both | + | invalid TPS + legacy false | `DD_APM_ERROR_TPS=abc` + legacy `false` | 0.0 | Some, the kill-switch message, not the both-set one | + | invalid env over valid YAML + legacy false | YAML `apm_error_tps: 5` + env `DD_APM_ERROR_TPS=abc` + env legacy `false` | 5.0 | Some, the both-set message naming `DD_APM_ERROR_TPS=5` | + + Rust formats `5.0_f64` with `{}` as `5`, so `"DD_APM_ERROR_TPS=5"` matches. + + Before finalizing the table, check how figment treats exponent notation without a dot (`DD_APM_ERROR_TPS=1e3`). The `1e400` row above suggests it stays a string and is rejected as invalid. Add a row pinning whatever actually happens, and if it is rejected, list it under Risks so the PR can mention it. + + - Add `default_apm_error_tps_matches_shared_sampler_default`, asserting `DEFAULT_APM_ERROR_TPS == datadog_agent_trace_sampler::ErrorSamplerConfig::default().target_tps`. + - Update `apm_features_error_rare_sample_tracer_drop_from_env` (~:752-764). Replace `assert!(!config.ext.serverless_error_sampler_enabled)` with `assert_eq!(config.ext.error_sampler_target_tps(), DEFAULT_APM_ERROR_TPS)`. Reword its comment to "The feature only widens rescue eligibility; it must not change the rescue budget." + - Verify: `cargo test --lib lambda_config_tests` fails to compile (red). +2. Add `pub const DEFAULT_APM_ERROR_TPS: f64 = 10.0;` near the top of the LambdaConfig section. Its doc comment should say it matches the shared sampler and Go agent default. +3. Add a private deserializer `deserialize_optional_finite_f64<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error>`. + - Call `deser_opt_lossless::<_, f64>(d)?`, then filter out non-finite values. + - When a value is dropped, call `tracing::warn!("Ignoring non-finite DD_APM_ERROR_TPS value {v}")` (fully qualified, since `tracing` is not imported in this file). +4. `LambdaConfig` (~:72-79, default ~:147): + - Change `serverless_error_sampler_enabled` to `Option` and give it default `None`. + - Rewrite its doc: deprecated raw value of `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`; read it only through `error_sampler_target_tps` and `legacy_error_sampler_warning`. + - Add `pub apm_error_tps: Option` with default `None`. Its doc should cover: + - `DD_APM_ERROR_TPS`: the rescue budget in errored traces per second, distributed fairly across trace signatures; `<= 0` disables rescue. + - The budget is per execution environment, so N concurrent sandboxes can rescue up to N times the budget. + - Rescue currently runs only when the extension computes stats (`lambda_extension_compute_stats`). Word this as current behavior; PR 2 widens it. + - The limit is approximate, with no hard ceiling (inherited from the Go agent and SCL). Rates are recomputed on 5s bucket rotation over a 30s window. A cold sampler keeps errors at rate 1.0 until volume in the current bucket exceeds about 5x the budget. After a freeze/thaw gap, counters expire but a throttled signature's rate recovers by at most 20% per evaluation. See Risks for why this wording differs from "unknown signatures sample at 1.0". + - `PartialEq` still derives fine with an f64 field. + - Keep the type change compiling: in `bottlecap/src/startup.rs`, change `!config.ext.serverless_error_sampler_enabled` (:172) to `config.ext.serverless_error_sampler_enabled != Some(true)` and the argument at :181 to `config.ext.serverless_error_sampler_enabled == Some(true)`. These are temporary; steps 3 and 4 replace them. Fix any other non-test reader the compiler reports the same way. +5. `LambdaConfigSource` (~:213-214): keep `serverless_error_sampler_enabled` (still `deser_opt_bool`). Add `#[serde(deserialize_with = "deserialize_optional_finite_f64")] pub apm_error_tps: Option` with a one-line `DD_APM_ERROR_TPS` doc. +6. `merge_from` (~:297): move `serverless_error_sampler_enabled` from the `value:` list to the `option:` list, and add `apm_error_tps` to `option:`. +7. In `impl LambdaConfig` (~:347), add two methods, both `#[must_use]` and `pub`: + - `error_sampler_target_tps(&self) -> f64`: + ``` + match (self.apm_error_tps, self.serverless_error_sampler_enabled) { + (Some(tps), _) => tps, + (None, Some(false)) => 0.0, + _ => DEFAULT_APM_ERROR_TPS, + } + ``` + - `legacy_error_sampler_warning(&self) -> Option`: + - `None` when the legacy flag is `None`. + - (TPS `Some`, legacy `Some(b)`): `"DD_SERVERLESS_ERROR_SAMPLER_ENABLED={b} is deprecated and ignored because DD_APM_ERROR_TPS={tps} is set"`. + - (TPS `None`, `true`): `"DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated and has no effect: error trace rescue is on by default at {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS to change the budget"`. + - (TPS `None`, `false`): `"DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated; set DD_APM_ERROR_TPS=0 to disable error trace rescue instead"`. + - No em dashes, and no removal promise in any message. Add a test asserting none of the three messages contains `removed`. +8. Verify: `cargo build --all-targets` compiles, and `cargo test --lib lambda_config_tests` passes, including `defaults_match_lambda_config_default`. + +### Step 3: sampler construction in `bottlecap/src/traces/trace_processor.rs`, plus every caller +1. Tests first, in `mod tests`: + - `error_sampler_config_is_rate_limited_with_given_tps`: for tps in `[10.0, 2.5, 0.0, -1.0]`, assert `error_sampler_config(tps)` has `mode == ErrorSamplerMode::RateLimited`, `target_tps == tps` and `extra_sample_rate == 1.0`. Use `#[allow(clippy::float_cmp)]`. + - `new_error_sampler_builds_rate_limited`: assert `matches!(*new_error_sampler(10.0).lock().expect("lock"), ErrorsSampler::RateLimited(_))`. For `[0.0, -1.0]` assert `is_disabled()`. For `10.0` assert `!is_disabled()`. + - `error_sampler_survives_clock_jumps`: + - Build `new_error_sampler(DEFAULT_APM_ERROR_TPS)` and a single errored `SpanView` (the crate tests show the shape). + - Call `.sample(now, &trace)` in sequence with now = `1_700_000_000`, `1_699_999_000` (backward), `1_700_100_000` (long gap), `0` (the `UNIX_EPOCH.elapsed()` fallback). + - Use distinct trace ids (`i.wrapping_mul(2_654_435_761)`). + - Assert no panic, and that the call after the long gap returns `SampleDecision::Keep { errors_sr: 1.0 }` (cold, unthrottled sampler). + - `error_sampler_stays_throttled_after_long_gap` (documents inherited behavior, deterministic timestamps): + - Build `new_error_sampler(1.0)`. Drive one errored signature with 200 traces in each of several consecutive 5s buckets (timestamps `t0 + 5*k`, distinct trace ids) so its stored rate drops well below 1.0. + - Then sample once at `t_last + 3600` (gap far beyond the 30s window). + - Assert no panic and that the returned decision, if `Keep`, has `errors_sr < 1.0`: stale counters are cleared, but the stored rate only recovers by at most 1.2x per evaluation (`MAX_RATE_INCREASE`, `score_sampler.rs:21,101-107`). If the hash makes the single decision a `Drop`, use a few distinct trace ids and assert the first `Keep` has `errors_sr < 1.0`. + - `error_sampler_budget_is_shared_across_invocations` (shared processor versus a deliberately reset control): + - Build `create_compute_stats_config()`. + - Shared: one `create_test_processor(&config, new_error_sampler(1.0), false)`, then 10 separate `process_traces` calls (one per simulated invocation). Each call carries 50 single-span errored P0 traces of the same signature with distinct nonzero trace ids (`(call * 50 + i + 1).wrapping_mul(2_654_435_761)`). + - Control: the same 10 batches and ids, but a fresh `create_test_processor(&config, new_error_sampler(1.0), false)` per call. + - Count rescued spans for each with the existing `trace_spans` helper (`trace_processor.rs:1599`). + - Assert `shared > 0` and `shared * 2 < control`. A deterministic simulation of the pinned sampler gives about 29 shared versus about 160 reset, so the margin is wide. A bucket rotation mid-test only lowers `shared` further, since rotation applies the throttled rate. + - Verify the test discriminates: temporarily make the shared run build a fresh sampler per call and confirm the assertion fails, then revert. + - In `test_error_sampler_rescues_errored_p0_chunks` (~:1477), add an assert that the rescued root's `_dd.errors_sr` is `1.0` (cold sampler, low volume). Add `#[allow(clippy::float_cmp)]`. + - In `test_disabled_error_sampler_drops_errored_p0_chunks` (~:1827), loop over `[0.0, -1.0]` to cover the `is_disabled` short-circuit with a negative budget. Change "(the shipping default)" in its doc (~:1825) to "(`DD_APM_ERROR_TPS <= 0`)". + - Verify: tests fail to compile (red). +2. Replace `new_error_sampler(enabled: bool)` (~:119-138): + - Add `#[must_use] pub fn error_sampler_config(target_tps: f64) -> ErrorSamplerConfig`. It returns `ErrorSamplerConfig { mode: ErrorSamplerMode::RateLimited, target_tps, extra_sample_rate: 1.0 }`. Set the fields explicitly so a crate default change cannot alter bottlecap behavior. + - Add `#[must_use] pub fn new_error_sampler(target_tps: f64) -> Arc>`, which wraps `ErrorsSampler::new(error_sampler_config(target_tps))`. + - Rewrite the doc comment. Drop the wrong freeze/thaw claim. Say instead: + - The rolling window is driven by the wall-clock seconds passed on each call, so a freeze/thaw gap expires stale counters. Stored per-signature rates are not reset: a throttled signature recovers by at most 20% per evaluation (inherited from the Go agent). + - `<= 0` disables rescue. + - Point to `LambdaConfig::apm_error_tps` for caveats. +3. Field doc (~:96-100): "a stateful mode (`RateLimited`) would keep its rolling window" becomes "Shared across invocations so the `RateLimited` budget and rolling window span the whole execution environment". +4. Test helper (~:792-796): `enabled_error_sampler()` returns `new_error_sampler(crate::config::DEFAULT_APM_ERROR_TPS)`. Change the doc to "Error sampler with the default budget". +5. In the same file, replace `new_error_sampler(false)` with `new_error_sampler(0.0)` at ~:1763, ~:1830 and ~:1862. +6. Update every other caller in this same step so the crate compiles: + - `bottlecap/src/lifecycle/invocation/processor_service.rs:834` and `bottlecap/src/traces/trace_agent.rs:1148`: `new_error_sampler(false)` becomes `new_error_sampler(0.0)`. + - `bottlecap/tests/apm_integration_test.rs:378`: `new_error_sampler(true)` becomes `new_error_sampler(bottlecap::config::DEFAULT_APM_ERROR_TPS)`. Check the import path against the file's existing `use bottlecap::...` lines. + - `bottlecap/src/startup.rs:180-182`: pass `config.ext.error_sampler_target_tps()` (replaces the temporary step 2 argument). + - `bottlecap/src/lifecycle/invocation/processor.rs:2296,2406,3044,3625` use `enabled_error_sampler()` and need no edit. +7. Verify: `cargo build --all-targets` compiles; `cargo test --lib trace_processor` passes; `ffgrep 'new_error_sampler\((true|false)\)'` on `bottlecap/` returns zero matches. + +### Step 4: startup wiring in `bottlecap/src/startup.rs:159-183` +1. Delete the `DD_APM_ERROR_TPS` "not supported" block (:159-166). Change `use std::{env, sync::Arc};` (:15) to `use std::sync::Arc;`, because `env` has no other use in this file. +2. Insert: + ``` + if let Some(message) = config.ext.legacy_error_sampler_warning() { warn!("{message}"); } + let error_tps = config.ext.error_sampler_target_tps(); + ``` +3. Retarget the rare-sample warning (:172) to `if error_rare_sample_tracer_drop && error_tps <= 0.0`. Message: `"DD_APM_FEATURES includes error_rare_sample_tracer_drop, but error trace rescue is disabled (effective DD_APM_ERROR_TPS {error_tps}): errored traces dropped by the tracer will not be rescued"`. +4. Build the sampler with `trace_processor::new_error_sampler(error_tps)` (reusing the local instead of the step 3 inline call). +5. Do not add a warning for "rescue configured but compute stats off" (settled semantics 9). +6. Verify: `cargo build` succeeds and `cargo test --lib startup` passes. The warning logic itself is covered by the step 2 tests on `legacy_error_sampler_warning` and `error_sampler_target_tps`. + +## Files to Modify +- `bottlecap/src/config/mod.rs`: + - `DEFAULT_APM_ERROR_TPS`, finite-f64 deserializer. + - `LambdaConfig` field type change plus `apm_error_tps`; source field; `merge_from`. + - `error_sampler_target_tps` and `legacy_error_sampler_warning`; tests. +- `bottlecap/src/traces/trace_processor.rs`: + - Stale `DD_APM_FEATURES` doc (step 1). + - `error_sampler_config` and `new_error_sampler(f64)`; docs; test helper; new and updated tests. +- `bottlecap/src/startup.rs`: real resolution, deprecation and precedence warning, retargeted rare-sample warning, drop the `env` import. +- `bottlecap/src/lifecycle/invocation/processor_service.rs`: `new_error_sampler(0.0)`. +- `bottlecap/src/traces/trace_agent.rs`: `new_error_sampler(0.0)`. +- `bottlecap/tests/apm_integration_test.rs`: `new_error_sampler(DEFAULT_APM_ERROR_TPS)`. + +## New Files +None. + +## Final Verification +Run everything from `bottlecap/` and report the pass/fail counts. +1. `cargo fmt --all -- --check` +2. `cargo test --workspace`. Use `cargo nextest run --workspace` if nextest is installed, to match CI. +3. `cargo nextest run --features test-mode --bin bottlecap-test-mode` (or `cargo test --features test-mode --bin bottlecap-test-mode`). +4. `cargo clippy --workspace --all-targets --features default -- -D warnings` +5. `cargo clippy --workspace --all-targets --features default,test-mode -- -D warnings` +6. `cargo clippy --workspace --all-targets --no-default-features --features fips -- -D warnings`. CI runs this. If the FIPS toolchain (aws-lc-fips-sys build deps) is missing locally, say it was skipped and why. +7. Run `ffgrep` for `AlwaysKeep` in `bottlecap/src`: zero matches outside comments. Run `ffgrep` for `not supported by the Lambda extension`: zero matches. +8. Grep the changed lines for `—` (em dash): zero matches. + +## Commits and PR (only when the user asks) +- Commit 1 (step 1 only): `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` +- Commit 2 (steps 2-4): `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS`. Body: + - Rescue is on by default at 10 errored traces/s per execution environment when the extension computes stats. + - `DD_APM_ERROR_TPS<=0` disables it. + - `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` is deprecated and logs a warning. +- PR title: `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS` (no ticket). +- `## Overview`: + - Follow-up to APMSVLS-469 (the bottlecap error sampler) and part 1 of 3; name PRs 2 and 3 briefly. + - The semantics table. + - Env parity with the Go trace-agent's `DD_APM_ERROR_TPS`; YAML key is flat `apm_error_tps`. + - The behavior change: compute-stats users who never set the flag now get rescue at 10/s. + - The caveats: per-environment budget, compute-stats path only, approximate limit with no hard ceiling, inherited from Go/SCL. + - `_dd.errors_sr` can now be below 1.0 under load. +- `## Testing`: describe what is covered. Do not include test counts. + +## Risks +- **The settled caveat wording conflicts with the code.** The caveat says "unknown signatures sample at 1.0 during the first 30s". That is not exactly what happens. `Sampler::default_rate` (`score_sampler.rs`, `fn default_rate`) returns `min(1, target_tps / (max_bucket_count / 5))`, counted after the current trace, so unknown signatures are throttled even in the first bucket once volume passes 5x TPS. "No hard ceiling" is still true: each decision is a trace-id hash, and keeps grow roughly logarithmically within a bucket. Use the wording from step 2.4 and point this out to the user rather than repeating the original phrasing. +- **YAML key divergence (settled).** The Go agent's YAML key is believed to be `apm_config.errors_per_second` (inferred, not verified). Bottlecap uses flat `apm_error_tps` by decision: parity target is env vars, not YAML. State it in one line in the PR. +- **Flaky-test risk.** The shared-budget test uses the real wall clock through `process_traces`, and a 5s bucket boundary can fall mid-test. Comparing against a reset control (`shared * 2 < control`) tolerates that, because rotation only throttles the shared run further. Do not assert exact counts. +- **Throttled recovery after freeze/thaw (inherited).** After a long gap, stale counters are cleared but a throttled signature's stored rate only rises 1.2x per evaluation, and it also holds `lowest_rate` (the default for unknown signatures) down. A thawed sandbox can therefore under-rescue for a while after a past burst. Same as the Go agent and SCL; document it in the `apm_error_tps` doc and the PR, do not deviate. +- **Legacy-only YAML users.** A user with `serverless_error_sampler_enabled: true` and no TPS keeps the same on/off result, but rescue is now rate-limited instead of keep-all. This is intended; state it in the PR. +- **Duplicate warnings.** The non-finite warning in the deserializer can fire once per source (YAML and env). That is acceptable at startup. +- **Lints.** Clippy pedantic needs `#[must_use]` on the new pub fns and backticks around identifiers in docs (`doc_markdown`). Use `.expect("...")`, not `.unwrap()`, in new tests unless the module already has `#[allow(clippy::unwrap_used)]` (the config test module does; the trace_processor test module relies on `expect`). + +## Follow-up PRs (out of scope for this plan; plan each in its own session) + +### PR 2: rescue when the tracer computes stats +- Base: PR 1's branch (or `main` after PR 1 merges). +- Run the error sampler when either the extension or the tracer computes stats (resolve stats ownership once in `process_traces`, as #1358 does today). +- Tracer-owned path: do not drop any chunk. For errored P0 chunks (plus negative-priority non-manual drops under `error_rare_sample_tracer_drop`), call the sampler and stamp `_dd.errors_sr` on `Keep`; forward everything. The backend's P0 drop then keeps the stamped chunks. +- Extension-owned path: unchanged from PR 1 (rescue + drop). +- Update the `apm_error_tps` doc ("rescue runs when the extension or the tracer computes stats"). +- Source material: the rescue-widening half of #1358 (`lpimentel/filter-sampled-traces-by-stats-owner`). + +### PR 3: filter sampled-out chunks when the tracer computes stats +- Rebase DataDog/datadog-lambda-extension#1358 onto PR 2 so it only adds dropping of sampled-out chunks on the tracer-owned path (rescued chunks kept). Keep its review history; update its title and description. From ac54c95aea4d0a2098121126dfb267e27e9da6d8 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Mon, 2 Nov 2026 17:11:57 -0500 Subject: [PATCH 02/15] docs(bottlecap): DD_APM_FEATURES accepts space-separated values --- bottlecap/src/config/mod.rs | 16 ++++++++++++++++ bottlecap/src/traces/trace_processor.rs | 4 ++-- 2 files changed, 18 insertions(+), 2 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index d1a9ed258..9a908ad35 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -779,6 +779,22 @@ mod lambda_config_tests { assert!(crate::traces::trace_processor::error_rare_sample_tracer_drop_enabled(&config)); } + #[test] + fn apm_features_space_separated_list_from_env() { + let config = load(|jail| { + jail.set_env( + "DD_APM_FEATURES", + "some_other_feature error_rare_sample_tracer_drop", + ); + Ok(()) + }); + assert_eq!( + config.apm_features, + ["some_other_feature", "error_rare_sample_tracer_drop"] + ); + assert!(crate::traces::trace_processor::error_rare_sample_tracer_drop_enabled(&config)); + } + #[test] fn apm_features_unrelated_feature_does_not_enable_error_rare_sample_tracer_drop() { let config = load(|jail| { diff --git a/bottlecap/src/traces/trace_processor.rs b/bottlecap/src/traces/trace_processor.rs index 57eb3ca0c..72eb9ce8f 100644 --- a/bottlecap/src/traces/trace_processor.rs +++ b/bottlecap/src/traces/trace_processor.rs @@ -79,8 +79,8 @@ const USER_DROP_PRIORITY: i32 = -1; /// Returns true when `config.apm_features` contains /// [`ERROR_RARE_SAMPLE_TRACER_DROP`]. /// -/// `DD_APM_FEATURES` is parsed upstream as a comma-separated list, so matching -/// is exact per entry; space-separated values are not supported. +/// `DD_APM_FEATURES` is split on commas when one is present, otherwise on +/// spaces, and matching is exact per entry. #[must_use] pub fn error_rare_sample_tracer_drop_enabled(config: &config::Config) -> bool { config From 29e543a85423949ff52518286d9fb73f6728b926 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Mon, 2 Nov 2026 17:43:40 -0500 Subject: [PATCH 03/15] feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS Error trace rescue now uses a rate-limited budget instead of an on/off switch: it is on by default at 10 errored traces per second per execution environment when the extension computes stats. Set DD_APM_ERROR_TPS<=0 to disable rescue, or a positive value to change the budget. Exponent values without a decimal point, such as 1e3, are rejected as invalid. DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated and now logs a startup warning; DD_APM_ERROR_TPS wins when both are set. --- bottlecap/src/config/mod.rs | 426 ++++++++++++++++-- .../src/lifecycle/invocation/processor.rs | 8 +- .../lifecycle/invocation/processor_service.rs | 2 +- bottlecap/src/startup.rs | 21 +- bottlecap/src/traces/trace_agent.rs | 2 +- bottlecap/src/traces/trace_processor.rs | 316 ++++++++++--- bottlecap/tests/apm_integration_test.rs | 4 +- 7 files changed, 664 insertions(+), 115 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index 9a908ad35..3ec8f6f27 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -51,6 +51,12 @@ use datadog_agent_config::{ flush_strategy::FlushStrategy as UpstreamFlushStrategy, }; +/// Default error trace rescue budget in errored traces per second. +/// +/// Matches the shared sampler's default (`ErrorSamplerConfig::default().target_tps`) +/// and the Go agent's `ErrorTPS`. +pub const DEFAULT_APM_ERROR_TPS: f64 = 10.0; + /// Lambda-specific configuration that lives alongside the shared /// `datadog_agent_config::Config` core fields under `config.ext` once the /// migration onto upstream lands. @@ -69,14 +75,30 @@ pub struct LambdaConfig { pub capture_lambda_payload_max_depth: u32, pub lambda_extension_compute_stats: bool, - /// `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`: rescue errored trace chunks that would - /// otherwise be dropped, whenever stats are computed before the backend, by - /// either the tracer or the extension. The sampler runs in `AlwaysKeep` mode, - /// so this is a plain on/off switch with no volume ceiling: enabled rescues - /// every errored chunk, whatever the tracer's sampling rate. A function that - /// errors on most invocations under `DD_TRACE_SAMPLE_RATE=0.01` will ingest - /// close to every trace, not 1%. See APMSVLS-469. - pub serverless_error_sampler_enabled: bool, + /// Deprecated raw value of `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`, kept so + /// the legacy on/off switch still resolves. Read only through + /// [`LambdaConfig::error_sampler_target_tps`] and + /// [`LambdaConfig::legacy_error_sampler_warning`]; set `DD_APM_ERROR_TPS` + /// instead. + pub serverless_error_sampler_enabled: Option, + + /// `DD_APM_ERROR_TPS`: error trace rescue budget in errored traces per + /// second, distributed fairly across trace signatures. `None` uses + /// [`DEFAULT_APM_ERROR_TPS`]; `<= 0` disables rescue. + /// + /// The budget is per execution environment, so N concurrent sandboxes can + /// rescue up to N times the budget. + /// + /// Rescue runs whenever stats are computed before the backend, by either + /// the tracer or the extension. + /// + /// The limit is approximate and has no hard ceiling (inherited from the Go + /// agent and the Serverless Compatibility Layer). Rates are recomputed on + /// each 5s bucket rotation over a 30s window. A cold sampler keeps errors + /// at rate 1.0 until volume in the current bucket exceeds about 5x the + /// budget. After a freeze/thaw gap, stale counters expire, but a throttled + /// signature's rate recovers by at most 20% per evaluation. + pub apm_error_tps: Option, pub span_dedup_timeout: Option, pub api_key_secret_reload_interval: Option, @@ -144,7 +166,8 @@ impl Default for LambdaConfig { capture_lambda_payload: false, capture_lambda_payload_max_depth: 10, lambda_extension_compute_stats: false, - serverless_error_sampler_enabled: false, + serverless_error_sampler_enabled: None, + apm_error_tps: None, span_dedup_timeout: None, api_key_secret_reload_interval: None, serverless_appsec_enabled: false, @@ -165,6 +188,24 @@ impl Default for LambdaConfig { } } +/// Deserialize an optional `f64`, discarding non-finite values. +/// +/// `figment` accepts non-finite floats from YAML (`.inf`, `.nan`) and from +/// dotted exponent strings (`1.0e999`), so filter them out here. +fn deserialize_optional_finite_f64<'de, D>(deserializer: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + let value = deser_opt_lossless::<_, f64>(deserializer)?; + match value { + Some(v) if !v.is_finite() => { + tracing::warn!("Ignoring non-finite DD_APM_ERROR_TPS (apm_error_tps) value {v}"); + Ok(None) + } + other => Ok(other), + } +} + /// Intermediate deserialization type shared by env-var and YAML loading. /// /// `#[serde(default)]` and the forgiving per-field deserializers are required @@ -213,6 +254,11 @@ pub struct LambdaConfigSource { #[serde(deserialize_with = "deser_opt_bool")] pub serverless_error_sampler_enabled: Option, + /// `DD_APM_ERROR_TPS`: error trace rescue budget in errored traces per + /// second. + #[serde(deserialize_with = "deserialize_optional_finite_f64")] + pub apm_error_tps: Option, + #[serde(deserialize_with = "deser_dur_secs_ignore_zero")] pub span_dedup_timeout: Option, #[serde(deserialize_with = "deser_dur_secs_ignore_zero")] @@ -294,7 +340,6 @@ impl DatadogConfigExtension for LambdaConfig { capture_lambda_payload, capture_lambda_payload_max_depth, lambda_extension_compute_stats, - serverless_error_sampler_enabled, serverless_appsec_enabled, appsec_waf_timeout, api_security_enabled, @@ -303,7 +348,7 @@ impl DatadogConfigExtension for LambdaConfig { lambda_durable_function_log_buffer_size, trace_experimental_features_enabled, ], - option: [span_dedup_timeout, api_key_secret_reload_interval, appsec_rules, dsm_exchange_name, dsm_kafka_group], + option: [span_dedup_timeout, api_key_secret_reload_interval, appsec_rules, serverless_error_sampler_enabled, apm_error_tps, dsm_exchange_name, dsm_kafka_group], ); // data_streams_enabled (source / DD_DATA_STREAMS_ENABLED) → @@ -353,6 +398,38 @@ impl DatadogConfigExtension for LambdaConfig { } impl LambdaConfig { + /// Effective error trace rescue budget in errored traces per second. + /// + /// `DD_APM_ERROR_TPS` wins when set. Otherwise the deprecated + /// `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` is a kill switch, and + /// everything else falls back to [`DEFAULT_APM_ERROR_TPS`]. + #[must_use] + pub fn error_sampler_target_tps(&self) -> f64 { + match (self.apm_error_tps, self.serverless_error_sampler_enabled) { + (Some(tps), _) => tps, + (None, Some(false)) => 0.0, + _ => DEFAULT_APM_ERROR_TPS, + } + } + + /// Deprecation warning for `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`, or `None` + /// when it is unset. + #[must_use] + pub fn legacy_error_sampler_warning(&self) -> Option { + match (self.apm_error_tps, self.serverless_error_sampler_enabled) { + (_, None) => None, + (Some(tps), Some(enabled)) => Some(format!( + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED={enabled} is deprecated and ignored because DD_APM_ERROR_TPS={tps} is set" + )), + (None, Some(true)) => Some(format!( + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated and has no effect: error trace rescue is on by default at {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS to change the budget" + )), + (None, Some(false)) => Some(String::from( + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated; set DD_APM_ERROR_TPS=0 to disable error trace rescue instead", + )), + } + } + /// Drop `additional_metric_tags` / `additional_metric_tags_cardinality_limit` unless /// `trace_experimental_features_enabled` is set, matching the Serverless Compatibility /// Layer (`datadog-trace-agent`). @@ -715,30 +792,316 @@ mod lambda_config_tests { assert!(!config.ext.dsm_consume_enabled); } - // ---- error sampler (serverless_error_sampler_enabled) ---- + // ---- error trace rescue budget (DD_APM_ERROR_TPS) ---- - #[test] - fn serverless_error_sampler_enabled_defaults_to_false() { - let config = load(|_| Ok(())); - assert!(!config.ext.serverless_error_sampler_enabled); + const TPS: &str = "DD_APM_ERROR_TPS"; + const LEGACY: &str = "DD_SERVERLESS_ERROR_SAMPLER_ENABLED"; + + /// One row in the error-sampler resolution table. + struct ErrorSamplerCase { + name: &'static str, + yaml: Option<&'static str>, + env: &'static [(&'static str, &'static str)], + target: f64, + /// Substrings the legacy-flag warning must contain; `None` expects no + /// warning. + warning_contains: Option<&'static [&'static str]>, + /// Whether the raw `apm_error_tps` is unset (missing or rejected). + tps_is_none: bool, } #[test] - fn serverless_error_sampler_enabled_from_env() { - let config = load(|jail| { - jail.set_env("DD_SERVERLESS_ERROR_SAMPLER_ENABLED", "true"); - Ok(()) - }); - assert!(config.ext.serverless_error_sampler_enabled); + #[allow(clippy::too_many_lines)] + #[allow(clippy::float_cmp)] + fn error_sampler_target_tps_resolution_table() { + let cases = [ + ErrorSamplerCase { + name: "default", + yaml: None, + env: &[], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "env int", + yaml: None, + env: &[(TPS, "5")], + target: 5.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "env float", + yaml: None, + env: &[(TPS, "2.5")], + target: 2.5, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "env padded", + yaml: None, + env: &[(TPS, " 7 ")], + target: 7.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "yaml", + yaml: Some("apm_error_tps: 3\n"), + env: &[], + target: 3.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "env over yaml", + yaml: Some("apm_error_tps: 3\n"), + env: &[(TPS, "4")], + target: 4.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "zero", + yaml: None, + env: &[(TPS, "0")], + target: 0.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "negative", + yaml: None, + env: &[(TPS, "-1")], + target: -1.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "large", + yaml: None, + env: &[(TPS, "1000000000")], + target: 1_000_000_000.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "exponent without dot", + yaml: None, + env: &[(TPS, "1e3")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "exponent with dot", + yaml: None, + env: &[(TPS, "1.0e3")], + target: 1000.0, + warning_contains: None, + tps_is_none: false, + }, + ErrorSamplerCase { + name: "invalid env: empty", + yaml: None, + env: &[(TPS, "")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid env: text", + yaml: None, + env: &[(TPS, "abc")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid env: inf", + yaml: None, + env: &[(TPS, "inf")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid env: nan", + yaml: None, + env: &[(TPS, "nan")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid env: overflow literal", + yaml: None, + env: &[(TPS, "1e400")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "non-finite float", + yaml: None, + env: &[(TPS, "1.0e999")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid yaml: inf", + yaml: Some("apm_error_tps: .inf\n"), + env: &[], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid yaml: nan", + yaml: Some("apm_error_tps: .nan\n"), + env: &[], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: None, + tps_is_none: true, + }, + ErrorSamplerCase { + name: "legacy true", + yaml: None, + env: &[(LEGACY, "true")], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: Some(&["deprecated"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "legacy false", + yaml: None, + env: &[(LEGACY, "false")], + target: 0.0, + warning_contains: Some(&["DD_APM_ERROR_TPS=0"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "legacy yaml false", + yaml: Some("serverless_error_sampler_enabled: false\n"), + env: &[], + target: 0.0, + warning_contains: Some(&["DD_APM_ERROR_TPS=0"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "both, tps 5 + legacy false", + yaml: None, + env: &[(TPS, "5"), (LEGACY, "false")], + target: 5.0, + warning_contains: Some(&[ + "DD_APM_ERROR_TPS=5", + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false", + ]), + tps_is_none: false, + }, + ErrorSamplerCase { + name: "both, tps 0 + legacy true", + yaml: None, + env: &[(TPS, "0"), (LEGACY, "true")], + target: 0.0, + warning_contains: Some(&[ + "DD_APM_ERROR_TPS=0", + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true", + ]), + tps_is_none: false, + }, + ErrorSamplerCase { + name: "cross-source", + yaml: Some("apm_error_tps: 5\n"), + env: &[(LEGACY, "false")], + target: 5.0, + warning_contains: Some(&[ + "DD_APM_ERROR_TPS=5", + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false", + ]), + tps_is_none: false, + }, + ErrorSamplerCase { + name: "invalid tps + legacy false", + yaml: None, + env: &[(TPS, "abc"), (LEGACY, "false")], + target: 0.0, + warning_contains: Some(&["set DD_APM_ERROR_TPS=0"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "invalid env over valid yaml + legacy false", + yaml: Some("apm_error_tps: 5\n"), + env: &[(TPS, "abc"), (LEGACY, "false")], + target: 5.0, + warning_contains: Some(&[ + "DD_APM_ERROR_TPS=5", + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false", + ]), + tps_is_none: false, + }, + ]; + + for case in cases { + let name = case.name; + let config = load(|jail| { + if let Some(yaml) = case.yaml { + jail.create_file("datadog.yaml", yaml)?; + } + for (key, value) in case.env { + jail.set_env(key, value); + } + Ok(()) + }); + + assert_eq!( + config.ext.error_sampler_target_tps(), + case.target, + "case {name:?}: unexpected effective target TPS" + ); + assert_eq!( + config.ext.apm_error_tps.is_none(), + case.tps_is_none, + "case {name:?}: unexpected raw apm_error_tps {:?}", + config.ext.apm_error_tps + ); + + match ( + case.warning_contains, + config.ext.legacy_error_sampler_warning(), + ) { + (None, None) => {} + (Some(needles), Some(message)) => { + assert!( + !message.contains("removed"), + "case {name:?}: deprecation warning must not promise removal: {message}" + ); + for needle in needles { + assert!( + message.contains(needle), + "case {name:?}: warning {message:?} must contain {needle:?}" + ); + } + } + (expected, actual) => { + panic!( + "case {name:?}: expected warning containing {expected:?}, got {actual:?}" + ) + } + } + } } #[test] - fn serverless_error_sampler_enabled_from_yaml() { - let config = load(|jail| { - jail.create_file("datadog.yaml", "serverless_error_sampler_enabled: true\n")?; - Ok(()) - }); - assert!(config.ext.serverless_error_sampler_enabled); + #[allow(clippy::float_cmp)] + fn default_apm_error_tps_matches_shared_sampler_default() { + assert_eq!( + DEFAULT_APM_ERROR_TPS, + datadog_agent_trace_sampler::ErrorSamplerConfig::default().target_tps + ); } // ---- APM features (DD_APM_FEATURES, error_rare_sample_tracer_drop) ---- @@ -751,6 +1114,7 @@ mod lambda_config_tests { } #[test] + #[allow(clippy::float_cmp)] fn apm_features_error_rare_sample_tracer_drop_from_env() { let config = load(|jail| { jail.set_env("DD_APM_FEATURES", "error_rare_sample_tracer_drop"); @@ -758,9 +1122,9 @@ mod lambda_config_tests { }); assert_eq!(config.apm_features, ["error_rare_sample_tracer_drop"]); assert!(crate::traces::trace_processor::error_rare_sample_tracer_drop_enabled(&config)); - // The feature only widens rescue eligibility; enabling it must not - // enable the sampler. - assert!(!config.ext.serverless_error_sampler_enabled); + // The feature only widens rescue eligibility; it must not change the + // rescue budget. + assert_eq!(config.ext.error_sampler_target_tps(), DEFAULT_APM_ERROR_TPS); } #[test] diff --git a/bottlecap/src/lifecycle/invocation/processor.rs b/bottlecap/src/lifecycle/invocation/processor.rs index 26a9fe417..89714d457 100644 --- a/bottlecap/src/lifecycle/invocation/processor.rs +++ b/bottlecap/src/lifecycle/invocation/processor.rs @@ -2293,7 +2293,7 @@ mod tests { appsec: None, processor: Arc::new(trace_processor::ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: trace_processor::enabled_error_sampler(), + error_sampler: trace_processor::default_error_sampler(), error_rare_sample_tracer_drop: false, }), trace_tx: tokio::sync::mpsc::channel(1).0, @@ -2403,7 +2403,7 @@ mod tests { appsec: None, processor: Arc::new(trace_processor::ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: trace_processor::enabled_error_sampler(), + error_sampler: trace_processor::default_error_sampler(), error_rare_sample_tracer_drop: false, }), trace_tx: tokio::sync::mpsc::channel(1).0, @@ -3041,7 +3041,7 @@ mod tests { appsec: None, processor: Arc::new(trace_processor::ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: trace_processor::enabled_error_sampler(), + error_sampler: trace_processor::default_error_sampler(), error_rare_sample_tracer_drop: false, }), trace_tx: tokio::sync::mpsc::channel(1).0, @@ -3622,7 +3622,7 @@ mod tests { appsec: None, processor: Arc::new(trace_processor::ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: trace_processor::enabled_error_sampler(), + error_sampler: trace_processor::default_error_sampler(), error_rare_sample_tracer_drop: false, }), trace_tx, diff --git a/bottlecap/src/lifecycle/invocation/processor_service.rs b/bottlecap/src/lifecycle/invocation/processor_service.rs index a77e888bd..6f592b45e 100644 --- a/bottlecap/src/lifecycle/invocation/processor_service.rs +++ b/bottlecap/src/lifecycle/invocation/processor_service.rs @@ -831,7 +831,7 @@ mod tests { use std::collections::HashMap; use tokio::time::{Duration, timeout}; - let error_sampler = trace_processor::new_error_sampler(false); + let error_sampler = trace_processor::new_error_sampler(0.0); let timeout_dur = Duration::from_millis(500); diff --git a/bottlecap/src/startup.rs b/bottlecap/src/startup.rs index cf12b0939..425613393 100644 --- a/bottlecap/src/startup.rs +++ b/bottlecap/src/startup.rs @@ -12,7 +12,7 @@ //! together trace, stats, proxy, lifecycle, tags, appsec, and flushing //! pieces: it is orchestration, not a trace-domain API. -use std::{env, sync::Arc}; +use std::sync::Arc; use dogstatsd::api_key::ApiKeyFactory; use libdd_common::regex_engine::{Regex, Replacer}; @@ -157,29 +157,24 @@ pub fn build_trace_agent( ..Default::default() }; - // The Agent's error sampler knob has no effect here: the extension's - // sampler is a plain on/off switch, not a TPS budget. - if env::var("DD_APM_ERROR_TPS").is_ok_and(|v| !v.trim().is_empty()) { - warn!( - "DD_APM_ERROR_TPS is not supported by the Lambda extension; error trace rescue is an on/off switch controlled by DD_SERVERLESS_ERROR_SAMPLER_ENABLED" - ); + if let Some(message) = config.ext.legacy_error_sampler_warning() { + warn!("{message}"); } + let error_tps = config.ext.error_sampler_target_tps(); // One-time startup warning: the feature only widens rescue eligibility, it - // does not enable the sampler. + // does not change the rescue budget. let error_rare_sample_tracer_drop = trace_processor::error_rare_sample_tracer_drop_enabled(config); - if error_rare_sample_tracer_drop && !config.ext.serverless_error_sampler_enabled { + if error_rare_sample_tracer_drop && error_tps <= 0.0 { warn!( - "DD_APM_FEATURES includes error_rare_sample_tracer_drop, but DD_SERVERLESS_ERROR_SAMPLER_ENABLED is false: errored traces dropped by the tracer will not be rescued" + "DD_APM_FEATURES includes error_rare_sample_tracer_drop, but error trace rescue is disabled (effective DD_APM_ERROR_TPS {error_tps}): errored traces dropped by the tracer will not be rescued" ); } let trace_processor = Arc::new(trace_processor::ServerlessTraceProcessor { obfuscation_config: Arc::new(obfuscation_config), - error_sampler: trace_processor::new_error_sampler( - config.ext.serverless_error_sampler_enabled, - ), + error_sampler: trace_processor::new_error_sampler(error_tps), error_rare_sample_tracer_drop, }); diff --git a/bottlecap/src/traces/trace_agent.rs b/bottlecap/src/traces/trace_agent.rs index bb7bdf6cf..d08448c06 100644 --- a/bottlecap/src/traces/trace_agent.rs +++ b/bottlecap/src/traces/trace_agent.rs @@ -1145,7 +1145,7 @@ mod tests { tokio::spawn(dedup_svc.run()); let trace_processor = Arc::new(trace_processor::ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: trace_processor::new_error_sampler(false), + error_sampler: trace_processor::new_error_sampler(0.0), error_rare_sample_tracer_drop: false, }); let stats_aggregator = Arc::new(Mutex::new( diff --git a/bottlecap/src/traces/trace_processor.rs b/bottlecap/src/traces/trace_processor.rs index 72eb9ce8f..c44940e9b 100644 --- a/bottlecap/src/traces/trace_processor.rs +++ b/bottlecap/src/traces/trace_processor.rs @@ -94,9 +94,10 @@ pub fn error_rare_sample_tracer_drop_enabled(config: &config::Config) -> bool { pub struct ServerlessTraceProcessor { pub obfuscation_config: Arc, /// Rescues errored chunks whenever stats are computed before the backend, - /// by either the tracer or the extension. Shared across invocations so a - /// stateful mode (`RateLimited`) would keep its rolling window; a std Mutex - /// rather than tokio because `process_traces` is synchronous. + /// by either the tracer or the extension. Shared across invocations so the + /// `RateLimited` budget and rolling window span the whole execution + /// environment; a std Mutex rather than tokio because `process_traces` is + /// synchronous. pub error_sampler: Arc>, /// Whether `DD_APM_FEATURES` enables [`ERROR_RARE_SAMPLE_TRACER_DROP`]. When /// true, chunks with a negative priority are rescued like `AutoDrop` chunks @@ -117,22 +118,32 @@ fn span_view(span: &Span) -> SpanView<'_> { } } -/// Builds the error sampler as the extension ships it, enabled or disabled by -/// `serverless_error_sampler_enabled`. +/// Builds the error sampler's configuration with the extension's fixed policy: +/// rate-limited rescue with the given budget and no extra sampling. /// -/// The mode is hardcoded to `AlwaysKeep`: Lambda's per-invocation trace volume -/// is low, and freeze/thaw breaks `RateLimited`'s 30s wall-clock window. There -/// is therefore no rate ceiling when enabled: every errored chunk is rescued. +/// All three fields are set explicitly so a change to the crate's defaults +/// cannot silently change bottlecap behavior. #[must_use] -pub fn new_error_sampler(enabled: bool) -> Arc> { +pub fn error_sampler_config(target_tps: f64) -> ErrorSamplerConfig { + ErrorSamplerConfig { + mode: ErrorSamplerMode::RateLimited, + target_tps, + extra_sample_rate: 1.0, + } +} + +/// Builds the error sampler as the extension ships it: rate-limited rescue with +/// the given budget. +/// +/// The rolling window is driven by the wall-clock seconds passed on each call, +/// so a freeze/thaw gap expires stale counters. Stored per-signature rates are +/// not reset by a gap: a throttled signature recovers by at most 20% per +/// evaluation (inherited from the Go agent). A budget `<= 0` disables rescue; +/// see [`crate::config::LambdaConfig::apm_error_tps`] for the caveats. +#[must_use] +pub fn new_error_sampler(target_tps: f64) -> Arc> { Arc::new(std::sync::Mutex::new(ErrorsSampler::new( - ErrorSamplerConfig { - mode: ErrorSamplerMode::AlwaysKeep, - // `is_disabled()` is `target_tps <= 0.0`; the magnitude only - // matters in RateLimited mode. - target_tps: if enabled { 1.0 } else { 0.0 }, - extra_sample_rate: 1.0, - }, + error_sampler_config(target_tps), ))) } @@ -792,10 +803,11 @@ impl SendingTraceProcessor { } } -/// Enabled error sampler for constructing `ServerlessTraceProcessor` in tests. +/// Error sampler with the default budget, for constructing +/// `ServerlessTraceProcessor` in tests. #[cfg(test)] -pub(crate) fn enabled_error_sampler() -> Arc> { - new_error_sampler(true) +pub(crate) fn default_error_sampler() -> Arc> { + new_error_sampler(crate::config::DEFAULT_APM_ERROR_TPS) } #[cfg(test)] @@ -964,7 +976,7 @@ mod tests { let trace_processor = ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: enabled_error_sampler(), + error_sampler: default_error_sampler(), error_rare_sample_tracer_drop: false, }; let config = create_test_config(); @@ -1428,7 +1440,7 @@ mod tests { for (compute_on_extension, client_computed_stats, expected_backend_span_count) in cases { let config = create_compute_stats_config_with(compute_on_extension); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), false); + create_test_processor(&config, default_error_sampler(), false); let mut header_tags = create_test_header_tags(); header_tags.generic.client_computed_stats = client_computed_stats; @@ -1466,6 +1478,7 @@ mod tests { /// dropped (`AutoDrop`), stamping `_dd.errors_sr`, while non-errored P0 chunks /// and explicit user drops are still dropped. Priorities are never rewritten. #[test] + #[allow(clippy::float_cmp)] fn test_error_sampler_rescues_errored_p0_chunks() { // (owner label, lambda_extension_compute_stats, client_computed_stats) let cases = [("extension", true, false), ("tracer", false, true)]; @@ -1473,7 +1486,7 @@ mod tests { for (owner, compute_on_extension, client_computed_stats) in cases { let config = create_compute_stats_config_with(compute_on_extension); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), false); + create_test_processor(&config, default_error_sampler(), false); let mut header_tags = create_test_header_tags(); header_tags.generic.client_computed_stats = client_computed_stats; @@ -1510,9 +1523,10 @@ mod tests { let rescued_root = trace_spans(backend_send_data.get_payloads()) .find(|s| s.trace_id == 2) .expect("rescued trace present"); - assert!( - rescued_root.metrics.contains_key("_dd.errors_sr"), - "_dd.errors_sr stamped on rescued root (owner={owner})" + assert_eq!( + rescued_root.metrics.get("_dd.errors_sr").copied(), + Some(1.0), + "_dd.errors_sr is exactly 1.0 for a cold sampler at low volume (owner={owner})" ); } } @@ -1530,7 +1544,7 @@ mod tests { for (owner, compute_on_extension, client_computed_stats) in cases { let config = create_compute_stats_config_with(compute_on_extension); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); let mut header_tags = create_test_header_tags(); header_tags.generic.client_computed_stats = client_computed_stats; @@ -1624,6 +1638,180 @@ mod tests { (kept, in_stats) } + #[test] + #[allow(clippy::float_cmp)] + fn error_sampler_config_is_rate_limited_with_given_tps() { + for tps in [10.0, 2.5, 0.0, -1.0] { + let config = error_sampler_config(tps); + assert_eq!(config.mode, ErrorSamplerMode::RateLimited, "tps {tps}"); + assert_eq!(config.target_tps, tps, "tps {tps}"); + assert_eq!(config.extra_sample_rate, 1.0, "tps {tps}"); + } + } + + #[test] + fn new_error_sampler_builds_rate_limited() { + let sampler = new_error_sampler(10.0); + assert!( + matches!( + *sampler.lock().expect("lock"), + ErrorsSampler::RateLimited(_) + ), + "a positive budget must build a RateLimited sampler" + ); + assert!( + !sampler.lock().expect("lock").is_disabled(), + "10 tps must not disable rescue" + ); + for tps in [0.0, -1.0] { + assert!( + new_error_sampler(tps).lock().expect("lock").is_disabled(), + "{tps} tps must disable rescue" + ); + } + } + + /// Samples one single-span errored trace at `now`. Every call uses the same + /// signature; `n` only spreads the trace id. + fn sample_errored_trace( + sampler: &std::sync::Mutex, + now: i64, + n: u64, + ) -> SampleDecision { + let spans = [SpanView { + service: "svc", + name: "op", + resource: "res", + error: true, + http_status_code: None, + error_type: None, + }]; + let trace = TraceView { + env: "test-env", + trace_id: n.wrapping_mul(2_654_435_761), + root_index: 0, + root_global_sample_rate: 1.0, + spans: &spans, + }; + sampler.lock().expect("lock").sample(now, &trace) + } + + /// A backward clock step, a gap longer than the 30s window, and the epoch + /// fallback value must all be survivable; after the gap a cold signature + /// falls back to rate 1.0. + #[test] + #[allow(clippy::float_cmp)] + fn error_sampler_survives_clock_jumps() { + let sampler = new_error_sampler(crate::config::DEFAULT_APM_ERROR_TPS); + sample_errored_trace(&sampler, 1_700_000_000, 1); + // Backward step. + sample_errored_trace(&sampler, 1_699_999_000, 2); + assert_eq!( + sample_errored_trace(&sampler, 1_700_100_000, 3), + SampleDecision::Keep { errors_sr: 1.0 }, + "after a long gap a cold signature falls back to rate 1.0" + ); + // The `UNIX_EPOCH.elapsed()` fallback value. + sample_errored_trace(&sampler, 0, 4); + } + + /// A long freeze/thaw gap clears stale counters, but a throttled + /// signature's stored rate recovers by at most 20% per evaluation, so it + /// stays below 1.0 after a single gap. + #[test] + fn error_sampler_stays_throttled_after_long_gap() { + let sampler = new_error_sampler(1.0); + let t0 = 1_700_000_000_i64; + let mut n = 0_u64; + for bucket in 0..4_i64 { + for _ in 0..200 { + n += 1; + sample_errored_trace(&sampler, t0 + 5 * bucket, n); + } + } + + match sample_errored_trace(&sampler, t0 + 15 + 3600, n + 1) { + SampleDecision::Drop => {} + SampleDecision::Keep { errors_sr } => { + assert!( + errors_sr < 1.0, + "a previously throttled signature must stay below 1.0, got {errors_sr}" + ); + } + } + } + + /// Runs one batch of 50 errored P0 traces with the same signature (distinct + /// trace ids derived from `call`) through `processor`, returning how many + /// spans were rescued. A batch whose chunks are all dropped returns 0. + fn run_errored_p0_batch( + processor: &ServerlessTraceProcessor, + config: &Arc, + tags_provider: &Arc, + call: usize, + ) -> usize { + let traces: Vec> = (0..50) + .map(|i| { + let trace_id = ((call * 50 + i + 1) as u64).wrapping_mul(2_654_435_761); + let mut metrics = HashMap::new(); + metrics.insert("_sampling_priority_v1".to_string(), 0.0); + vec![pb::Span { + trace_id, + span_id: trace_id, + parent_id: 0, + error: 1, + metrics, + service: "svc".to_string(), + name: "op".to_string(), + resource: "res".to_string(), + ..Default::default() + }] + }) + .collect(); + + processor + .process_traces( + config.clone(), + tags_provider.clone(), + create_test_header_tags(), + traces, + 0, + None, + ) + .0 + .map_or(0, |info| { + trace_spans(info.builder.build().get_payloads()).count() + }) + } + + /// The rescue budget lives on the shared sampler, so it spans invocations: + /// ten batches through one processor rescue far fewer traces than ten + /// batches each with a fresh sampler. + #[test] + fn error_sampler_budget_is_shared_across_invocations() { + let config = create_compute_stats_config(); + + let (tags_provider, processor) = + create_test_processor(&config, new_error_sampler(1.0), false); + let shared: usize = (0..10) + .map(|call| run_errored_p0_batch(&processor, &config, &tags_provider, call)) + .sum(); + + let control: usize = (0..10) + .map(|call| { + let (tags_provider, processor) = + create_test_processor(&config, new_error_sampler(1.0), false); + run_errored_p0_batch(&processor, &config, &tags_provider, call) + }) + .sum(); + + assert!(shared > 0, "the shared run must rescue some traces"); + assert!( + shared * 2 < control, + "one shared budget must rescue far fewer traces than a fresh sampler per batch: shared {shared}, control {control}" + ); + } + /// A manual tracer drop (priority -1 with decision maker "-4" in the root /// span meta) is never rescued, even with the feature enabled, matching the /// Go agent's `isManualUserDrop`. @@ -1631,7 +1819,7 @@ mod tests { fn test_error_rare_sample_tracer_drop_excludes_manual_drop() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); let trace = vec![make_tracer_drop_span(7, -1.0, 1, Some("-4"))]; let (kept, in_stats) = run_single_trace(&processor, &config, tags_provider, trace); @@ -1650,7 +1838,7 @@ mod tests { fn test_error_rare_sample_tracer_drop_manual_drop_on_child_span() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); // Root is fine and carries no decision maker; the errored child does. let mut root = make_tracer_drop_span(7, -1.0, 0, None); @@ -1672,7 +1860,7 @@ mod tests { fn test_error_rare_sample_tracer_drop_nonmanual_decision_maker_rescued() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); let trace = vec![make_tracer_drop_span(7, -1.0, 1, Some("-3"))]; let (kept, _) = run_single_trace(&processor, &config, tags_provider, trace); @@ -1689,7 +1877,7 @@ mod tests { fn test_error_rare_sample_tracer_drop_other_negative_priority_rescued() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); let trace = vec![make_tracer_drop_span(7, -2.0, 1, Some("-4"))]; let (kept, _) = run_single_trace(&processor, &config, tags_provider, trace); @@ -1708,7 +1896,7 @@ mod tests { for (first_dm, second_dm, rescued) in [("-3", "-4", true), ("-4", "-3", false)] { let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); let mut root = make_tracer_drop_span(7, -1.0, 1, Some(first_dm)); root.span_id = 1; let mut child = make_tracer_drop_span(7, -1.0, 0, Some(second_dm)); @@ -1729,7 +1917,7 @@ mod tests { fn test_error_rare_sample_tracer_drop_nonerrored_still_dropped() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), true); + create_test_processor(&config, default_error_sampler(), true); let trace = vec![make_tracer_drop_span(7, -1.0, 0, None)]; let (kept, in_stats) = run_single_trace(&processor, &config, tags_provider, trace); @@ -1747,7 +1935,7 @@ mod tests { fn test_error_rare_sample_tracer_drop_requires_sampler_enabled() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, new_error_sampler(false), true); + create_test_processor(&config, new_error_sampler(0.0), true); let trace = vec![make_tracer_drop_span(7, -1.0, 1, None)]; let (kept, _) = run_single_trace(&processor, &config, tags_provider, trace); @@ -1764,7 +1952,7 @@ mod tests { fn test_error_sampler_rescues_chunk_with_errored_child_span() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), false); + create_test_processor(&config, default_error_sampler(), false); let header_tags = create_test_header_tags(); let make_span = |span_id: u64, parent_id: u64, error: i32| -> pb::Span { @@ -1808,35 +1996,37 @@ mod tests { ); } - /// With the error sampler disabled (the shipping default), errored P0 + /// With error trace rescue disabled (`DD_APM_ERROR_TPS <= 0`), errored P0 /// chunks are dropped: no rescue, no `_dd.errors_sr`. #[test] fn test_disabled_error_sampler_drops_errored_p0_chunks() { - let config = create_compute_stats_config(); - let (tags_provider, processor) = - create_test_processor(&config, new_error_sampler(false), false); - let header_tags = create_test_header_tags(); + for tps in [0.0, -1.0] { + let config = create_compute_stats_config(); + let (tags_provider, processor) = + create_test_processor(&config, new_error_sampler(tps), false); + let header_tags = create_test_header_tags(); - let mut metrics = HashMap::new(); - metrics.insert("_sampling_priority_v1".to_string(), 0.0); - let traces = vec![vec![pb::Span { - trace_id: 1, - span_id: 1, - parent_id: 0, - error: 1, - metrics, - service: "svc".to_string(), - name: "op".to_string(), - resource: "res".to_string(), - ..Default::default() - }]]; + let mut metrics = HashMap::new(); + metrics.insert("_sampling_priority_v1".to_string(), 0.0); + let traces = vec![vec![pb::Span { + trace_id: 1, + span_id: 1, + parent_id: 0, + error: 1, + metrics, + service: "svc".to_string(), + name: "op".to_string(), + resource: "res".to_string(), + ..Default::default() + }]]; - let (payload_info, _stats) = - processor.process_traces(config, tags_provider, header_tags, traces, 0, None); - assert!( - payload_info.is_none(), - "errored P0 trace must stay dropped when the error sampler is disabled" - ); + let (payload_info, _stats) = + processor.process_traces(config, tags_provider, header_tags, traces, 0, None); + assert!( + payload_info.is_none(), + "errored P0 trace must stay dropped when rescue is disabled (tps {tps})" + ); + } } /// With the error sampler disabled, an errored P0 chunk dropped from the @@ -1846,7 +2036,7 @@ mod tests { fn test_disabled_error_sampler_keeps_dropped_errored_p0_chunk_in_stats() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, new_error_sampler(false), false); + create_test_processor(&config, new_error_sampler(0.0), false); let header_tags = create_test_header_tags(); let make_span = |trace_id: u64, priority: f64, error: i32| -> pb::Span { @@ -1914,7 +2104,7 @@ mod tests { fn test_process_traces_returns_none_when_all_sampled_out() { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), false); + create_test_processor(&config, default_error_sampler(), false); let header_tags = create_test_header_tags(); let make_dropped_span = |trace_id: u64| -> pb::Span { @@ -1965,7 +2155,7 @@ mod tests { let config = create_compute_stats_config(); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), false); + create_test_processor(&config, default_error_sampler(), false); let header_tags = create_test_header_tags(); let make_span = |trace_id: u64, priority: f64| -> pb::Span { @@ -2049,7 +2239,7 @@ mod tests { ..Config::default() }); let (tags_provider, processor) = - create_test_processor(&config, enabled_error_sampler(), false); + create_test_processor(&config, default_error_sampler(), false); let header_tags = create_test_header_tags(); let span = pb::Span { @@ -2605,7 +2795,7 @@ mod tests { appsec: None, processor: Arc::new(ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: enabled_error_sampler(), + error_sampler: default_error_sampler(), error_rare_sample_tracer_drop: false, }), trace_tx, diff --git a/bottlecap/tests/apm_integration_test.rs b/bottlecap/tests/apm_integration_test.rs index d4a50d61d..c3c5c5ec5 100644 --- a/bottlecap/tests/apm_integration_test.rs +++ b/bottlecap/tests/apm_integration_test.rs @@ -21,7 +21,7 @@ use std::sync::Arc; use std::time::{SystemTime, UNIX_EPOCH}; use bottlecap::LAMBDA_RUNTIME_SLUG; -use bottlecap::config::{Config, get_config}; +use bottlecap::config::{Config, DEFAULT_APM_ERROR_TPS, get_config}; use bottlecap::tags::provider::Provider; use bottlecap::traces::http_client::create_client; use bottlecap::traces::stats_aggregator::StatsAggregator; @@ -375,7 +375,7 @@ async fn run_processor_pipeline_with_settings( appsec: None, processor: Arc::new(ServerlessTraceProcessor { obfuscation_config: Arc::new(ObfuscationConfig::default()), - error_sampler: new_error_sampler(true), + error_sampler: new_error_sampler(DEFAULT_APM_ERROR_TPS), error_rare_sample_tracer_drop, }), trace_tx, From 3c5a6171d52d18656d94afd34ff5287502d8ff05 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Mon, 2 Nov 2026 19:22:38 -0500 Subject: [PATCH 04/15] docs: update temporary plan with implementation status --- PLAN-error-sampler-tps.md | 26 ++++++++++++++++++++++---- 1 file changed, 22 insertions(+), 4 deletions(-) diff --git a/PLAN-error-sampler-tps.md b/PLAN-error-sampler-tps.md index ac1f98a28..e59dde0d4 100644 --- a/PLAN-error-sampler-tps.md +++ b/PLAN-error-sampler-tps.md @@ -1,6 +1,19 @@ # Plan: rate-limit Lambda error trace rescue with `DD_APM_ERROR_TPS` and deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` -> Temporary planning file (untracked). Delete before merging the PR. This file plans **PR 1 of 3**; PRs 2 and 3 are listed under "Follow-up PRs" and are out of scope here. +> Temporary planning file, committed on its own so it can be dropped. Delete it before merging the PR. This file plans **PR 1 of 3**; PRs 2 and 3 are listed under "Follow-up PRs" and are out of scope here. + +## Status (PR 1) +- **Implemented** on `lpimentel/error-sampler-rate-limited`, rebased onto `origin/main` `f32a2bc0` (#1471), and pushed. No PR opened yet. +- Commits: this plan file, then `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` (step 1), then `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS` (steps 2-4, with review nits and test simplifications squashed in). +- Final verification passed: `cargo fmt --check`, `cargo test --workspace`, the test-mode bin, and clippy `-D warnings` for default, default+test-mode, and fips. Greps for `AlwaysKeep`, `not supported by the Lambda extension`, `new_error_sampler(true|false)` and added em dashes are all clean. +- Not verified: the "make the shared run reset per call and watch the shared-budget test fail" check from step 3. +- Remaining: open the PR (text under "Commits and PR"), and drop this plan commit (or add a commit deleting the file) before merge. +- Deviations from the steps below, as implemented: + - The step 2 table rows are data (`yaml: Option<&str>`, `env: &[(key, value)]`, `tps_is_none: bool`) applied by one loop. The "no removal promise" check runs inside that loop on every warning row instead of in a separate test. The legacy raw-field check was dropped, since a warning exists exactly when the legacy flag is set. + - The non-finite warning reads `Ignoring non-finite DD_APM_ERROR_TPS (apm_error_tps) value {v}`, so it also covers YAML. + - The test helper `enabled_error_sampler()` is renamed `default_error_sampler()` (20 call sites in `trace_processor.rs` and `lifecycle/invocation/processor.rs`). + - The two sampler-level tests share a `sample_errored_trace(sampler, now, n)` helper. + - `DEFAULT_APM_ERROR_TPS` is declared after the `use datadog_agent_config::{...}` block. ## PR series | PR | Scope | Base | @@ -15,7 +28,7 @@ Order matters: filtering on the tracer path before rescue exists there would dro Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescue always uses the shared `ErrorSamplerMode::RateLimited` sampler with Go agent / SCL trace-agent defaults: rescue is on at 10 TPS when nothing is set, and `<= 0` turns it off. `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` stays parsed but deprecated. ## Context (verified by reading the code) -- Repo: DataDog/datadog-lambda-extension. Worktree: `/home/lucas/source/datadog/datadog-lambda-extension--lpimentel-error-sampler-rate-limited`. Branch: `lpimentel/error-sampler-rate-limited` at `aa9d3f54`, 1 commit behind `origin/main` (`e36a4709`, #1424, `AGENTS.md` only). Fast-forward to `origin/main` before starting; no line references below change. All Rust code is under `bottlecap/`. Toolchain is 1.93.1. +- Repo: DataDog/datadog-lambda-extension. Worktree: `/home/lucas/source/datadog/datadog-lambda-extension--lpimentel-error-sampler-rate-limited`. Branch: `lpimentel/error-sampler-rate-limited`, now based on `origin/main` `f32a2bc0`. Line references below are from the original base (`aa9d3f54`) and may have drifted by a few lines. All Rust code is under `bottlecap/`. Toolchain is 1.93.1. - `AGENTS.md` (after #1424) says to check `serverless-components` before adding functionality. This plan adds no new shared logic: the sampler is already the shared crate, and the new code is bottlecap's `LambdaConfig` extension field plus its resolution (bottlecap-specific because of the deprecated flag). SCL's `parse_error_sampler_config` lives in `datadog-trace-agent`, not in a shared crate, so there is nothing to reuse. - Sampler crate `datadog-agent-trace-sampler` is pinned at serverless-components rev `e462105e918904e6b1ad2ebc153ae561cebe5a34` (`bottlecap/Cargo.toml:100`). Do not bump it. Its source is at `~/.cargo/git/checkouts/serverless-components-d7345b4db0f8d2aa/e462105/crates/datadog-agent-trace-sampler/src/{lib.rs,score_sampler.rs}`. What it provides: - `ErrorSamplerConfig { mode, target_tps, extra_sample_rate }`. `Default` is `RateLimited`, `10.0`, `1.0`. @@ -95,7 +108,7 @@ Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescu Rust formats `5.0_f64` with `{}` as `5`, so `"DD_APM_ERROR_TPS=5"` matches. - Before finalizing the table, check how figment treats exponent notation without a dot (`DD_APM_ERROR_TPS=1e3`). The `1e400` row above suggests it stays a string and is rejected as invalid. Add a row pinning whatever actually happens, and if it is rejected, list it under Risks so the PR can mention it. + Resolved: figment only tries a float parse when the text contains `.` (`figment-0.10.19/src/value/parse.rs:90-100`, and again in `value.rs:512-532`). So `DD_APM_ERROR_TPS=1e3` stays a string and is rejected (target 10.0, `apm_error_tps == None`), while `1.0e3` parses to 1000.0. The table has both rows, and `1e400` is rejected the same way. - Add `default_apm_error_tps_matches_shared_sampler_default`, asserting `DEFAULT_APM_ERROR_TPS == datadog_agent_trace_sampler::ErrorSamplerConfig::default().target_tps`. - Update `apm_features_error_rare_sample_tracer_drop_from_env` (~:752-764). Replace `assert!(!config.ext.serverless_error_sampler_enabled)` with `assert_eq!(config.ext.error_sampler_target_tps(), DEFAULT_APM_ERROR_TPS)`. Reword its comment to "The feature only widens rescue eligibility; it must not change the rescue budget." @@ -103,7 +116,7 @@ Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescu 2. Add `pub const DEFAULT_APM_ERROR_TPS: f64 = 10.0;` near the top of the LambdaConfig section. Its doc comment should say it matches the shared sampler and Go agent default. 3. Add a private deserializer `deserialize_optional_finite_f64<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error>`. - Call `deser_opt_lossless::<_, f64>(d)?`, then filter out non-finite values. - - When a value is dropped, call `tracing::warn!("Ignoring non-finite DD_APM_ERROR_TPS value {v}")` (fully qualified, since `tracing` is not imported in this file). + - When a value is dropped, call `tracing::warn!("Ignoring non-finite DD_APM_ERROR_TPS (apm_error_tps) value {v}")` (fully qualified, since `tracing` is not imported in this file). 4. `LambdaConfig` (~:72-79, default ~:147): - Change `serverless_error_sampler_enabled` to `Option` and give it default `None`. - Rewrite its doc: deprecated raw value of `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`; read it only through `error_sampler_target_tps` and `legacy_error_sampler_warning`. @@ -213,6 +226,8 @@ Run everything from `bottlecap/` and report the pass/fail counts. 8. Grep the changed lines for `—` (em dash): zero matches. ## Commits and PR (only when the user asks) +Both commits are done (see Status). The PR is not opened yet. + - Commit 1 (step 1 only): `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` - Commit 2 (steps 2-4): `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS`. Body: - Rescue is on by default at 10 errored traces/s per execution environment when the extension computes stats. @@ -226,6 +241,8 @@ Run everything from `bottlecap/` and report the pass/fail counts. - The behavior change: compute-stats users who never set the flag now get rescue at 10/s. - The caveats: per-environment budget, compute-stats path only, approximate limit with no hard ceiling, inherited from Go/SCL. - `_dd.errors_sr` can now be below 1.0 under load. + - Legacy-only YAML users keep the same on/off result, but rescue becomes rate-limited. + - Exponent values without a dot, such as `1e3`, are rejected as invalid. - `## Testing`: describe what is covered. Do not include test counts. ## Risks @@ -234,6 +251,7 @@ Run everything from `bottlecap/` and report the pass/fail counts. - **Flaky-test risk.** The shared-budget test uses the real wall clock through `process_traces`, and a 5s bucket boundary can fall mid-test. Comparing against a reset control (`shared * 2 < control`) tolerates that, because rotation only throttles the shared run further. Do not assert exact counts. - **Throttled recovery after freeze/thaw (inherited).** After a long gap, stale counters are cleared but a throttled signature's stored rate only rises 1.2x per evaluation, and it also holds `lowest_rate` (the default for unknown signatures) down. A thawed sandbox can therefore under-rescue for a while after a past burst. Same as the Go agent and SCL; document it in the `apm_error_tps` doc and the PR, do not deviate. - **Legacy-only YAML users.** A user with `serverless_error_sampler_enabled: true` and no TPS keeps the same on/off result, but rescue is now rate-limited instead of keep-all. This is intended; state it in the PR. +- **`1e3` is rejected (parity, intentional).** Exponent notation without a dot stays a string in figment and is ignored with a deserializer warning. Mention it in the PR; do not add a custom parser. - **Duplicate warnings.** The non-finite warning in the deserializer can fire once per source (YAML and env). That is acceptable at startup. - **Lints.** Clippy pedantic needs `#[must_use]` on the new pub fns and backticks around identifiers in docs (`doc_markdown`). Use `.expect("...")`, not `.unwrap()`, in new tests unless the module already has `#[allow(clippy::unwrap_used)]` (the config test module does; the trace_processor test module relies on `expect`). From 1f95e98ddd07725675b88a963d1a28b8ff4d3d7d Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Tue, 3 Nov 2026 11:28:40 -0500 Subject: [PATCH 05/15] docs(bottlecap): clarify error sampler rate recovery --- bottlecap/src/config/mod.rs | 16 +++++++++++----- bottlecap/src/traces/trace_processor.rs | 8 +++++--- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index 3ec8f6f27..543cb85d7 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -93,11 +93,17 @@ pub struct LambdaConfig { /// the tracer or the extension. /// /// The limit is approximate and has no hard ceiling (inherited from the Go - /// agent and the Serverless Compatibility Layer). Rates are recomputed on - /// each 5s bucket rotation over a 30s window. A cold sampler keeps errors - /// at rate 1.0 until volume in the current bucket exceeds about 5x the - /// budget. After a freeze/thaw gap, stale counters expire, but a throttled - /// signature's rate recovers by at most 20% per evaluation. + /// agent and the Serverless Compatibility Layer). Errored traces are counted + /// in 5s buckets over a 30s window. Rates are recomputed only when a rescue + /// candidate arrives in a new bucket, not on a timer. A cold sampler keeps + /// errors at rate 1.0 until volume in the current bucket exceeds about 5x + /// the budget. + /// + /// A freeze/thaw gap expires stale counters but does not reset stored rates. + /// A throttled signature's rate recovers by at most 20% per recomputation, + /// so with sparse traffic recovery takes several errored invocations however + /// long the sandbox was frozen. Until then, the lowest stored rate also caps + /// signatures the sampler has not seen yet. pub apm_error_tps: Option, pub span_dedup_timeout: Option, diff --git a/bottlecap/src/traces/trace_processor.rs b/bottlecap/src/traces/trace_processor.rs index c44940e9b..a012e2d54 100644 --- a/bottlecap/src/traces/trace_processor.rs +++ b/bottlecap/src/traces/trace_processor.rs @@ -137,9 +137,11 @@ pub fn error_sampler_config(target_tps: f64) -> ErrorSamplerConfig { /// /// The rolling window is driven by the wall-clock seconds passed on each call, /// so a freeze/thaw gap expires stale counters. Stored per-signature rates are -/// not reset by a gap: a throttled signature recovers by at most 20% per -/// evaluation (inherited from the Go agent). A budget `<= 0` disables rescue; -/// see [`crate::config::LambdaConfig::apm_error_tps`] for the caveats. +/// not reset by a gap: they are recomputed only when a rescue candidate arrives +/// in a new 5s bucket, a throttled signature recovers by at most 20% per +/// recomputation, and the lowest stored rate also caps unseen signatures +/// (inherited from the Go agent). A budget `<= 0` disables rescue; see +/// [`crate::config::LambdaConfig::apm_error_tps`] for the caveats. #[must_use] pub fn new_error_sampler(target_tps: f64) -> Arc> { Arc::new(std::sync::Mutex::new(ErrorsSampler::new( From 8321223af998f32681918c6dc3c123f0254af434 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Tue, 3 Nov 2026 11:30:09 -0500 Subject: [PATCH 06/15] fix(bottlecap): name YAML keys in error sampler warnings --- bottlecap/src/config/mod.rs | 31 +++++++++++++++++-------------- 1 file changed, 17 insertions(+), 14 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index 543cb85d7..a8569c25b 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -425,13 +425,13 @@ impl LambdaConfig { match (self.apm_error_tps, self.serverless_error_sampler_enabled) { (_, None) => None, (Some(tps), Some(enabled)) => Some(format!( - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED={enabled} is deprecated and ignored because DD_APM_ERROR_TPS={tps} is set" + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated; its value {enabled} is ignored because DD_APM_ERROR_TPS (apm_error_tps) is set to {tps}" )), (None, Some(true)) => Some(format!( - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated and has no effect: error trace rescue is on by default at {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS to change the budget" + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: error trace rescue is on by default at {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" )), (None, Some(false)) => Some(String::from( - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated; set DD_APM_ERROR_TPS=0 to disable error trace rescue instead", + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated; set DD_APM_ERROR_TPS (apm_error_tps) to 0 to disable error trace rescue instead", )), } } @@ -986,7 +986,7 @@ mod lambda_config_tests { yaml: None, env: &[(LEGACY, "false")], target: 0.0, - warning_contains: Some(&["DD_APM_ERROR_TPS=0"]), + warning_contains: Some(&["set DD_APM_ERROR_TPS (apm_error_tps) to 0"]), tps_is_none: true, }, ErrorSamplerCase { @@ -994,7 +994,10 @@ mod lambda_config_tests { yaml: Some("serverless_error_sampler_enabled: false\n"), env: &[], target: 0.0, - warning_contains: Some(&["DD_APM_ERROR_TPS=0"]), + warning_contains: Some(&[ + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled)", + "set DD_APM_ERROR_TPS (apm_error_tps) to 0", + ]), tps_is_none: true, }, ErrorSamplerCase { @@ -1003,8 +1006,8 @@ mod lambda_config_tests { env: &[(TPS, "5"), (LEGACY, "false")], target: 5.0, warning_contains: Some(&[ - "DD_APM_ERROR_TPS=5", - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false", + "DD_APM_ERROR_TPS (apm_error_tps) is set to 5", + "its value false is ignored", ]), tps_is_none: false, }, @@ -1014,8 +1017,8 @@ mod lambda_config_tests { env: &[(TPS, "0"), (LEGACY, "true")], target: 0.0, warning_contains: Some(&[ - "DD_APM_ERROR_TPS=0", - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true", + "DD_APM_ERROR_TPS (apm_error_tps) is set to 0", + "its value true is ignored", ]), tps_is_none: false, }, @@ -1025,8 +1028,8 @@ mod lambda_config_tests { env: &[(LEGACY, "false")], target: 5.0, warning_contains: Some(&[ - "DD_APM_ERROR_TPS=5", - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false", + "DD_APM_ERROR_TPS (apm_error_tps) is set to 5", + "its value false is ignored", ]), tps_is_none: false, }, @@ -1035,7 +1038,7 @@ mod lambda_config_tests { yaml: None, env: &[(TPS, "abc"), (LEGACY, "false")], target: 0.0, - warning_contains: Some(&["set DD_APM_ERROR_TPS=0"]), + warning_contains: Some(&["set DD_APM_ERROR_TPS (apm_error_tps) to 0"]), tps_is_none: true, }, ErrorSamplerCase { @@ -1044,8 +1047,8 @@ mod lambda_config_tests { env: &[(TPS, "abc"), (LEGACY, "false")], target: 5.0, warning_contains: Some(&[ - "DD_APM_ERROR_TPS=5", - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false", + "DD_APM_ERROR_TPS (apm_error_tps) is set to 5", + "its value false is ignored", ]), tps_is_none: false, }, From 43e4676ab2d48eb4a38f4480f4a2fea7cd1afa2f Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Tue, 3 Nov 2026 11:30:52 -0500 Subject: [PATCH 07/15] fix(bottlecap): warn when DD_APM_ERROR_TPS cannot apply --- bottlecap/src/startup.rs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/bottlecap/src/startup.rs b/bottlecap/src/startup.rs index 425613393..2179c8e01 100644 --- a/bottlecap/src/startup.rs +++ b/bottlecap/src/startup.rs @@ -162,6 +162,14 @@ pub fn build_trace_agent( } let error_tps = config.ext.error_sampler_target_tps(); + // Rescue currently runs only on the compute-stats path, so an explicit + // budget has no effect without it. + if config.ext.apm_error_tps.is_some() && !config.ext.lambda_extension_compute_stats { + warn!( + "DD_APM_ERROR_TPS (apm_error_tps) has no effect unless DD_LAMBDA_EXTENSION_COMPUTE_STATS (lambda_extension_compute_stats) is true" + ); + } + // One-time startup warning: the feature only widens rescue eligibility, it // does not change the rescue budget. let error_rare_sample_tracer_drop = From 0b6eba84a4d3d74c93e869bdaba10148136c11b3 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Tue, 3 Nov 2026 12:26:09 -0500 Subject: [PATCH 08/15] docs: consolidate error rescue into two PRs --- PLAN-error-sampler-tps.md | 63 +++++++++++++++++++++++++-------------- 1 file changed, 40 insertions(+), 23 deletions(-) diff --git a/PLAN-error-sampler-tps.md b/PLAN-error-sampler-tps.md index e59dde0d4..3c839fc96 100644 --- a/PLAN-error-sampler-tps.md +++ b/PLAN-error-sampler-tps.md @@ -1,28 +1,43 @@ # Plan: rate-limit Lambda error trace rescue with `DD_APM_ERROR_TPS` and deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` -> Temporary planning file, committed on its own so it can be dropped. Delete it before merging the PR. This file plans **PR 1 of 3**; PRs 2 and 3 are listed under "Follow-up PRs" and are out of scope here. +> Temporary planning file, committed separately so its commits can be dropped. Delete it before merging the PR. This file records **PR 1 of 2**; the combined rescue-and-filtering follow-up is existing PR #1358, listed under "Follow-up PR" and out of scope here. ## Status (PR 1) -- **Implemented** on `lpimentel/error-sampler-rate-limited`, rebased onto `origin/main` `f32a2bc0` (#1471), and pushed. No PR opened yet. +- **Implemented** on `lpimentel/error-sampler-rate-limited` and open as draft [PR #1490](https://github.com/DataDog/datadog-lambda-extension/pull/1490), targeting `main`. - Commits: this plan file, then `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` (step 1), then `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS` (steps 2-4, with review nits and test simplifications squashed in). - Final verification passed: `cargo fmt --check`, `cargo test --workspace`, the test-mode bin, and clippy `-D warnings` for default, default+test-mode, and fips. Greps for `AlwaysKeep`, `not supported by the Lambda extension`, `new_error_sampler(true|false)` and added em dashes are all clean. - Not verified: the "make the shared run reset per call and watch the shared-budget test fail" check from step 3. -- Remaining: open the PR (text under "Commits and PR"), and drop this plan commit (or add a commit deleting the file) before merge. +- Remaining: update #1490's description from a three-PR series to the two-PR plan below, and drop all plan-file commits (or add a commit deleting the file) before merge. - Deviations from the steps below, as implemented: - The step 2 table rows are data (`yaml: Option<&str>`, `env: &[(key, value)]`, `tps_is_none: bool`) applied by one loop. The "no removal promise" check runs inside that loop on every warning row instead of in a separate test. The legacy raw-field check was dropped, since a warning exists exactly when the legacy flag is set. - The non-finite warning reads `Ignoring non-finite DD_APM_ERROR_TPS (apm_error_tps) value {v}`, so it also covers YAML. - The test helper `enabled_error_sampler()` is renamed `default_error_sampler()` (20 call sites in `trace_processor.rs` and `lifecycle/invocation/processor.rs`). - The two sampler-level tests share a `sample_errored_trace(sampler, now, n)` helper. - `DEFAULT_APM_ERROR_TPS` is declared after the `use datadog_agent_config::{...}` block. + - A later startup change warns when a valid explicit TPS is set but `lambda_extension_compute_stats` is false, because PR 1 only rescues on the extension-configured path. Revisit this warning in #1358: tracer-computed stats will also enable rescue, and startup cannot know that per-request signal. ## PR series | PR | Scope | Base | |---|---|---| -| **1 (this plan)** | `DD_APM_ERROR_TPS` budget, `RateLimited` mode, deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. Rescue gating unchanged: only when the extension computes stats. | `main` | -| 2 | Also run rescue when the tracer computes stats (`Datadog-Client-Computed-Stats: true`). On that path rescue only stamps `_dd.errors_sr` on kept errored P0 chunks; every chunk is still forwarded, so the backend's P0 drop keeps the stamped ones (same model as SCL, APMSVLS-472). | PR 1 | -| 3 | DataDog/datadog-lambda-extension#1358, rebased onto PR 2 and trimmed to filtering sampled-out chunks when the tracer computes stats. | PR 2 | +| **1: #1490 (this plan)** | `DD_APM_ERROR_TPS` budget, `RateLimited` mode, deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. Preserve the existing config-only rescue/filtering gate, including when the tracer also reports computed stats. | `main` | +| **2: #1358** | Keep its existing scope: extend both error rescue and sampled-out filtering to tracer-computed stats. Preserve its review history; do not split out a stamp-and-forward intermediate PR. | `main` | -Order matters: filtering on the tracer path before rescue exists there would drop errored P0 chunks unrescued. Today tracers still send P0 chunks because `/info` reports `client_drop_p0s: false` (`bottlecap/src/traces/trace_agent.rs:653`). +**Preferred merge order:** merge #1490 first, then update #1358 against `main`, resolve conflicts, update warnings/docs, and test the combined behavior. There is no requirement to stack #1358 onto the unmerged #1490 branch. + +**Independent functionality:** #1358 can merge first using the existing `AlwaysKeep` behavior, with #1490 subsequently adding rate limiting. Both PRs currently target `main`, but a read-only merge check found conflicts between their heads in config documentation and trace-processor tests. Whichever merges second needs reconciliation and validation; they cannot both merge unchanged. + +Rescue and filtering must widen together in #1358 so newly filtered errored chunks get a rescue decision first. The existing helper already does both. Today tracers still send P0 chunks because `/info` reports `client_drop_p0s: false` (`bottlecap/src/traces/trace_agent.rs:653`, original reference). + +After both PRs, use this ownership contract: + +| Extension stats configured | Tracer computed stats | Stats owner | Rescue/filtering | +|---|---|---|---| +| false | false | Backend | No rescue; forward every chunk for backend stats | +| true | false | Extension | Rescue eligible errors; filter unrescued sampled-out chunks | +| false | true | Tracer | Rescue eligible errors; filter unrescued sampled-out chunks | +| true | true | Tracer | Rescue eligible errors; preserve existing local filtering | + +There is no stamp-and-forward-only release or temporary forwarding change for the both-flags combination. ## Goal Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescue always uses the shared `ErrorSamplerMode::RateLimited` sampler with Go agent / SCL trace-agent defaults: rescue is on at 10 TPS when nothing is set, and `<= 0` turns it off. `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` stays parsed but deprecated. @@ -58,14 +73,14 @@ Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescu 6. Large finite values (for example `1000000000`) are accepted as-is, no cap and no warning (Go/SCL parity). 7. Configuration keys: env `DD_APM_ERROR_TPS` (parity with the Go trace-agent's env var) and flat YAML `apm_error_tps`. Go's YAML key `apm_config.errors_per_second` is intentionally not supported: parity target is env vars, not YAML files. 8. Deprecation warnings do not promise removal (no "will be removed" wording). -9. No "setting has no effect" warning when compute stats is off: whether the tracer computes stats is known only per request, and PR 2 makes rescue apply there too. +9. PR 1 currently warns when an explicit valid TPS is set without extension stats configured. In #1358, remove or revise that unconditional startup warning: rescue will also apply when the tracer reports computed stats, which is known only per request. ## Constraints - TDD: write each step's tests first, watch them fail (or fail to compile), then implement. - Every step must leave the whole crate compiling (`cargo test --lib` builds all lib tests regardless of name filter). Each step therefore updates every caller its type or signature change breaks; do not defer callers to a later step. - Keep `lambda_extension_compute_stats` gating unchanged (`trace_processor.rs:638`). - Keep comments concise and leave history out of them. No em dashes in code comments or strings. -- Non-goals: removing `AlwaysKeep` from the shared crate, stats-payload `errorTPS`, changing compute-stats gating (PR 2), tracer-path filtering (PR 3), public docs. The repo has no changelog and no doc mentions `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. +- Non-goals for PR 1: removing `AlwaysKeep` from the shared crate, stats-payload `errorTPS`, widening rescue or filtering to the tracer-stats path (both belong to #1358), public docs. The repo has no changelog and no doc mentions `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. - Do not commit or push unless the user asks. When asked, make two commits (step 1 alone, then everything else). Commit messages must be user-facing. - The PR description must not include test counts. Use `.github/pull_request_template.md` (`## Overview`, `## Testing`). No Jira ticket in the PR title; mention APMSVLS-469 in the description. @@ -195,7 +210,7 @@ Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescu ``` 3. Retarget the rare-sample warning (:172) to `if error_rare_sample_tracer_drop && error_tps <= 0.0`. Message: `"DD_APM_FEATURES includes error_rare_sample_tracer_drop, but error trace rescue is disabled (effective DD_APM_ERROR_TPS {error_tps}): errored traces dropped by the tracer will not be rescued"`. 4. Build the sampler with `trace_processor::new_error_sampler(error_tps)` (reusing the local instead of the step 3 inline call). -5. Do not add a warning for "rescue configured but compute stats off" (settled semantics 9). +5. Keep PR 1's later startup warning for an explicit valid TPS with extension stats off; see settled semantics 9. Remove or revise it in #1358 when tracer-computed stats also enable rescue. 6. Verify: `cargo build` succeeds and `cargo test --lib startup` passes. The warning logic itself is covered by the step 2 tests on `legacy_error_sampler_warning` and `error_sampler_target_tps`. ## Files to Modify @@ -226,7 +241,7 @@ Run everything from `bottlecap/` and report the pass/fail counts. 8. Grep the changed lines for `—` (em dash): zero matches. ## Commits and PR (only when the user asks) -Both commits are done (see Status). The PR is not opened yet. +Both original code commits are done (see Status), with later follow-up fixes. Draft PR #1490 is open. - Commit 1 (step 1 only): `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` - Commit 2 (steps 2-4): `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS`. Body: @@ -235,7 +250,7 @@ Both commits are done (see Status). The PR is not opened yet. - `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` is deprecated and logs a warning. - PR title: `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS` (no ticket). - `## Overview`: - - Follow-up to APMSVLS-469 (the bottlecap error sampler) and part 1 of 3; name PRs 2 and 3 briefly. + - Follow-up to APMSVLS-469 (the bottlecap error sampler) and part 1 of 2; name #1358 as the combined tracer-path rescue-and-filtering follow-up. - The semantics table. - Env parity with the Go trace-agent's `DD_APM_ERROR_TPS`; YAML key is flat `apm_error_tps`. - The behavior change: compute-stats users who never set the flag now get rescue at 10/s. @@ -255,15 +270,17 @@ Both commits are done (see Status). The PR is not opened yet. - **Duplicate warnings.** The non-finite warning in the deserializer can fire once per source (YAML and env). That is acceptable at startup. - **Lints.** Clippy pedantic needs `#[must_use]` on the new pub fns and backticks around identifiers in docs (`doc_markdown`). Use `.expect("...")`, not `.unwrap()`, in new tests unless the module already has `#[allow(clippy::unwrap_used)]` (the config test module does; the trace_processor test module relies on `expect`). -## Follow-up PRs (out of scope for this plan; plan each in its own session) - -### PR 2: rescue when the tracer computes stats -- Base: PR 1's branch (or `main` after PR 1 merges). -- Run the error sampler when either the extension or the tracer computes stats (resolve stats ownership once in `process_traces`, as #1358 does today). -- Tracer-owned path: do not drop any chunk. For errored P0 chunks (plus negative-priority non-manual drops under `error_rare_sample_tracer_drop`), call the sampler and stamp `_dd.errors_sr` on `Keep`; forward everything. The backend's P0 drop then keeps the stamped chunks. -- Extension-owned path: unchanged from PR 1 (rescue + drop). -- Update the `apm_error_tps` doc ("rescue runs when the extension or the tracer computes stats"). -- Source material: the rescue-widening half of #1358 (`lpimentel/filter-sampled-traces-by-stats-owner`). +## Follow-up PR (out of scope for PR 1) -### PR 3: filter sampled-out chunks when the tracer computes stats -- Rebase DataDog/datadog-lambda-extension#1358 onto PR 2 so it only adds dropping of sampled-out chunks on the tracer-owned path (rescued chunks kept). Keep its review history; update its title and description. +### PR 2: #1358, rescue and filter when the tracer computes stats +- Keep [DataDog/datadog-lambda-extension#1358](https://github.com/DataDog/datadog-lambda-extension/pull/1358), branch `lpimentel/filter-sampled-traces-by-stats-owner`, with its current combined scope and review history. Do not create a separate stamp-only rescue PR or trim #1358 to filtering alone. +- Prefer to merge #1490 first, then update #1358 against `main`. Rebasing onto the unmerged #1490 branch is optional, not a dependency. Do not rebase, commit, or push without user approval. +- Resolve overlapping config docs and trace-processor tests. Retain #1490's optional raw config fields, TPS resolution, `RateLimited` constructor, `default_error_sampler()` helper, and sampler regression coverage; do not restore the old boolean or `AlwaysKeep` wiring. +- Resolve stats ownership once in `process_traces` and reuse it for `_dd.compute_stats` stamping and the rescue/filtering gate. Tracer-computed stats win even when extension stats are configured. +- Run the existing rescue-and-filter pass whenever ownership is Extension or Tracer. Keep eligible errored P0 chunks on sampler `Keep`, stamp `_dd.errors_sr`, and filter unrescued sampled-out chunks. Preserve negative-priority eligibility under `error_rare_sample_tracer_drop`, manual-drop exclusions, absent-priority behavior, and unchanged priorities/decision makers. +- Preserve one shared TPS budget per execution environment across requests and stats-ownership paths. `DD_APM_ERROR_TPS <= 0` disables rescue, not filtering. +- Leave the Backend-owned path unchanged: no rescue, forward every chunk. Preserve the pre-filter stats payload; submit it to the extension's stats generator only when the extension owns stats. +- Update `apm_error_tps` and sampler docs to cover extension- or tracer-computed stats. Remove or revise #1490's unconditional "TPS has no effect unless extension stats are enabled" startup warning, since tracer ownership is request-specific. +- Extend unit coverage across all four ownership combinations, including rescue, filtering, disabled budgets, negative-priority feature behavior, manual-drop exclusions, original priorities, and the pre-filter stats payload. Verify that extension and tracer paths consume the same sampler budget. +- Retain and adapt #1358's fake-intake E2E coverage for filtering and error rescue on both non-backend paths; assert that tracer-owned requests do not generate duplicate extension stats. +- Before merging, rerun the workspace tests, test-mode binary tests, formatting, and default/test-mode/FIPS clippy checks listed above. Report counts in the session, not the PR description; update the PR description to describe the rate-limited combined behavior. From aad7ac1a764194f2d834e6543924625df5a763cd Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Wed, 4 Nov 2026 10:59:21 -0500 Subject: [PATCH 09/15] docs: remove temporary error sampler plan --- PLAN-error-sampler-tps.md | 286 -------------------------------------- 1 file changed, 286 deletions(-) delete mode 100644 PLAN-error-sampler-tps.md diff --git a/PLAN-error-sampler-tps.md b/PLAN-error-sampler-tps.md deleted file mode 100644 index 3c839fc96..000000000 --- a/PLAN-error-sampler-tps.md +++ /dev/null @@ -1,286 +0,0 @@ -# Plan: rate-limit Lambda error trace rescue with `DD_APM_ERROR_TPS` and deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` - -> Temporary planning file, committed separately so its commits can be dropped. Delete it before merging the PR. This file records **PR 1 of 2**; the combined rescue-and-filtering follow-up is existing PR #1358, listed under "Follow-up PR" and out of scope here. - -## Status (PR 1) -- **Implemented** on `lpimentel/error-sampler-rate-limited` and open as draft [PR #1490](https://github.com/DataDog/datadog-lambda-extension/pull/1490), targeting `main`. -- Commits: this plan file, then `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` (step 1), then `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS` (steps 2-4, with review nits and test simplifications squashed in). -- Final verification passed: `cargo fmt --check`, `cargo test --workspace`, the test-mode bin, and clippy `-D warnings` for default, default+test-mode, and fips. Greps for `AlwaysKeep`, `not supported by the Lambda extension`, `new_error_sampler(true|false)` and added em dashes are all clean. -- Not verified: the "make the shared run reset per call and watch the shared-budget test fail" check from step 3. -- Remaining: update #1490's description from a three-PR series to the two-PR plan below, and drop all plan-file commits (or add a commit deleting the file) before merge. -- Deviations from the steps below, as implemented: - - The step 2 table rows are data (`yaml: Option<&str>`, `env: &[(key, value)]`, `tps_is_none: bool`) applied by one loop. The "no removal promise" check runs inside that loop on every warning row instead of in a separate test. The legacy raw-field check was dropped, since a warning exists exactly when the legacy flag is set. - - The non-finite warning reads `Ignoring non-finite DD_APM_ERROR_TPS (apm_error_tps) value {v}`, so it also covers YAML. - - The test helper `enabled_error_sampler()` is renamed `default_error_sampler()` (20 call sites in `trace_processor.rs` and `lifecycle/invocation/processor.rs`). - - The two sampler-level tests share a `sample_errored_trace(sampler, now, n)` helper. - - `DEFAULT_APM_ERROR_TPS` is declared after the `use datadog_agent_config::{...}` block. - - A later startup change warns when a valid explicit TPS is set but `lambda_extension_compute_stats` is false, because PR 1 only rescues on the extension-configured path. Revisit this warning in #1358: tracer-computed stats will also enable rescue, and startup cannot know that per-request signal. - -## PR series -| PR | Scope | Base | -|---|---|---| -| **1: #1490 (this plan)** | `DD_APM_ERROR_TPS` budget, `RateLimited` mode, deprecate `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. Preserve the existing config-only rescue/filtering gate, including when the tracer also reports computed stats. | `main` | -| **2: #1358** | Keep its existing scope: extend both error rescue and sampled-out filtering to tracer-computed stats. Preserve its review history; do not split out a stamp-and-forward intermediate PR. | `main` | - -**Preferred merge order:** merge #1490 first, then update #1358 against `main`, resolve conflicts, update warnings/docs, and test the combined behavior. There is no requirement to stack #1358 onto the unmerged #1490 branch. - -**Independent functionality:** #1358 can merge first using the existing `AlwaysKeep` behavior, with #1490 subsequently adding rate limiting. Both PRs currently target `main`, but a read-only merge check found conflicts between their heads in config documentation and trace-processor tests. Whichever merges second needs reconciliation and validation; they cannot both merge unchanged. - -Rescue and filtering must widen together in #1358 so newly filtered errored chunks get a rescue decision first. The existing helper already does both. Today tracers still send P0 chunks because `/info` reports `client_drop_p0s: false` (`bottlecap/src/traces/trace_agent.rs:653`, original reference). - -After both PRs, use this ownership contract: - -| Extension stats configured | Tracer computed stats | Stats owner | Rescue/filtering | -|---|---|---|---| -| false | false | Backend | No rescue; forward every chunk for backend stats | -| true | false | Extension | Rescue eligible errors; filter unrescued sampled-out chunks | -| false | true | Tracer | Rescue eligible errors; filter unrescued sampled-out chunks | -| true | true | Tracer | Rescue eligible errors; preserve existing local filtering | - -There is no stamp-and-forward-only release or temporary forwarding change for the both-flags combination. - -## Goal -Make `DD_APM_ERROR_TPS` the only knob for error trace rescue in bottlecap. Rescue always uses the shared `ErrorSamplerMode::RateLimited` sampler with Go agent / SCL trace-agent defaults: rescue is on at 10 TPS when nothing is set, and `<= 0` turns it off. `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` stays parsed but deprecated. - -## Context (verified by reading the code) -- Repo: DataDog/datadog-lambda-extension. Worktree: `/home/lucas/source/datadog/datadog-lambda-extension--lpimentel-error-sampler-rate-limited`. Branch: `lpimentel/error-sampler-rate-limited`, now based on `origin/main` `f32a2bc0`. Line references below are from the original base (`aa9d3f54`) and may have drifted by a few lines. All Rust code is under `bottlecap/`. Toolchain is 1.93.1. -- `AGENTS.md` (after #1424) says to check `serverless-components` before adding functionality. This plan adds no new shared logic: the sampler is already the shared crate, and the new code is bottlecap's `LambdaConfig` extension field plus its resolution (bottlecap-specific because of the deprecated flag). SCL's `parse_error_sampler_config` lives in `datadog-trace-agent`, not in a shared crate, so there is nothing to reuse. -- Sampler crate `datadog-agent-trace-sampler` is pinned at serverless-components rev `e462105e918904e6b1ad2ebc153ae561cebe5a34` (`bottlecap/Cargo.toml:100`). Do not bump it. Its source is at `~/.cargo/git/checkouts/serverless-components-d7345b4db0f8d2aa/e462105/crates/datadog-agent-trace-sampler/src/{lib.rs,score_sampler.rs}`. What it provides: - - `ErrorSamplerConfig { mode, target_tps, extra_sample_rate }`. `Default` is `RateLimited`, `10.0`, `1.0`. - - `ErrorsSampler` is a public enum with variants `AlwaysKeep { disabled }` and `RateLimited(Box)`. Tests can `matches!` on it. - - `is_disabled()` returns `target_tps <= 0.0`. - - `sample(now_unix_secs, &TraceView)` rotates its buckets only from the `now_unix_secs` argument. `bucket_index` uses `rem_euclid`. A backward clock step (`new_bucket < prev`) zeroes nothing and does not panic. A gap longer than 30s zeroes every bucket. -- Config loading: `datadog-agent-config` builds the YAML source and then the env source. For each source it runs `figment.extract::()` and calls `LambdaConfig::merge_from`. - - Env provider: `Env::prefixed("DD_")`. A source field named `apm_error_tps` therefore maps to `DD_APM_ERROR_TPS` and to the flat YAML key `apm_error_tps`. - - The core `Config` has no `error_tps` field, so the new field does not collide with anything. - - The `merge_fields!` `option:` group copies the value only when the source value is `Some`. -- Figment parses env values as follows (`figment-0.10.19/src/value/parse.rs`): - - `"5"` and `"-1"` become integers, `"2.5"` becomes f64, and `" 7 "` is trimmed. - - `""`, `"abc"`, `"inf"`, `"nan"` and `"1e400"` stay strings. `deserialize_option_lossless::` then warns and returns `None`. - - `"1.0e999"` contains a `.`, so it parses to f64 `inf` and passes `deserialize_option_lossless`. - - YAML `.inf` and `.nan` also arrive as non-finite f64. - - **A finiteness filter is therefore required after deserialization.** -- Logging is initialized before config load (`bottlecap/src/bin/bottlecap/main.rs:116` vs `:164`), so warnings from the deserializer are visible. -- Lints: `bottlecap/src/lib.rs:2-10` denies `clippy::all`, `clippy::pedantic`, `clippy::unwrap_used`, `unreachable_pub` and `missing_copy_implementations`. `clippy::float_cmp` is pedantic; the repo convention is `#[allow(clippy::float_cmp)]` on the test (see `src/metrics/enhanced/lambda.rs:876`). Pedantic also requires `#[must_use]` on value-returning pub fns and backticks around identifiers in doc comments. -- SCL reference: DataDog/serverless-components branch `lpimentel/add-error-sampler`, `crates/datadog-trace-agent/src/config.rs`, function `parse_error_sampler_config`. It starts from `ErrorSamplerConfig::default()` and only overrides `target_tps`. Blank, unparsable and non-finite values warn and fall back to the default. - -## Settled semantics (do not reopen) -1. `DD_APM_ERROR_TPS` (f64, YAML `apm_error_tps`) is the budget. Mode is always `RateLimited` and `extra_sample_rate` is always `1.0`. Bottlecap can never select `AlwaysKeep`. -2. With nothing set, the target is `10.0`, which equals `ErrorSamplerConfig::default().target_tps`. A target `<= 0` disables rescue. -3. `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` is deprecated but still parsed. Warn whenever it is explicitly set. `true` is redundant (target 10). `false` with no usable TPS is a kill switch (target 0). -4. If both are set, TPS wins, and the warning names both values. This holds regardless of which source (YAML or env) each came from. -5. An invalid, blank or non-finite TPS warns and is ignored for that source only. Resolution then uses any remaining valid TPS from a lower-priority source (YAML under env), then the legacy flag, then 10.0. Example: YAML `apm_error_tps: 5` + env `DD_APM_ERROR_TPS=abc` + `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` resolves to 5. -6. Large finite values (for example `1000000000`) are accepted as-is, no cap and no warning (Go/SCL parity). -7. Configuration keys: env `DD_APM_ERROR_TPS` (parity with the Go trace-agent's env var) and flat YAML `apm_error_tps`. Go's YAML key `apm_config.errors_per_second` is intentionally not supported: parity target is env vars, not YAML files. -8. Deprecation warnings do not promise removal (no "will be removed" wording). -9. PR 1 currently warns when an explicit valid TPS is set without extension stats configured. In #1358, remove or revise that unconditional startup warning: rescue will also apply when the tracer reports computed stats, which is known only per request. - -## Constraints -- TDD: write each step's tests first, watch them fail (or fail to compile), then implement. -- Every step must leave the whole crate compiling (`cargo test --lib` builds all lib tests regardless of name filter). Each step therefore updates every caller its type or signature change breaks; do not defer callers to a later step. -- Keep `lambda_extension_compute_stats` gating unchanged (`trace_processor.rs:638`). -- Keep comments concise and leave history out of them. No em dashes in code comments or strings. -- Non-goals for PR 1: removing `AlwaysKeep` from the shared crate, stats-payload `errorTPS`, widening rescue or filtering to the tracer-stats path (both belong to #1358), public docs. The repo has no changelog and no doc mentions `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`. -- Do not commit or push unless the user asks. When asked, make two commits (step 1 alone, then everything else). Commit messages must be user-facing. -- The PR description must not include test counts. Use `.github/pull_request_template.md` (`## Overview`, `## Testing`). No Jira ticket in the PR title; mention APMSVLS-469 in the description. - -## Plan - -### Step 1: stale `DD_APM_FEATURES` comment (separate commit) -1. Test first: in `bottlecap/src/config/mod.rs` `lambda_config_tests` (next to `apm_features_comma_separated_list_from_env`, ~:767), add `apm_features_space_separated_list_from_env`. - - Set `DD_APM_FEATURES="some_other_feature error_rare_sample_tracer_drop"`. - - Assert `config.apm_features == ["some_other_feature", "error_rare_sample_tracer_drop"]` and `error_rare_sample_tracer_drop_enabled(&config)`. - - It should pass immediately: it is a regression test, because the pinned crate uses `deserialize_comma_then_space_separated_string` (`datadog-agent-config/src/sources/env.rs:237`). -2. In `bottlecap/src/traces/trace_processor.rs:81-84`, replace "parsed upstream as a comma-separated list ... space-separated values are not supported" with this: `DD_APM_FEATURES` is split on commas when one is present, otherwise on spaces, and matching is exact per entry. -3. Verify: run `cd bottlecap && cargo test --lib apm_features`. All `apm_features*` tests pass. - -### Step 2: config resolution in `bottlecap/src/config/mod.rs` -1. Tests first, in `lambda_config_tests`: - - Replace the three tests at ~:718-742 (`serverless_error_sampler_enabled_*`) with table-driven tests. Each case calls `load(|jail| ...)`, then asserts `config.ext.error_sampler_target_tps()` and whether `config.ext.legacy_error_sampler_warning()` is `Some`. - - Put `#[allow(clippy::float_cmp)]` on the tests, and include the case input in every assert message. - - | Case | Setup | Target | Warning | - |---|---|---|---| - | default | nothing set (also assert `apm_error_tps == None`, `serverless_error_sampler_enabled == None`) | 10.0 | None | - | env int | `DD_APM_ERROR_TPS=5` | 5.0 | None | - | env float | `DD_APM_ERROR_TPS=2.5` | 2.5 | None | - | env padded | `DD_APM_ERROR_TPS=" 7 "` | 7.0 | None | - | YAML | `apm_error_tps: 3` | 3.0 | None | - | env over YAML | YAML `apm_error_tps: 3` + env `DD_APM_ERROR_TPS=4` | 4.0 | None | - | zero | `DD_APM_ERROR_TPS=0` | 0.0 | None | - | negative | `DD_APM_ERROR_TPS=-1` | -1.0 | None | - | large | `DD_APM_ERROR_TPS=1000000000` | 1e9 | None | - | invalid env | `DD_APM_ERROR_TPS` = `""`, `"abc"`, `"inf"`, `"nan"`, `"1.0e999"` (assert `apm_error_tps == None`) | 10.0 | None | - | invalid YAML | `apm_error_tps: .inf`, `apm_error_tps: .nan` (assert `apm_error_tps == None`) | 10.0 | None | - | legacy true | `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true` | 10.0 | Some, contains `deprecated` | - | legacy false | `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` | 0.0 | Some, contains `DD_APM_ERROR_TPS=0` (migration hint) | - | legacy YAML false | `serverless_error_sampler_enabled: false` | 0.0 | Some | - | both, TPS 5 + legacy false | env both | 5.0 | Some, contains `DD_APM_ERROR_TPS=5` and `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` | - | both, TPS 0 + legacy true | env both | 0.0 | Some, contains `DD_APM_ERROR_TPS=0` and `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true` | - | cross-source | YAML `apm_error_tps: 5` + env `DD_SERVERLESS_ERROR_SAMPLER_ENABLED=false` | 5.0 | Some, names both | - | invalid TPS + legacy false | `DD_APM_ERROR_TPS=abc` + legacy `false` | 0.0 | Some, the kill-switch message, not the both-set one | - | invalid env over valid YAML + legacy false | YAML `apm_error_tps: 5` + env `DD_APM_ERROR_TPS=abc` + env legacy `false` | 5.0 | Some, the both-set message naming `DD_APM_ERROR_TPS=5` | - - Rust formats `5.0_f64` with `{}` as `5`, so `"DD_APM_ERROR_TPS=5"` matches. - - Resolved: figment only tries a float parse when the text contains `.` (`figment-0.10.19/src/value/parse.rs:90-100`, and again in `value.rs:512-532`). So `DD_APM_ERROR_TPS=1e3` stays a string and is rejected (target 10.0, `apm_error_tps == None`), while `1.0e3` parses to 1000.0. The table has both rows, and `1e400` is rejected the same way. - - - Add `default_apm_error_tps_matches_shared_sampler_default`, asserting `DEFAULT_APM_ERROR_TPS == datadog_agent_trace_sampler::ErrorSamplerConfig::default().target_tps`. - - Update `apm_features_error_rare_sample_tracer_drop_from_env` (~:752-764). Replace `assert!(!config.ext.serverless_error_sampler_enabled)` with `assert_eq!(config.ext.error_sampler_target_tps(), DEFAULT_APM_ERROR_TPS)`. Reword its comment to "The feature only widens rescue eligibility; it must not change the rescue budget." - - Verify: `cargo test --lib lambda_config_tests` fails to compile (red). -2. Add `pub const DEFAULT_APM_ERROR_TPS: f64 = 10.0;` near the top of the LambdaConfig section. Its doc comment should say it matches the shared sampler and Go agent default. -3. Add a private deserializer `deserialize_optional_finite_f64<'de, D: serde::Deserializer<'de>>(d: D) -> Result, D::Error>`. - - Call `deser_opt_lossless::<_, f64>(d)?`, then filter out non-finite values. - - When a value is dropped, call `tracing::warn!("Ignoring non-finite DD_APM_ERROR_TPS (apm_error_tps) value {v}")` (fully qualified, since `tracing` is not imported in this file). -4. `LambdaConfig` (~:72-79, default ~:147): - - Change `serverless_error_sampler_enabled` to `Option` and give it default `None`. - - Rewrite its doc: deprecated raw value of `DD_SERVERLESS_ERROR_SAMPLER_ENABLED`; read it only through `error_sampler_target_tps` and `legacy_error_sampler_warning`. - - Add `pub apm_error_tps: Option` with default `None`. Its doc should cover: - - `DD_APM_ERROR_TPS`: the rescue budget in errored traces per second, distributed fairly across trace signatures; `<= 0` disables rescue. - - The budget is per execution environment, so N concurrent sandboxes can rescue up to N times the budget. - - Rescue currently runs only when the extension computes stats (`lambda_extension_compute_stats`). Word this as current behavior; PR 2 widens it. - - The limit is approximate, with no hard ceiling (inherited from the Go agent and SCL). Rates are recomputed on 5s bucket rotation over a 30s window. A cold sampler keeps errors at rate 1.0 until volume in the current bucket exceeds about 5x the budget. After a freeze/thaw gap, counters expire but a throttled signature's rate recovers by at most 20% per evaluation. See Risks for why this wording differs from "unknown signatures sample at 1.0". - - `PartialEq` still derives fine with an f64 field. - - Keep the type change compiling: in `bottlecap/src/startup.rs`, change `!config.ext.serverless_error_sampler_enabled` (:172) to `config.ext.serverless_error_sampler_enabled != Some(true)` and the argument at :181 to `config.ext.serverless_error_sampler_enabled == Some(true)`. These are temporary; steps 3 and 4 replace them. Fix any other non-test reader the compiler reports the same way. -5. `LambdaConfigSource` (~:213-214): keep `serverless_error_sampler_enabled` (still `deser_opt_bool`). Add `#[serde(deserialize_with = "deserialize_optional_finite_f64")] pub apm_error_tps: Option` with a one-line `DD_APM_ERROR_TPS` doc. -6. `merge_from` (~:297): move `serverless_error_sampler_enabled` from the `value:` list to the `option:` list, and add `apm_error_tps` to `option:`. -7. In `impl LambdaConfig` (~:347), add two methods, both `#[must_use]` and `pub`: - - `error_sampler_target_tps(&self) -> f64`: - ``` - match (self.apm_error_tps, self.serverless_error_sampler_enabled) { - (Some(tps), _) => tps, - (None, Some(false)) => 0.0, - _ => DEFAULT_APM_ERROR_TPS, - } - ``` - - `legacy_error_sampler_warning(&self) -> Option`: - - `None` when the legacy flag is `None`. - - (TPS `Some`, legacy `Some(b)`): `"DD_SERVERLESS_ERROR_SAMPLER_ENABLED={b} is deprecated and ignored because DD_APM_ERROR_TPS={tps} is set"`. - - (TPS `None`, `true`): `"DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated and has no effect: error trace rescue is on by default at {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS to change the budget"`. - - (TPS `None`, `false`): `"DD_SERVERLESS_ERROR_SAMPLER_ENABLED is deprecated; set DD_APM_ERROR_TPS=0 to disable error trace rescue instead"`. - - No em dashes, and no removal promise in any message. Add a test asserting none of the three messages contains `removed`. -8. Verify: `cargo build --all-targets` compiles, and `cargo test --lib lambda_config_tests` passes, including `defaults_match_lambda_config_default`. - -### Step 3: sampler construction in `bottlecap/src/traces/trace_processor.rs`, plus every caller -1. Tests first, in `mod tests`: - - `error_sampler_config_is_rate_limited_with_given_tps`: for tps in `[10.0, 2.5, 0.0, -1.0]`, assert `error_sampler_config(tps)` has `mode == ErrorSamplerMode::RateLimited`, `target_tps == tps` and `extra_sample_rate == 1.0`. Use `#[allow(clippy::float_cmp)]`. - - `new_error_sampler_builds_rate_limited`: assert `matches!(*new_error_sampler(10.0).lock().expect("lock"), ErrorsSampler::RateLimited(_))`. For `[0.0, -1.0]` assert `is_disabled()`. For `10.0` assert `!is_disabled()`. - - `error_sampler_survives_clock_jumps`: - - Build `new_error_sampler(DEFAULT_APM_ERROR_TPS)` and a single errored `SpanView` (the crate tests show the shape). - - Call `.sample(now, &trace)` in sequence with now = `1_700_000_000`, `1_699_999_000` (backward), `1_700_100_000` (long gap), `0` (the `UNIX_EPOCH.elapsed()` fallback). - - Use distinct trace ids (`i.wrapping_mul(2_654_435_761)`). - - Assert no panic, and that the call after the long gap returns `SampleDecision::Keep { errors_sr: 1.0 }` (cold, unthrottled sampler). - - `error_sampler_stays_throttled_after_long_gap` (documents inherited behavior, deterministic timestamps): - - Build `new_error_sampler(1.0)`. Drive one errored signature with 200 traces in each of several consecutive 5s buckets (timestamps `t0 + 5*k`, distinct trace ids) so its stored rate drops well below 1.0. - - Then sample once at `t_last + 3600` (gap far beyond the 30s window). - - Assert no panic and that the returned decision, if `Keep`, has `errors_sr < 1.0`: stale counters are cleared, but the stored rate only recovers by at most 1.2x per evaluation (`MAX_RATE_INCREASE`, `score_sampler.rs:21,101-107`). If the hash makes the single decision a `Drop`, use a few distinct trace ids and assert the first `Keep` has `errors_sr < 1.0`. - - `error_sampler_budget_is_shared_across_invocations` (shared processor versus a deliberately reset control): - - Build `create_compute_stats_config()`. - - Shared: one `create_test_processor(&config, new_error_sampler(1.0), false)`, then 10 separate `process_traces` calls (one per simulated invocation). Each call carries 50 single-span errored P0 traces of the same signature with distinct nonzero trace ids (`(call * 50 + i + 1).wrapping_mul(2_654_435_761)`). - - Control: the same 10 batches and ids, but a fresh `create_test_processor(&config, new_error_sampler(1.0), false)` per call. - - Count rescued spans for each with the existing `trace_spans` helper (`trace_processor.rs:1599`). - - Assert `shared > 0` and `shared * 2 < control`. A deterministic simulation of the pinned sampler gives about 29 shared versus about 160 reset, so the margin is wide. A bucket rotation mid-test only lowers `shared` further, since rotation applies the throttled rate. - - Verify the test discriminates: temporarily make the shared run build a fresh sampler per call and confirm the assertion fails, then revert. - - In `test_error_sampler_rescues_errored_p0_chunks` (~:1477), add an assert that the rescued root's `_dd.errors_sr` is `1.0` (cold sampler, low volume). Add `#[allow(clippy::float_cmp)]`. - - In `test_disabled_error_sampler_drops_errored_p0_chunks` (~:1827), loop over `[0.0, -1.0]` to cover the `is_disabled` short-circuit with a negative budget. Change "(the shipping default)" in its doc (~:1825) to "(`DD_APM_ERROR_TPS <= 0`)". - - Verify: tests fail to compile (red). -2. Replace `new_error_sampler(enabled: bool)` (~:119-138): - - Add `#[must_use] pub fn error_sampler_config(target_tps: f64) -> ErrorSamplerConfig`. It returns `ErrorSamplerConfig { mode: ErrorSamplerMode::RateLimited, target_tps, extra_sample_rate: 1.0 }`. Set the fields explicitly so a crate default change cannot alter bottlecap behavior. - - Add `#[must_use] pub fn new_error_sampler(target_tps: f64) -> Arc>`, which wraps `ErrorsSampler::new(error_sampler_config(target_tps))`. - - Rewrite the doc comment. Drop the wrong freeze/thaw claim. Say instead: - - The rolling window is driven by the wall-clock seconds passed on each call, so a freeze/thaw gap expires stale counters. Stored per-signature rates are not reset: a throttled signature recovers by at most 20% per evaluation (inherited from the Go agent). - - `<= 0` disables rescue. - - Point to `LambdaConfig::apm_error_tps` for caveats. -3. Field doc (~:96-100): "a stateful mode (`RateLimited`) would keep its rolling window" becomes "Shared across invocations so the `RateLimited` budget and rolling window span the whole execution environment". -4. Test helper (~:792-796): `enabled_error_sampler()` returns `new_error_sampler(crate::config::DEFAULT_APM_ERROR_TPS)`. Change the doc to "Error sampler with the default budget". -5. In the same file, replace `new_error_sampler(false)` with `new_error_sampler(0.0)` at ~:1763, ~:1830 and ~:1862. -6. Update every other caller in this same step so the crate compiles: - - `bottlecap/src/lifecycle/invocation/processor_service.rs:834` and `bottlecap/src/traces/trace_agent.rs:1148`: `new_error_sampler(false)` becomes `new_error_sampler(0.0)`. - - `bottlecap/tests/apm_integration_test.rs:378`: `new_error_sampler(true)` becomes `new_error_sampler(bottlecap::config::DEFAULT_APM_ERROR_TPS)`. Check the import path against the file's existing `use bottlecap::...` lines. - - `bottlecap/src/startup.rs:180-182`: pass `config.ext.error_sampler_target_tps()` (replaces the temporary step 2 argument). - - `bottlecap/src/lifecycle/invocation/processor.rs:2296,2406,3044,3625` use `enabled_error_sampler()` and need no edit. -7. Verify: `cargo build --all-targets` compiles; `cargo test --lib trace_processor` passes; `ffgrep 'new_error_sampler\((true|false)\)'` on `bottlecap/` returns zero matches. - -### Step 4: startup wiring in `bottlecap/src/startup.rs:159-183` -1. Delete the `DD_APM_ERROR_TPS` "not supported" block (:159-166). Change `use std::{env, sync::Arc};` (:15) to `use std::sync::Arc;`, because `env` has no other use in this file. -2. Insert: - ``` - if let Some(message) = config.ext.legacy_error_sampler_warning() { warn!("{message}"); } - let error_tps = config.ext.error_sampler_target_tps(); - ``` -3. Retarget the rare-sample warning (:172) to `if error_rare_sample_tracer_drop && error_tps <= 0.0`. Message: `"DD_APM_FEATURES includes error_rare_sample_tracer_drop, but error trace rescue is disabled (effective DD_APM_ERROR_TPS {error_tps}): errored traces dropped by the tracer will not be rescued"`. -4. Build the sampler with `trace_processor::new_error_sampler(error_tps)` (reusing the local instead of the step 3 inline call). -5. Keep PR 1's later startup warning for an explicit valid TPS with extension stats off; see settled semantics 9. Remove or revise it in #1358 when tracer-computed stats also enable rescue. -6. Verify: `cargo build` succeeds and `cargo test --lib startup` passes. The warning logic itself is covered by the step 2 tests on `legacy_error_sampler_warning` and `error_sampler_target_tps`. - -## Files to Modify -- `bottlecap/src/config/mod.rs`: - - `DEFAULT_APM_ERROR_TPS`, finite-f64 deserializer. - - `LambdaConfig` field type change plus `apm_error_tps`; source field; `merge_from`. - - `error_sampler_target_tps` and `legacy_error_sampler_warning`; tests. -- `bottlecap/src/traces/trace_processor.rs`: - - Stale `DD_APM_FEATURES` doc (step 1). - - `error_sampler_config` and `new_error_sampler(f64)`; docs; test helper; new and updated tests. -- `bottlecap/src/startup.rs`: real resolution, deprecation and precedence warning, retargeted rare-sample warning, drop the `env` import. -- `bottlecap/src/lifecycle/invocation/processor_service.rs`: `new_error_sampler(0.0)`. -- `bottlecap/src/traces/trace_agent.rs`: `new_error_sampler(0.0)`. -- `bottlecap/tests/apm_integration_test.rs`: `new_error_sampler(DEFAULT_APM_ERROR_TPS)`. - -## New Files -None. - -## Final Verification -Run everything from `bottlecap/` and report the pass/fail counts. -1. `cargo fmt --all -- --check` -2. `cargo test --workspace`. Use `cargo nextest run --workspace` if nextest is installed, to match CI. -3. `cargo nextest run --features test-mode --bin bottlecap-test-mode` (or `cargo test --features test-mode --bin bottlecap-test-mode`). -4. `cargo clippy --workspace --all-targets --features default -- -D warnings` -5. `cargo clippy --workspace --all-targets --features default,test-mode -- -D warnings` -6. `cargo clippy --workspace --all-targets --no-default-features --features fips -- -D warnings`. CI runs this. If the FIPS toolchain (aws-lc-fips-sys build deps) is missing locally, say it was skipped and why. -7. Run `ffgrep` for `AlwaysKeep` in `bottlecap/src`: zero matches outside comments. Run `ffgrep` for `not supported by the Lambda extension`: zero matches. -8. Grep the changed lines for `—` (em dash): zero matches. - -## Commits and PR (only when the user asks) -Both original code commits are done (see Status), with later follow-up fixes. Draft PR #1490 is open. - -- Commit 1 (step 1 only): `docs(bottlecap): DD_APM_FEATURES accepts space-separated values` -- Commit 2 (steps 2-4): `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS`. Body: - - Rescue is on by default at 10 errored traces/s per execution environment when the extension computes stats. - - `DD_APM_ERROR_TPS<=0` disables it. - - `DD_SERVERLESS_ERROR_SAMPLER_ENABLED` is deprecated and logs a warning. -- PR title: `feat(bottlecap): rate-limit error trace rescue with DD_APM_ERROR_TPS` (no ticket). -- `## Overview`: - - Follow-up to APMSVLS-469 (the bottlecap error sampler) and part 1 of 2; name #1358 as the combined tracer-path rescue-and-filtering follow-up. - - The semantics table. - - Env parity with the Go trace-agent's `DD_APM_ERROR_TPS`; YAML key is flat `apm_error_tps`. - - The behavior change: compute-stats users who never set the flag now get rescue at 10/s. - - The caveats: per-environment budget, compute-stats path only, approximate limit with no hard ceiling, inherited from Go/SCL. - - `_dd.errors_sr` can now be below 1.0 under load. - - Legacy-only YAML users keep the same on/off result, but rescue becomes rate-limited. - - Exponent values without a dot, such as `1e3`, are rejected as invalid. -- `## Testing`: describe what is covered. Do not include test counts. - -## Risks -- **The settled caveat wording conflicts with the code.** The caveat says "unknown signatures sample at 1.0 during the first 30s". That is not exactly what happens. `Sampler::default_rate` (`score_sampler.rs`, `fn default_rate`) returns `min(1, target_tps / (max_bucket_count / 5))`, counted after the current trace, so unknown signatures are throttled even in the first bucket once volume passes 5x TPS. "No hard ceiling" is still true: each decision is a trace-id hash, and keeps grow roughly logarithmically within a bucket. Use the wording from step 2.4 and point this out to the user rather than repeating the original phrasing. -- **YAML key divergence (settled).** The Go agent's YAML key is believed to be `apm_config.errors_per_second` (inferred, not verified). Bottlecap uses flat `apm_error_tps` by decision: parity target is env vars, not YAML. State it in one line in the PR. -- **Flaky-test risk.** The shared-budget test uses the real wall clock through `process_traces`, and a 5s bucket boundary can fall mid-test. Comparing against a reset control (`shared * 2 < control`) tolerates that, because rotation only throttles the shared run further. Do not assert exact counts. -- **Throttled recovery after freeze/thaw (inherited).** After a long gap, stale counters are cleared but a throttled signature's stored rate only rises 1.2x per evaluation, and it also holds `lowest_rate` (the default for unknown signatures) down. A thawed sandbox can therefore under-rescue for a while after a past burst. Same as the Go agent and SCL; document it in the `apm_error_tps` doc and the PR, do not deviate. -- **Legacy-only YAML users.** A user with `serverless_error_sampler_enabled: true` and no TPS keeps the same on/off result, but rescue is now rate-limited instead of keep-all. This is intended; state it in the PR. -- **`1e3` is rejected (parity, intentional).** Exponent notation without a dot stays a string in figment and is ignored with a deserializer warning. Mention it in the PR; do not add a custom parser. -- **Duplicate warnings.** The non-finite warning in the deserializer can fire once per source (YAML and env). That is acceptable at startup. -- **Lints.** Clippy pedantic needs `#[must_use]` on the new pub fns and backticks around identifiers in docs (`doc_markdown`). Use `.expect("...")`, not `.unwrap()`, in new tests unless the module already has `#[allow(clippy::unwrap_used)]` (the config test module does; the trace_processor test module relies on `expect`). - -## Follow-up PR (out of scope for PR 1) - -### PR 2: #1358, rescue and filter when the tracer computes stats -- Keep [DataDog/datadog-lambda-extension#1358](https://github.com/DataDog/datadog-lambda-extension/pull/1358), branch `lpimentel/filter-sampled-traces-by-stats-owner`, with its current combined scope and review history. Do not create a separate stamp-only rescue PR or trim #1358 to filtering alone. -- Prefer to merge #1490 first, then update #1358 against `main`. Rebasing onto the unmerged #1490 branch is optional, not a dependency. Do not rebase, commit, or push without user approval. -- Resolve overlapping config docs and trace-processor tests. Retain #1490's optional raw config fields, TPS resolution, `RateLimited` constructor, `default_error_sampler()` helper, and sampler regression coverage; do not restore the old boolean or `AlwaysKeep` wiring. -- Resolve stats ownership once in `process_traces` and reuse it for `_dd.compute_stats` stamping and the rescue/filtering gate. Tracer-computed stats win even when extension stats are configured. -- Run the existing rescue-and-filter pass whenever ownership is Extension or Tracer. Keep eligible errored P0 chunks on sampler `Keep`, stamp `_dd.errors_sr`, and filter unrescued sampled-out chunks. Preserve negative-priority eligibility under `error_rare_sample_tracer_drop`, manual-drop exclusions, absent-priority behavior, and unchanged priorities/decision makers. -- Preserve one shared TPS budget per execution environment across requests and stats-ownership paths. `DD_APM_ERROR_TPS <= 0` disables rescue, not filtering. -- Leave the Backend-owned path unchanged: no rescue, forward every chunk. Preserve the pre-filter stats payload; submit it to the extension's stats generator only when the extension owns stats. -- Update `apm_error_tps` and sampler docs to cover extension- or tracer-computed stats. Remove or revise #1490's unconditional "TPS has no effect unless extension stats are enabled" startup warning, since tracer ownership is request-specific. -- Extend unit coverage across all four ownership combinations, including rescue, filtering, disabled budgets, negative-priority feature behavior, manual-drop exclusions, original priorities, and the pre-filter stats payload. Verify that extension and tracer paths consume the same sampler budget. -- Retain and adapt #1358's fake-intake E2E coverage for filtering and error rescue on both non-backend paths; assert that tracer-owned requests do not generate duplicate extension stats. -- Before merging, rerun the workspace tests, test-mode binary tests, formatting, and default/test-mode/FIPS clippy checks listed above. Report counts in the session, not the PR description; update the PR description to describe the rate-limited combined behavior. From 4e71d02bc002bb8d7f2207b9d48931d30d51723b Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Wed, 4 Nov 2026 15:55:38 -0500 Subject: [PATCH 10/15] test(bottlecap): assert post-gap rescue in throttle test The post-gap check only asserted on a Keep, so it also passed when rescue returned Drop and would not notice rescue stopping after a long freeze/thaw gap. Scan 400 distinct trace IDs at one post-gap timestamp, require at least one Keep, and assert its errors_sr stays below 1.0 at the capped recovery rate. --- bottlecap/src/traces/trace_processor.rs | 31 ++++++++++++++++--------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/bottlecap/src/traces/trace_processor.rs b/bottlecap/src/traces/trace_processor.rs index a012e2d54..4a3088df7 100644 --- a/bottlecap/src/traces/trace_processor.rs +++ b/bottlecap/src/traces/trace_processor.rs @@ -1718,8 +1718,8 @@ mod tests { } /// A long freeze/thaw gap clears stale counters, but a throttled - /// signature's stored rate recovers by at most 20% per evaluation, so it - /// stays below 1.0 after a single gap. + /// signature's stored rate recovers by at most 20% per evaluation, so + /// rescue keeps working after the gap at a rate still below 1.0. #[test] fn error_sampler_stays_throttled_after_long_gap() { let sampler = new_error_sampler(1.0); @@ -1732,15 +1732,24 @@ mod tests { } } - match sample_errored_trace(&sampler, t0 + 15 + 3600, n + 1) { - SampleDecision::Drop => {} - SampleDecision::Keep { errors_sr } => { - assert!( - errors_sr < 1.0, - "a previously throttled signature must stay below 1.0, got {errors_sr}" - ); - } - } + // All post-gap calls share one bucket, so the rate stays fixed. Ids are + // deterministic, so this bounded scan is stable. + let after_gap = t0 + 15 + 3600; + let errors_sr = (n + 1..=n + 400) + .find_map(|id| match sample_errored_trace(&sampler, after_gap, id) { + SampleDecision::Keep { errors_sr } => Some(errors_sr), + SampleDecision::Drop => None, + }) + .expect("a throttled signature must still rescue some traces after a long gap"); + assert!( + errors_sr < 1.0, + "a previously throttled signature must stay below 1.0, got {errors_sr}" + ); + // Stored rate 1/40 before the gap, capped at a 20% increase. + assert!( + (errors_sr - 0.025 * 1.2).abs() < 1e-9, + "expected the capped recovery rate 0.03, got {errors_sr}" + ); } /// Runs one batch of 50 errored P0 traces with the same signature (distinct From f767c6c82c799f82c7eda72026b5d246eec39cfa Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Thu, 5 Nov 2026 13:32:40 -0500 Subject: [PATCH 11/15] Shorten error trace rescue configuration docs --- bottlecap/src/config/mod.rs | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index a8569c25b..6a497b6d8 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -82,28 +82,15 @@ pub struct LambdaConfig { /// instead. pub serverless_error_sampler_enabled: Option, - /// `DD_APM_ERROR_TPS`: error trace rescue budget in errored traces per - /// second, distributed fairly across trace signatures. `None` uses - /// [`DEFAULT_APM_ERROR_TPS`]; `<= 0` disables rescue. - /// - /// The budget is per execution environment, so N concurrent sandboxes can - /// rescue up to N times the budget. - /// - /// Rescue runs whenever stats are computed before the backend, by either - /// the tracer or the extension. - /// - /// The limit is approximate and has no hard ceiling (inherited from the Go - /// agent and the Serverless Compatibility Layer). Errored traces are counted - /// in 5s buckets over a 30s window. Rates are recomputed only when a rescue - /// candidate arrives in a new bucket, not on a timer. A cold sampler keeps - /// errors at rate 1.0 until volume in the current bucket exceeds about 5x - /// the budget. + /// `DD_APM_ERROR_TPS`: approximate error trace rescue budget per execution + /// environment, in traces per second, shared across trace signatures. + /// `None` uses [`DEFAULT_APM_ERROR_TPS`]; `<= 0` disables rescue. Rescue + /// runs whenever stats are computed before the backend, by either the + /// tracer or the extension. /// - /// A freeze/thaw gap expires stale counters but does not reset stored rates. - /// A throttled signature's rate recovers by at most 20% per recomputation, - /// so with sparse traffic recovery takes several errored invocations however - /// long the sandbox was frozen. Until then, the lowest stored rate also caps - /// signatures the sampler has not seen yet. + /// Uses 5s buckets over a 30s window, with no hard ceiling. Freeze/thaw + /// expires counters but preserves throttled rates, which recover by at most + /// 20% per recomputation. The lowest stored rate also caps new signatures. pub apm_error_tps: Option, pub span_dedup_timeout: Option, From bd26a886e7233696327d2cc5e239860c6f8f88e7 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Fri, 6 Nov 2026 22:02:16 -0500 Subject: [PATCH 12/15] Clarify when error trace rescue applies --- bottlecap/src/config/mod.rs | 51 ++++++++++++++++++++++++++++++++++--- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index 6a497b6d8..013c95463 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -415,7 +415,7 @@ impl LambdaConfig { "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated; its value {enabled} is ignored because DD_APM_ERROR_TPS (apm_error_tps) is set to {tps}" )), (None, Some(true)) => Some(format!( - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: error trace rescue is on by default at {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: when filtering sampled-out traces, error trace rescue uses a default budget of {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" )), (None, Some(false)) => Some(String::from( "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated; set DD_APM_ERROR_TPS (apm_error_tps) to 0 to disable error trace rescue instead", @@ -961,9 +961,43 @@ mod lambda_config_tests { tps_is_none: true, }, ErrorSamplerCase { - name: "legacy true", + name: "legacy env true, compute stats false", yaml: None, - env: &[(LEGACY, "true")], + env: &[ + (LEGACY, "true"), + ("DD_LAMBDA_EXTENSION_COMPUTE_STATS", "false"), + ], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: Some(&["deprecated"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "legacy env true, compute stats true", + yaml: None, + env: &[ + (LEGACY, "true"), + ("DD_LAMBDA_EXTENSION_COMPUTE_STATS", "true"), + ], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: Some(&["deprecated"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "legacy yaml true, compute stats false", + yaml: Some( + "serverless_error_sampler_enabled: true\nlambda_extension_compute_stats: false\n", + ), + env: &[], + target: DEFAULT_APM_ERROR_TPS, + warning_contains: Some(&["deprecated"]), + tps_is_none: true, + }, + ErrorSamplerCase { + name: "legacy yaml true, compute stats true", + yaml: Some( + "serverless_error_sampler_enabled: true\nlambda_extension_compute_stats: true\n", + ), + env: &[], target: DEFAULT_APM_ERROR_TPS, warning_contains: Some(&["deprecated"]), tps_is_none: true, @@ -1081,6 +1115,17 @@ mod lambda_config_tests { "case {name:?}: warning {message:?} must contain {needle:?}" ); } + if config.ext.serverless_error_sampler_enabled == Some(true) + && config.ext.apm_error_tps.is_none() + { + assert_eq!( + message, + format!( + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: when filtering sampled-out traces, error trace rescue uses a default budget of {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" + ), + "case {name:?}: warning must qualify when error trace rescue applies" + ); + } } (expected, actual) => { panic!( From 59c1b713f750b8d439061ffad6a729475caa4f4e Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Sat, 7 Nov 2026 00:23:29 -0500 Subject: [PATCH 13/15] Drop redundant compute-stats test cases lambda_extension_compute_stats does not affect the error sampler resolution or the deprecation warning, so the duplicate table rows asserted identical outcomes. --- bottlecap/src/config/mod.rs | 24 +++--------------------- 1 file changed, 3 insertions(+), 21 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index 013c95463..f96b9a47d 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -960,6 +960,9 @@ mod lambda_config_tests { warning_contains: None, tps_is_none: true, }, + // `lambda_extension_compute_stats` does not participate in this + // resolution; both sources and both values are still exercised + // across these two cases. ErrorSamplerCase { name: "legacy env true, compute stats false", yaml: None, @@ -971,27 +974,6 @@ mod lambda_config_tests { warning_contains: Some(&["deprecated"]), tps_is_none: true, }, - ErrorSamplerCase { - name: "legacy env true, compute stats true", - yaml: None, - env: &[ - (LEGACY, "true"), - ("DD_LAMBDA_EXTENSION_COMPUTE_STATS", "true"), - ], - target: DEFAULT_APM_ERROR_TPS, - warning_contains: Some(&["deprecated"]), - tps_is_none: true, - }, - ErrorSamplerCase { - name: "legacy yaml true, compute stats false", - yaml: Some( - "serverless_error_sampler_enabled: true\nlambda_extension_compute_stats: false\n", - ), - env: &[], - target: DEFAULT_APM_ERROR_TPS, - warning_contains: Some(&["deprecated"]), - tps_is_none: true, - }, ErrorSamplerCase { name: "legacy yaml true, compute stats true", yaml: Some( From 0fe5f63141d4cd360d216e4ebfaf1df6adac8863 Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Sat, 7 Nov 2026 01:25:10 -0500 Subject: [PATCH 14/15] Rescue error traces regardless of stats ownership --- bottlecap/src/config/mod.rs | 10 +- bottlecap/src/startup.rs | 8 -- bottlecap/src/traces/trace_processor.rs | 133 +++++++++++++++++------- bottlecap/tests/apm_integration_test.rs | 60 +++++++---- bottlecap/tests/fault_injection_test.rs | 99 ++++++++++++++++++ 5 files changed, 239 insertions(+), 71 deletions(-) diff --git a/bottlecap/src/config/mod.rs b/bottlecap/src/config/mod.rs index f96b9a47d..a291eaddf 100644 --- a/bottlecap/src/config/mod.rs +++ b/bottlecap/src/config/mod.rs @@ -85,8 +85,8 @@ pub struct LambdaConfig { /// `DD_APM_ERROR_TPS`: approximate error trace rescue budget per execution /// environment, in traces per second, shared across trace signatures. /// `None` uses [`DEFAULT_APM_ERROR_TPS`]; `<= 0` disables rescue. Rescue - /// runs whenever stats are computed before the backend, by either the - /// tracer or the extension. + /// runs independently of where stats are computed; rescued traces carry + /// `_dd.errors_sr` for backend retention. /// /// Uses 5s buckets over a 30s window, with no hard ceiling. Freeze/thaw /// expires counters but preserves throttled rates, which recover by at most @@ -415,7 +415,7 @@ impl LambdaConfig { "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated; its value {enabled} is ignored because DD_APM_ERROR_TPS (apm_error_tps) is set to {tps}" )), (None, Some(true)) => Some(format!( - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: when filtering sampled-out traces, error trace rescue uses a default budget of {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: error trace rescue uses a default budget of {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" )), (None, Some(false)) => Some(String::from( "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated; set DD_APM_ERROR_TPS (apm_error_tps) to 0 to disable error trace rescue instead", @@ -1103,9 +1103,9 @@ mod lambda_config_tests { assert_eq!( message, format!( - "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: when filtering sampled-out traces, error trace rescue uses a default budget of {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" + "DD_SERVERLESS_ERROR_SAMPLER_ENABLED (serverless_error_sampler_enabled) is deprecated and has no effect: error trace rescue uses a default budget of {DEFAULT_APM_ERROR_TPS} traces per second; use DD_APM_ERROR_TPS (apm_error_tps) to change the budget" ), - "case {name:?}: warning must qualify when error trace rescue applies" + "case {name:?}: warning must describe the default rescue budget" ); } } diff --git a/bottlecap/src/startup.rs b/bottlecap/src/startup.rs index 2179c8e01..425613393 100644 --- a/bottlecap/src/startup.rs +++ b/bottlecap/src/startup.rs @@ -162,14 +162,6 @@ pub fn build_trace_agent( } let error_tps = config.ext.error_sampler_target_tps(); - // Rescue currently runs only on the compute-stats path, so an explicit - // budget has no effect without it. - if config.ext.apm_error_tps.is_some() && !config.ext.lambda_extension_compute_stats { - warn!( - "DD_APM_ERROR_TPS (apm_error_tps) has no effect unless DD_LAMBDA_EXTENSION_COMPUTE_STATS (lambda_extension_compute_stats) is true" - ); - } - // One-time startup warning: the feature only widens rescue eligibility, it // does not change the rescue budget. let error_rare_sample_tracer_drop = diff --git a/bottlecap/src/traces/trace_processor.rs b/bottlecap/src/traces/trace_processor.rs index 4a3088df7..d0f6651a8 100644 --- a/bottlecap/src/traces/trace_processor.rs +++ b/bottlecap/src/traces/trace_processor.rs @@ -93,11 +93,10 @@ pub fn error_rare_sample_tracer_drop_enabled(config: &config::Config) -> bool { #[allow(clippy::module_name_repetitions)] pub struct ServerlessTraceProcessor { pub obfuscation_config: Arc, - /// Rescues errored chunks whenever stats are computed before the backend, - /// by either the tracer or the extension. Shared across invocations so the - /// `RateLimited` budget and rolling window span the whole execution - /// environment; a std Mutex rather than tokio because `process_traces` is - /// synchronous. + /// Rescues errored chunks independently of stats ownership. Shared across + /// invocations so the `RateLimited` budget and rolling window span the whole + /// execution environment; a std Mutex rather than tokio because + /// `process_traces` is synchronous. pub error_sampler: Arc>, /// Whether `DD_APM_FEATURES` enables [`ERROR_RARE_SAMPLE_TRACER_DROP`]. When /// true, chunks with a negative priority are rescued like `AutoDrop` chunks @@ -150,10 +149,14 @@ pub fn new_error_sampler(target_tps: f64) -> Arc } impl ServerlessTraceProcessor { - /// Removes sampled-out chunks so they won't be sent to Datadog, then drops - /// any payload left without chunks. `SamplerPriority::None` (-128) means no - /// explicit priority was set and the trace is kept. - fn drop_sampled_out_chunks(&self, tracer_payloads: &mut Vec) { + /// Marks rescued error chunks for every stats owner. When `drop_sampled_out` + /// is true, also removes rejected chunks and any payload left without chunks. + /// `SamplerPriority::None` (-128) means no explicit priority was set and is kept. + fn sample_and_filter_chunks( + &self, + tracer_payloads: &mut Vec, + drop_sampled_out: bool, + ) { // Read once up front so non-errored and explicitly-dropped chunks never // touch the lock. Recover through poisoning: the sampler holds only // rolling-window counters, so a partially updated bucket is far cheaper @@ -189,15 +192,18 @@ impl ServerlessTraceProcessor { if chunk.priority < 0 && (!self.error_rare_sample_tracer_drop || Self::is_manual_tracer_drop(chunk)) { - return false; + return !drop_sampled_out; } if !rescue_enabled { - return false; + return !drop_sampled_out; } - self.rescue_error_chunk(chunk, env, now_secs) + // Consult the sampler even when rejected chunks must be forwarded. + self.rescue_error_chunk(chunk, env, now_secs) || !drop_sampled_out }); } - tracer_payloads.retain(|tp| !tp.chunks.is_empty()); + if drop_sampled_out { + tracer_payloads.retain(|tp| !tp.chunks.is_empty()); + } } /// Whether the chunk is an explicit tracer manual drop: priority exactly -1 @@ -647,15 +653,12 @@ impl TraceProcessor for ServerlessTraceProcessor { } }; - // Sampled-out chunks are preserved in payloads_for_stats above, so their - // stats are still counted after they are removed here. Filter whenever - // stats are computed before the backend, by either the tracer or the - // extension: the backend only needs these chunks when it owns stats. - if stats_computed_by != StatsComputedBy::Backend - && let TracerPayloadCollection::V07(ref mut tracer_payloads) = payload - { - self.drop_sampled_out_chunks(tracer_payloads); - if tracer_payloads.is_empty() { + // Rescue is independent of stats ownership. Preserve the pre-rescue stats + // snapshot and existing forwarding behavior when the backend owns stats. + let drop_sampled_out = stats_computed_by != StatsComputedBy::Backend; + if let TracerPayloadCollection::V07(ref mut tracer_payloads) = payload { + self.sample_and_filter_chunks(tracer_payloads, drop_sampled_out); + if drop_sampled_out && tracer_payloads.is_empty() { return (None, payloads_for_stats); } } @@ -1426,8 +1429,8 @@ mod tests { /// Verifies that `process_traces` filters sampled-out chunks from the backend /// payload whenever stats are computed before the backend, by either the tracer - /// or the extension, while preserving them in the stats collection. When the - /// backend owns stats, all chunks are kept so the backend can compute stats. + /// or the extension, while preserving them in the stats collection. The + /// backend-owned stats path preserves its existing forwarding behavior. #[test] #[allow(clippy::unwrap_used)] fn test_process_traces_filters_sampled_out_chunks() { @@ -1475,17 +1478,21 @@ mod tests { } } - /// On any stats path where stats are computed before the backend (extension or - /// tracer), the error sampler rescues errored chunks that would otherwise be - /// dropped (`AutoDrop`), stamping `_dd.errors_sr`, while non-errored P0 chunks - /// and explicit user drops are still dropped. Priorities are never rewritten. + /// Error rescue stamps eligible errored P0 chunks for every stats owner. + /// Rejected chunks are forwarded only on the backend-owned stats path. + /// Priorities and the pre-rescue stats snapshot are never rewritten. #[test] #[allow(clippy::float_cmp)] fn test_error_sampler_rescues_errored_p0_chunks() { - // (owner label, lambda_extension_compute_stats, client_computed_stats) - let cases = [("extension", true, false), ("tracer", false, true)]; + // (owner, extension stats enabled, tracer computed stats, forwarded count) + let cases = [ + ("backend", false, false, 4), + ("extension", true, false, 2), + ("tracer", false, true, 2), + ("tracer-precedence", true, true, 2), + ]; - for (owner, compute_on_extension, client_computed_stats) in cases { + for (owner, compute_on_extension, client_computed_stats, forwarded_count) in cases { let config = create_compute_stats_config_with(compute_on_extension); let (tags_provider, processor) = create_test_processor(&config, default_error_sampler(), false); @@ -1493,7 +1500,7 @@ mod tests { header_tags.generic.client_computed_stats = client_computed_stats; // trace 1: kept normally (priority 1). trace 2: errored P0 (rescued). - // trace 3: non-errored P0 (dropped). trace 4: errored user drop (dropped). + // Traces 3 (non-errored P0) and 4 (errored user drop) are not rescued. let traces = vec![ vec![make_tracer_drop_span(1, 1.0, 0, None)], vec![make_tracer_drop_span(2, 0.0, 1, None)], @@ -1501,8 +1508,10 @@ mod tests { vec![make_tracer_drop_span(4, -1.0, 1, None)], ]; - let (payload_info, _stats) = + let (payload_info, stats) = processor.process_traces(config, tags_provider, header_tags, traces, 0, None); + assert_eq!(trace_spans(&stats).count(), 4); + assert!(trace_spans(&stats).all(|s| !s.metrics.contains_key("_dd.errors_sr"))); let payload_info = payload_info.expect("expected Some payload"); let backend_send_data = payload_info.builder.build(); let kept: Vec<(u64, f64)> = trace_spans(backend_send_data.get_payloads()) @@ -1510,8 +1519,8 @@ mod tests { .collect(); assert_eq!( kept.len(), - 2, - "kept normal trace + rescued errored trace (owner={owner})" + forwarded_count, + "unexpected forwarded count (owner={owner})" ); assert!( kept.contains(&(1, 1.0)), @@ -1530,18 +1539,30 @@ mod tests { Some(1.0), "_dd.errors_sr is exactly 1.0 for a cold sampler at low volume (owner={owner})" ); + assert!( + trace_spans(backend_send_data.get_payloads()) + .filter(|s| s.trace_id != 2) + .all(|s| !s.metrics.contains_key("_dd.errors_sr")) + ); + if owner == "backend" { + assert!(kept.contains(&(3, 0.0))); + assert!(kept.contains(&(4, -1.0))); + } } } /// With `error_rare_sample_tracer_drop` enabled, an errored chunk the tracer /// dropped itself (priority -1, no decision maker) is rescued like an - /// `AutoDrop` chunk, matching the Go agent. Runs for both stats owners: the - /// tracer-owned path reaches the rescue only when filtering also applies to - /// tracer-computed stats. + /// `AutoDrop` chunk, matching the Go agent, regardless of stats ownership. #[test] fn test_error_rare_sample_tracer_drop_rescues_errored_user_drop() { // (owner label, lambda_extension_compute_stats, client_computed_stats) - let cases = [("extension", true, false), ("tracer", false, true)]; + let cases = [ + ("backend", false, false), + ("extension", true, false), + ("tracer", false, true), + ("tracer-precedence", true, true), + ]; for (owner, compute_on_extension, client_computed_stats) in cases { let config = create_compute_stats_config_with(compute_on_extension); @@ -2040,6 +2061,40 @@ mod tests { } } + #[test] + fn test_backend_stats_forwards_unrescued_chunks() { + for feature_enabled in [false, true] { + for tps in [0.0, -1.0] { + let config = create_compute_stats_config_with(false); + let (tags_provider, processor) = + create_test_processor(&config, new_error_sampler(tps), feature_enabled); + let traces = vec![ + vec![make_tracer_drop_span(1, 0.0, 1, None)], + vec![make_tracer_drop_span(2, -1.0, 1, Some("-4"))], + vec![make_tracer_drop_span(3, 0.0, 0, None)], + ]; + let (payload_info, stats) = processor.process_traces( + config, + tags_provider, + create_test_header_tags(), + traces, + 0, + None, + ); + let payload = payload_info + .expect("backend path must forward rejected chunks") + .builder + .build(); + assert_eq!(trace_spans(payload.get_payloads()).count(), 3); + assert!( + trace_spans(payload.get_payloads()) + .all(|s| !s.metrics.contains_key("_dd.errors_sr")) + ); + assert_eq!(trace_spans(&stats).count(), 3); + } + } + } + /// With the error sampler disabled, an errored P0 chunk dropped from the /// backend payload must still appear in the stats collection, so its stats /// are counted. diff --git a/bottlecap/tests/apm_integration_test.rs b/bottlecap/tests/apm_integration_test.rs index c3c5c5ec5..fe55730c9 100644 --- a/bottlecap/tests/apm_integration_test.rs +++ b/bottlecap/tests/apm_integration_test.rs @@ -509,13 +509,17 @@ fn sampled_root_span(trace_id: u64, priority: f64, error: i32) -> pb::Span { } } -/// An errored P0 trace is rescued and reaches the intake, while a non-errored P0 trace is -/// dropped. Applies whenever stats are computed before the backend, by either the -/// extension or the tracer. +/// An errored P0 trace is marked as rescued for every stats owner. Non-errored +/// P0 traces are forwarded only when the backend owns stats. #[tokio::test] async fn e2e_error_sampler_rescues_only_errored_p0_traces() { // (owner label, lambda_extension_compute_stats, client_computed_stats) - let cases = [("extension", true, false), ("tracer", false, true)]; + let cases = [ + ("backend", false, false), + ("extension", true, false), + ("tracer", false, true), + ("tracer-precedence", true, true), + ]; for (owner, compute_on_extension, client_computed_stats) in cases { let rescued_trace_id = 1; @@ -533,11 +537,20 @@ async fn e2e_error_sampler_rescues_only_errored_p0_traces() { let captured_spans: Vec<&pb::Span> = captured_trace_spans(&outcome.traces).collect(); assert_eq!( captured_spans.len(), - 1, - "only the rescued trace reaches intake (owner={owner})" + if owner == "backend" { 2 } else { 1 }, + "unexpected forwarded count (owner={owner})" + ); + assert!( + captured_spans + .iter() + .filter(|s| s.trace_id != rescued_trace_id) + .all(|s| !s.metrics.contains_key("_dd.errors_sr")) ); - let rescued_span = captured_spans[0]; + let rescued_span = captured_spans + .iter() + .find(|s| s.trace_id == rescued_trace_id) + .expect("rescued trace present"); assert_eq!(rescued_span.trace_id, rescued_trace_id); assert!( rescued_span.metrics.contains_key("_dd.errors_sr"), @@ -547,8 +560,8 @@ async fn e2e_error_sampler_rescues_only_errored_p0_traces() { } /// Sampled-out chunks are filtered from the trace intake whenever stats are computed -/// before the backend, by either the extension or the tracer, while backend-owned -/// stats keep them so the backend can compute stats. +/// before the backend, by either the extension or the tracer. The backend-owned +/// stats path preserves its existing forwarding behavior. #[tokio::test] async fn e2e_sampled_out_chunks_filtered_by_stats_owner() { // (owner label, lambda_extension_compute_stats, client_computed_stats, expected_trace_ids) @@ -670,14 +683,18 @@ fn captured_trace_spans(traces: &[pb::AgentPayload]) -> impl Iterator = captured_trace_spans(&outcome.traces).collect(); assert_eq!( captured_spans.len(), - 2, - "only the rescued tracer drop and the normally kept trace reach intake \ - (owner={owner})" + if owner == "backend" { 4 } else { 2 }, + "unexpected forwarded count (owner={owner})" ); assert!( captured_spans @@ -729,7 +745,13 @@ async fn e2e_error_rare_sample_tracer_drop_rescues_tracer_drops_selectively() { // Dropped inputs must still be accounted for in stats, but only when the // extension computes them: with tracer-owned stats the extension // suppresses its own stats generation entirely. - if compute_on_extension { + assert!( + captured_spans + .iter() + .filter(|s| s.trace_id != rescued_trace_id) + .all(|s| !s.metrics.contains_key("_dd.errors_sr")) + ); + if compute_on_extension && !client_computed_stats { let grouped = captured_grouped_stats(&outcome.stats); let total_hits: u64 = grouped.iter().map(|g| g.hits).sum(); let total_errors: u64 = grouped.iter().map(|g| g.errors).sum(); @@ -744,7 +766,7 @@ async fn e2e_error_rare_sample_tracer_drop_rescues_tracer_drops_selectively() { } else { assert!( outcome.stats.is_empty(), - "extension must not emit stats when the tracer computed them (owner={owner})" + "extension must not emit stats for another stats owner (owner={owner})" ); } } diff --git a/bottlecap/tests/fault_injection_test.rs b/bottlecap/tests/fault_injection_test.rs index 20a61974a..6e180aab7 100644 --- a/bottlecap/tests/fault_injection_test.rs +++ b/bottlecap/tests/fault_injection_test.rs @@ -19,10 +19,26 @@ mod fixtures; mod harness; use harness::prelude::*; +use libdd_trace_protobuf::pb; +use prost::Message as _; const TRACES_PATH: &str = "/api/v0.2/traces"; const LOGS_PATH: &str = "/api/v2/logs"; +/// The root span with `trace_id` from an accepted trace request. +fn decoded_root_span(request: &IntakeRequest, trace_id: u64) -> pb::Span { + let body = request.decoded_body().expect("decode intake body"); + let payload = pb::AgentPayload::decode(body.as_slice()).expect("decode AgentPayload"); + payload + .tracer_payloads + .iter() + .flat_map(|t| &t.chunks) + .flat_map(|c| &c.spans) + .find(|span| span.trace_id == trace_id) + .expect("the trace must reach the intake") + .clone() +} + /// The function log [`end_invocation_and_wait`] sends for `request_id`. fn log_text(request_id: &str) -> String { format!("log for {request_id}") @@ -213,3 +229,86 @@ async fn intake_closing_mid_body_does_not_hold_next() { ); assert!(exit.is_some_and(|e| e.success()), "exit {exit:?}"); } + +/// An errored priority-0 trace is marked for backend retention even when the +/// backend owns stats, without changing its sampling priority. +#[tokio::test(flavor = "multi_thread")] +#[serial_test::serial(bottlecap_ports)] +async fn backend_owned_stats_rescues_errored_sampled_out_trace() { + let s = Scenario::start(FaultMode::Ok).await; + // Backend owns stats: the extension computes none and the tracer sends no + // `Datadog-Client-Computed-Stats` header (`post_trace` sends none). + let mut bc = s + .bottlecap() + .env("DD_LAMBDA_EXTENSION_COMPUTE_STATS", "false") + .env("DD_APM_ERROR_TPS", "10") + .spawn(); + let deadline = s + .invoke("req-1", secs(10), 1, bc.startup_deadline()) + .await + .unwrap_or_else(|| panic!("no /next poll. {}", bc.log_tail(40))); + + let mut traces = fixtures::traces::make_traces(1, 1, false); + let root = &mut traces[0][0]; + root.error = 1; + root.metrics + .insert("_sampling_priority_v1".to_string(), 0.0); + let trace_id = root.trace_id; + let status = post_trace(fixtures::traces::v04_msgpack(&traces), STARTUP_BUDGET) + .await + .unwrap(); + eprintln!("trace agent answered {status} to the errored priority-0 trace"); + + let released = end_invocation_and_wait(&s, "req-1", true, 2, deadline).await; + let accepted = s + .intake + .wait_for( + 1, + |r| { + r.path.contains(TRACES_PATH) + && r.status == 202 + && r.api_key() == Some(TEST_API_KEY) + && r.trace_ids().unwrap().contains(&trace_id) + }, + deadline, + ) + .await; + + s.shutdown_cleanly(&mut bc).await; + + assert!( + released, + "the errored sampled-out trace held /next. {}", + bc.log_tail(40) + ); + assert!( + accepted, + "the errored priority-0 trace never reached the intake with the API key (trace agent answered {status}). Intake:\n{}", + s.intake.summary() + ); + let request = s + .intake + .requests_to(TRACES_PATH) + .into_iter() + .find(|r| r.status == 202 && r.api_key() == Some(TEST_API_KEY)) + .expect("accepted trace request"); + let root = decoded_root_span(&request, trace_id); + assert!( + root.error != 0, + "the decoded root span must still be errored. Intake:\n{}", + s.intake.summary() + ); + let errors_sr = root.metrics.get("_dd.errors_sr").copied(); + assert!( + errors_sr.is_some_and(|rate| rate > 0.0), + "the backend-owned errored trace must be rescued with _dd.errors_sr > 0, got {errors_sr:?}. Intake:\n{}\n{}", + s.intake.summary(), + bc.log_tail(40) + ); + assert_eq!( + root.metrics.get("_sampling_priority_v1").copied(), + Some(0.0), + "the rescue must not change the sampling priority. Intake:\n{}", + s.intake.summary() + ); +} From e669ebe65d81a51a69fb76ee117d7099ef52f95a Mon Sep 17 00:00:00 2001 From: Lucas Pimentel Date: Sat, 7 Nov 2026 01:46:15 -0500 Subject: [PATCH 15/15] Simplify error trace rescue regression tests --- bottlecap/src/traces/trace_processor.rs | 28 ++----------------------- bottlecap/tests/apm_integration_test.rs | 8 +++---- bottlecap/tests/fault_injection_test.rs | 7 +++---- 3 files changed, 9 insertions(+), 34 deletions(-) diff --git a/bottlecap/src/traces/trace_processor.rs b/bottlecap/src/traces/trace_processor.rs index d0f6651a8..e2a877b66 100644 --- a/bottlecap/src/traces/trace_processor.rs +++ b/bottlecap/src/traces/trace_processor.rs @@ -1785,19 +1785,7 @@ mod tests { let traces: Vec> = (0..50) .map(|i| { let trace_id = ((call * 50 + i + 1) as u64).wrapping_mul(2_654_435_761); - let mut metrics = HashMap::new(); - metrics.insert("_sampling_priority_v1".to_string(), 0.0); - vec![pb::Span { - trace_id, - span_id: trace_id, - parent_id: 0, - error: 1, - metrics, - service: "svc".to_string(), - name: "op".to_string(), - resource: "res".to_string(), - ..Default::default() - }] + vec![make_tracer_drop_span(trace_id, 0.0, 1, None)] }) .collect(); @@ -2038,19 +2026,7 @@ mod tests { create_test_processor(&config, new_error_sampler(tps), false); let header_tags = create_test_header_tags(); - let mut metrics = HashMap::new(); - metrics.insert("_sampling_priority_v1".to_string(), 0.0); - let traces = vec![vec![pb::Span { - trace_id: 1, - span_id: 1, - parent_id: 0, - error: 1, - metrics, - service: "svc".to_string(), - name: "op".to_string(), - resource: "res".to_string(), - ..Default::default() - }]]; + let traces = vec![vec![make_tracer_drop_span(1, 0.0, 1, None)]]; let (payload_info, _stats) = processor.process_traces(config, tags_provider, header_tags, traces, 0, None); diff --git a/bottlecap/tests/apm_integration_test.rs b/bottlecap/tests/apm_integration_test.rs index fe55730c9..ed6f7f87d 100644 --- a/bottlecap/tests/apm_integration_test.rs +++ b/bottlecap/tests/apm_integration_test.rs @@ -551,7 +551,6 @@ async fn e2e_error_sampler_rescues_only_errored_p0_traces() { .iter() .find(|s| s.trace_id == rescued_trace_id) .expect("rescued trace present"); - assert_eq!(rescued_span.trace_id, rescued_trace_id); assert!( rescued_span.metrics.contains_key("_dd.errors_sr"), "the rescued root span must carry its error sampling rate (owner={owner})", @@ -742,15 +741,16 @@ async fn e2e_error_rare_sample_tracer_drop_rescues_tracer_drops_selectively() { "the rescued root span must carry its error sampling rate (owner={owner})", ); - // Dropped inputs must still be accounted for in stats, but only when the - // extension computes them: with tracer-owned stats the extension - // suppresses its own stats generation entirely. assert!( captured_spans .iter() .filter(|s| s.trace_id != rescued_trace_id) .all(|s| !s.metrics.contains_key("_dd.errors_sr")) ); + + // Dropped inputs must still be accounted for in stats, but only when the + // extension computes them: with tracer-owned stats the extension + // suppresses its own stats generation entirely. if compute_on_extension && !client_computed_stats { let grouped = captured_grouped_stats(&outcome.stats); let total_hits: u64 = grouped.iter().map(|g| g.hits).sum(); diff --git a/bottlecap/tests/fault_injection_test.rs b/bottlecap/tests/fault_injection_test.rs index 6e180aab7..b1f4af96c 100644 --- a/bottlecap/tests/fault_injection_test.rs +++ b/bottlecap/tests/fault_injection_test.rs @@ -31,12 +31,11 @@ fn decoded_root_span(request: &IntakeRequest, trace_id: u64) -> pb::Span { let payload = pb::AgentPayload::decode(body.as_slice()).expect("decode AgentPayload"); payload .tracer_payloads - .iter() - .flat_map(|t| &t.chunks) - .flat_map(|c| &c.spans) + .into_iter() + .flat_map(|payload| payload.chunks) + .flat_map(|chunk| chunk.spans) .find(|span| span.trace_id == trace_id) .expect("the trace must reach the intake") - .clone() } /// The function log [`end_invocation_and_wait`] sends for `request_id`.