Repository navigation
[Traces] Add error sampling with opt-in tracer-drop rescue - #169
lucaspimentel wants to merge 35 commits into
Conversation
|
@DataDog review |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Concurrent requests can send times to the shared sampler in reverse order at a bucket limit. This can clear valid counts and permit too many rescued traces.
🤖 Datadog Autotest · Commit f316a2a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
|
@DataDog review |
There was a problem hiding this comment.
A backward system-clock change can move the sampler to an old bucket. The sampler then uses stale counts and applies the TPS limit incorrectly.
🤖 Datadog Autotest · Commit e5fcd5e · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
e5fcd5e to
bfbe164
Compare
lucaspimentel
changed the title
Codex Review SummaryThis 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. |
![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: bfbe16481b
ℹ️ 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.
Copilot review overview
🟡 Changes recommended
Moderate issues remain in sampler test equivalence, mutex contention, and integration-test error propagation.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 2
Open (3)
What changed in this PR
Adds shared error sampling to rescue eligible errored P0 trace chunks while preserving priorities and stats behavior.
Changes:
- Integrates configurable, shared error sampling.
- Stamps
_dd.errors_sron rescued root spans. - Adds configuration, dependency wiring, and unit/integration coverage.
| File | Review summary |
|---|---|
crates/datadog-trace-agent/tests/integration_test.rs |
Moderate findings (1 vote each): propagate mini-agent startup and shutdown/task errors. |
crates/datadog-trace-agent/src/trace_processor.rs |
Moderate findings: align test environments in sampler oracles (2 votes each); avoid holding a blocking mutex during the full payload walk (1 vote). Nit: document root-resolution fallback behavior (2 votes). |
crates/datadog-trace-agent/src/stats_processor.rs |
Test configuration fixtures updated; no findings. |
crates/datadog-trace-agent/src/config.rs |
Sampler configuration parsing updated; no findings. |
crates/datadog-trace-agent/Cargo.toml |
Adds the sampler dependency; no findings. |
crates/datadog-serverless-compat/src/main.rs |
Initializes the shared sampler; no findings. |
Cargo.lock |
Records dependency updates; no findings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@DataDog review |
There was a problem hiding this comment.
lucaspimentel
changed the title
|
@DataDog review |
There was a problem hiding this comment.
3788c42 to
0346af6
Compare
Wire the shared datadog-agent-trace-sampler into the SCL trace processor so error chunks with automatic-drop priority (0) get a second look before being forwarded. On a keep, the sampler's rate is stamped as a positive `_dd.errors_sr` metric on the chunk's root span, which the APM backend treats as an `error` ingestion reason and retains without any priority promotion. Chunks are never dropped locally: unrescued chunks are forwarded unchanged for the backend to discard. - Default to rate-limited rescue at 10 error TPS per process, with a shared budget across requests and processor clones - Configure via DD_APM_ERROR_SAMPLER_MODE (rate_limited | always_keep) and DD_APM_ERROR_TPS (<= 0 disables rescue in either mode); invalid values warn and fall back to defaults - Gate rescue on agent stats computation and submit all chunks to the stats concentrator before any rescue stamping
|
@DataDog review |
|
@codex review |
There was a problem hiding this comment.
More details
The diff reflects the discussed design revisions: no-priority error chunks qualify, and rescue runs independently of stats computation. Static review identified no reportable defect.
🤖 Bits Code Review · Commit 391cbfe
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4f436a7174
ℹ️ 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".
lucaspimentel
changed the title
lym953
left a comment
lym953
left a comment
There was a problem hiding this comment.
[Written by Claude, an AI assistant, for Yiming]
Thank you for the changes. I read the new head 0c9cb42. All the earlier fixes are good. The PR now adds about 344 lines of production code and about 1,374 lines of tests. I think the test changes in these comments can remove about 400 more lines.
Comments 1 to 3 are the important ones. I marked the others "minor" or "nit".
Correction to my first review: I said that the Lambda extension splits DD_APM_FEATURES only on commas. That is true only because the extension pins a serverless-components commit from before #186. When the extension updates that pin, it splits the same way as this PR.
| agent_stats_computation_enabled: env::var("DD_AGENT_STATS_COMPUTATION_ENABLED") | ||
| .map(|val| val.to_lowercase() == "true") | ||
| .unwrap_or(true), | ||
| error_sampler: parse_error_sampler_config(env::var("DD_APM_ERROR_TPS").ok().as_deref()), |
There was a problem hiding this comment.
[Written by Claude, an AI assistant, for Yiming]
The settings still differ from the Lambda extension
Thank you for removing the mode setting. Two other differences from the Lambda extension remain:
| Lambda extension | This PR | |
|---|---|---|
| Turn rescue on or off | DD_SERVERLESS_ERROR_SAMPLER_ENABLED, off by default |
DD_APM_ERROR_TPS <= 0 turns it off. It is on by default. |
DD_APM_ERROR_TPS |
Not read. The extension logs a warning when a user sets it. | The budget, default 10 |
Do you plan to align the two products, in this PR or in a follow-up? If the differences are intentional, can you put the reason in the PR description?
There was a problem hiding this comment.
Keeping the current SCL settings in this PR. SCL uses the Go-agent-style RateLimited sampler, with DD_APM_ERROR_TPS controlling its target and zero disabling rescue. No configuration alignment with the Lambda extension is included here.
|
|
||
| const DEFAULT_APM_RECEIVER_PORT: u16 = 8126; | ||
| /// Default error rescue budget: error traces per second, matching the Go agent's `ErrorTPS`. | ||
| const DEFAULT_ERROR_SAMPLER_TPS: f64 = 10.0; |
There was a problem hiding this comment.
[Written by Claude, an AI assistant, for Yiming]
Rescue is on by default for all users
When a user upgrades, rescue starts at 10 error traces per second. The user does not need to change a setting. The Lambda extension makes rescue opt-in. Its config comment gives the reason: the extension uses AlwaysKeep, so it has no limit on volume. This PR has a limit, so on by default can be the right choice. But it changes how many traces each user ingests.
The budget is per process. So the total increases with the number of function instances.
Can the PR description state this change in behavior, and say who approved the default? It can also tell users how to turn rescue off (DD_APM_ERROR_TPS=0).
There was a problem hiding this comment.
Keeping rescue enabled by default with a per-process target of 10 TPS, matching the Go agent's ErrorTPS default. DD_APM_ERROR_TPS=0 disables it. The PR description already documents both settings; 10 TPS is an adaptive target, not a hard cap.
|
|
||
| // The trace agent already constructs its own configuration. Adopting | ||
| // datadog-agent-config solely for this flag would introduce broader loader coupling. | ||
| fn error_rare_sample_tracer_drop_enabled(raw: &str) -> bool { |
There was a problem hiding this comment.
[Written by Claude, an AI assistant, for Yiming]
Minor: this parser copies the shared config crate
The shared config crate in this repo has the same split in split_comma_then_space, with its own test. This PR copies the 6-line function and adds a 28-line grammar test. I understand that you do not want this crate to use the shared config loader. A small public function in the shared crate can avoid that. It still adds a dependency, so this is your decision.
There was a problem hiding this comment.
Keeping the small local parser for now. It predates #186. Consolidating the two config types would let this code use the shared apm_features field directly, without introducing a public helper and temporary dependency wiring.
What does this PR do?
Integrate the shared error sampler from #141 into the SCL trace processor, giving dropped error chunks another sampling opportunity.
error != 0. Priority0and missing-priority sentinel-128are eligible by default, regardless of decision maker.DD_APM_FEATURES=error_rare_sample_tracer_drop. Exclude exact manual drops (priority == -1and chunk_dd.p.dm == "-4"); positive priorities never qualify._dd.p.dmfrom the first span carrying it to chunk tags before enrichment, independently of rescue settings. Existing chunk values take precedence, including empty strings; span metadata remains intact._dd.errors_sron the resolved root when the sampler keeps a chunk. This enables backend retention without changing sampling priorities or decision makers. Forward every chunk, including those not rescued.Motivation
APMSVLS-472: retain a representative sample of errors that would otherwise be dropped, with a shared adaptive sampling budget. The opt-in feature widens eligibility only; it does not enable a disabled sampler.
Additional Notes
RateLimitedmode with a default target of10error traces per second and extra sample rate1.DD_APM_ERROR_TPS <= 0disables rescue; malformed/non-finite values warn and fall back to10.DD_APM_ERROR_SAMPLER_MODEis not read.Describe how to test/QA your changes
cargo check, and workspace all-features Clippy. These broader checks were not rerun for this description update.