Repository navigation
[APMSVLS-469] Rate-limit error trace rescue with DD_APM_ERROR_TPS - #1490
lucaspimentel wants to merge 11 commits into
Conversation
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 2 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: f1c205e | Docs | View more details | Give us feedback! |
lucaspimentel
changed the title
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@DataDog review |
31b98de to
e6e7837
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31b98deb11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![datadog-prod-us1-3[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
Configuration precedence, disabling rules, and startup wiring are consistent with the documented rescue-budget behavior. Runtime sampler behavior remains unverified.
🤖 Bits Code Review · Commit 31b98de
There was a problem hiding this comment.
🟡 Changes recommended
Concurrent batches crossing a 5-second boundary can reset live sampler counts and distort rescue rates, while the freeze/thaw regression test does not verify that rescue resumes after a gap.
2 open issues
What changed in this PR
This follow-up to #1320 replaces keep-all error trace rescue with a configurable, per-execution-environment rate limit while preserving the existing compute-stats gate.
Changes:
- Adds
DD_APM_ERROR_TPSwith a default of 10 TPS, invalid-value fallback, and legacy-flag compatibility. - Switches rescue to
RateLimitedmode and updates startup warnings. - Expands configuration and sampler regression coverage, including shared budgets and freeze/thaw behavior.
| File | Description |
|---|---|
PLAN-error-sampler-tps.md |
Records implementation plans, caveats, and follow-up scope. |
bottlecap/tests/apm_integration_test.rs |
Uses the default TPS budget in integration fixtures. |
bottlecap/src/traces/trace_processor.rs |
Enables rate-limited sampling and adds regression tests. |
bottlecap/src/traces/trace_agent.rs |
Updates the disabled-sampler test fixture. |
bottlecap/src/startup.rs |
Wires the resolved budget and configuration warnings. |
bottlecap/src/lifecycle/invocation/processor.rs |
Updates tests to use the renamed sampler helper. |
bottlecap/src/lifecycle/invocation/processor_service.rs |
Uses a zero budget to disable rescue in tests. |
bottlecap/src/config/mod.rs |
Adds TPS parsing, precedence, legacy warnings, and configuration tests. |
Review effort: Balanced
lucaspimentel
changed the title
d395f60 to
ad85245
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad85245249
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| (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" |
There was a problem hiding this comment.
When DD_SERVERLESS_ERROR_SAMPLER_ENABLED=true is set while DD_LAMBDA_EXTENSION_COMPUTE_STATS remains false, this warning says error rescue is on by default at 10 TPS, but process_traces still invokes drop_sampled_out_chunks only when extension-side stats are enabled. The separate no-effect warning in startup.rs only checks apm_error_tps.is_some(), so legacy-only users receive no correction and may believe their errors are being rescued when they are not; make this warning conditional on the compute-stats gate or emit the same no-effect warning for the legacy setting.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Codex Review: Didn't find any major issues. Bravo.
Reviewed commit: ad85245249
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
🛡️ Codex Security Review · Automatically triggered
Security review completed. No security issues were found in this pull request.
Reviewed commit: ad85245249
Only the user who started this review can view the report in Codex.
ℹ️ About Codex security reviews in GitHub
This is an experimental Codex feature. Security reviews are triggered when:
- You comment "@codex security review"
- A regular code review gets triggered (for example, "@codex review" or when a PR is opened), and you’re opted in so security review runs alongside code review
Once complete, Codex will leave suggestions, or a comment if no findings are found.
Planning notes for rate-limited error trace rescue and the deprecation of DD_SERVERLESS_ERROR_SAMPLER_ENABLED. To be deleted before the PR merges.
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.
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.
ad85245 to
f1c205e
Compare
Overview
Replace keep-all error trace rescue (#1320) with the shared Go/SCL rate-limited sampler.
DD_APM_ERROR_TPS(YAML:apm_error_tps) defaults to 10 errored traces/s per execution environment, shared across invocations. Values<= 0disable rescue.DD_LAMBDA_EXTENSION_COMPUTE_STATS=true. An explicit TPS setting without it logs a startup warning.DD_SERVERLESS_ERROR_SAMPLER_ENABLEDand warn whenever it is set.falsedisables rescue only when no valid TPS is configured; valid TPS always wins.1.0e3, not1e3, for exponent notation._dd.errors_srpotentially below 1.0.Testing
DD_APM_FEATURESsupport.-D warningsfor default, default+test-mode, and FIPS.