fix(stats): retry only retriable stats failures, with jittered backoff - #1387
lucaspimentel wants to merge 2 commits into
Conversation
Scope: this does NOT fix the ambiguous-accept stats double-count reproduced by the statsv2 double-count rig. In that experiment the intake processed a payload, the response was lost, and the flusher re-sent the same bytes; the intake counted both, because it does not deduplicate retried POST /api/v0.2/stats requests. Transport errors and per-attempt timeouts are exactly the failures where the intake may already have the payload, so they must stay retriable (matching the Go agent's isRetriableStatus). The real fix is intake-side deduplication of retried stats requests, or an idempotency key the intake can dedupe on. Both are out of scope here. What this change does: - Removes one real double-count path: any 2xx other than 202 (for example a 200 or 204 returned by a proxy) was treated as failure and retried even though the intake accepted it. Any 2xx now counts as delivered. - Stops wasted retries on permanent 4xx responses (400, 401, 403, 404, 413, ...): the retry loop stops immediately instead of re-sending the same bytes FLUSH_RETRY_COUNT times. - Spreads out retries under 429/5xx storms with full-jitter backoff over an exponential base (base 50 ms, upper bound doubles per retry; worst case 150 ms per send round). Retry-After on 429 is ignored for pacing (far smaller than any realistic value) but logged at debug. What changed internally: send_stats_payload now returns a typed SendOutcome (Success / Retriable / Permanent) instead of anyhow errors, with status classification in a pure classify_status function mirroring dd-trace-go. The retry loop moved into a free send_with_retry function. The 512-byte response-body preview is kept in failure details. What did not change: - Redrive semantics: after any failed round, including a permanent failure, send still returns Some(stats) for the one extra flush round in flushing/service.rs. Open question (noted in code): should permanent statuses skip redrive? - trace_flusher.rs and the shared FLUSH_RETRY_COUNT (= 3). - Empty-input, missing-API-key, and serialization-error handling.
❌ 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: 223d1ea | Docs | View more details | Give us feedback! |
![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: bfa507c69f
ℹ️ 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".
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. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved retry redrive, response-data logging, and Retry-After handling issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (2)
What changed in this PR
This PR hardens stats flushing with HTTP outcome classification and jittered retry backoff.
Changes:
- Accepts all 2xx responses.
- Retries only transient failures.
- Adds jittered exponential backoff and focused tests.
| File | Summary | Findings |
|---|---|---|
bottlecap/src/traces/stats_flusher.rs |
Implements status classification, retry backoff, and tests. | Permanent failures are still redriven (moderate, 3 votes); response details may leak into logs (critical, 4 votes); Retry-After is ignored (moderate, 1 vote). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let retry = u32::try_from(attempt).unwrap_or(u32::MAX); | ||
| let delay = backoff.delay(&mut rand::thread_rng(), retry); | ||
| debug!("STATS | Retrying stats flush in {} ms", delay.as_millis()); | ||
| tokio::time::sleep(delay).await; |
There was a problem hiding this comment.
StatsFlusher::flush holds the aggregator mutex for the entire retry loop in send() (stats_flusher.rs:150-157), and this PR adds up to ~150ms of tokio::time::sleep per retriable failure inside that same call path (stats_flusher.rs:288-292) — pre-PR, retries were back-to-back with no sleep, so this is new lock-hold time. This causes lock contention with the background task that drains tracer-submitted stats into the aggregator (trace_agent.rs:230-233), which can add latency to the tracer's stats requests during an intake outage. Suggest scoping the guard to just the get_batch call (drop it before send() runs) in stats_flusher.rs:150-157 so retry sleeps happen outside the critical section:
loop {
let stats = {
let mut guard = self.aggregator.lock().await;
guard.get_batch(force_flush).await // lock held only for this line
}; // guard dropped here, lock released
if stats.is_empty() {
break;
}
if let Some(failed) = self.send(stats).await { // now runs with no lock held
all_failed.extend(failed);
}
}
Overview
Hardens the stats flusher's retry behavior in
bottlecap/src/traces/stats_flusher.rs:202was accepted, so a200or204from a proxy was treated as failure and the same bytes were re-sent even though the intake had already processed them. This removes one real stats double-count path.FLUSH_RETRY_COUNTtimes.Retry-Afteris ignored for pacing (far smaller than any realistic value) but logged at debug.Internally,
send_stats_payloadnow returns a typedSendOutcome(Success / Retriable / Permanent) instead ofanyhowerrors, with status classification in a pureclassify_statusfunction mirroring the Go agent'sisRetriableStatus. The retry loop moved into a freesend_with_retryfunction. The 512-byte response-body preview is preserved in failure details.Scope: this does NOT fix the ambiguous-accept stats double-count
A local experiment (the statsv2 double-count rig) reproduced a stats double-count end to end: the intake processed a payload, the response was lost, and the flusher re-sent the same bytes; the intake counted both, because it does not deduplicate retried
POST /api/v0.2/statsrequests.This PR does not fix that. Transport errors and per-attempt timeouts are exactly the failures where the intake may already have the payload, so they must stay retriable (matching the Go agent). The real fix is intake-side deduplication of retried stats requests, or an idempotency key the intake can dedupe on. Both are out of scope here. The rig's ambiguous-accept arm still double-counts with this change applied (verified: 3/3 rig tests pass, cherry-picked onto the experiment branch).
Testing
stats_flusher.rs:classify_status: 2xx → success; 408/425/429/500/502/503/599 → retriable; 400/401/403/404/413 → permanent.[0, upper].cargo fmt --all -- --check,cargo clippy --workspace --all-targets(default,fips,default,test-mode),cargo nextest run --workspace: 701/701 passed.statsv2-double-count-rigbranch; all 3double_count_integration_testarms pass (baseline: 1 payload, safe rejection: 1 payload, ambiguous accept: still 2, as expected).Why not libdatadog's
send_with_retry?The status classification mirrors the Go tracer's
isRetriableStatus(dd-trace-go/internal/llmobs/transport/transport.go:462). It deliberately diverges from libdatadog'ssend_with_retry(the agent-side Rust mechanism), which would be a behavior regression for this endpoint:send_with_retrysend_with_retry/mod.rs:170)send_with_retry/mod.rs:178)retry_strategy.rs:74)FLUSH_RETRY_COUNT), 50 ms baseThe permanent-4xx split is the notable divergence: libdatadog would burn all its retries on a 401; this flusher stops after one. The Go trace-agent itself does not do in-flush status classification; it retries failed payloads through a bounded retry buffer, which is conceptually closer to this flusher's existing redrive path (unchanged by this PR).