fix(telemetry): scope the winning attempt's latency_ms to that attempt - #839
Merged
Conversation
`UsageEvent.latency_ms` is documented as scoped to ONE upstream attempt, which is what makes a request's events summable. Failed attempts honored that (`ms_since(attempt_started)`), but most winning attempts reported `started.elapsed()` — the whole request, measured from handler entry. A winner's latency therefore swallowed every failed attempt that preceded it, plus request parsing, guardrail scans and the retry backoff. Summing a request's attempts double-counted the losers: an initial attempt that burned 430ms followed by a streamed winner showed 430 + 13578 rather than 430 + (the winner's own 8s). Only `/v1/chat/completions`' non-streaming path was already correct (it used the winner's `AttemptRecord.latency_ms`). Brought the rest of the family in line: - chat.rs streaming: `StreamWin` now carries `attempt_started`, and the end-of-stream emit reports that attempt's elapsed time. - chat.rs billed-then-blocked terminal event: uses the winner's latency. - messages.rs non-streaming winner, plus both streaming dispatches (native Anthropic passthrough and cross-provider) — `attempt_started` is threaded through `dispatch_to_target`. - responses.rs non-streaming winner, plus both streaming dispatches. The SLO e2e histograms (`record_request_e2e_latency`) keep `started` — they are request-scoped by design, as is the access log's `latency_ms`. Also states the measurement boundaries on the `UsageEvent` contract: what an attempt's latency does and does not include, that a streamed attempt is measured to end-of-stream (`ttft_ms` carries first-token), and that summing attempts yields upstream time rather than the user-perceived total. Endpoints that emit a single event and no per-attempt failures (embeddings, rerank, completions, audio, images, passthrough) are unchanged — they have no second event to double-count against. E2E: two regressions in per-attempt-telemetry-e2e, one non-streaming (/v1/responses) and one streaming, each failing over from a deliberately slow 502 primary to a fast secondary. A correctly scoped winner is much faster than the loser it followed; a request-scoped one is necessarily slower. Both fail before this change (734ms / 705ms observed against a 350ms bound) and pass after.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
UsageEvent.latency_msis documented as scoped to ONE upstream attempt — that is what makes a request's per-attempt events summable:// `status_code`, `latency_ms`, and `ttft_ms` are scoped to THIS attemptFailed attempts honored it (
ms_since(attempt_started)), but most winning attempts reportedstarted.elapsed()— the whole request, measured from handler entry. A winner's latency therefore swallowed every failed attempt that preceded it, plus request parsing, guardrail scans and the retry backoff.Observed on a real failover request: an initial attempt that burned 430ms, then a streamed winner on the same target. The two events read 430ms and 13578ms, so summing them gave 14008ms — the winner's figure already contained the loser's 430ms and ~4.6s of pre-dispatch work.
What changed
Only
/v1/chat/completions' non-streaming path was already correct (it used the winner'sAttemptRecord.latency_ms). The rest of the handler family now matches:chat.rsstreaming end-of-streamstarted.elapsed()attempt_started.elapsed()(carried onStreamWin)chat.rsbilled-then-blocked terminalelapsedAttemptRecord.latency_msmessages.rsnon-streaming winnerelapsedAttemptRecord.latency_msmessages.rsnative-Anthropic + cross-provider streamingstarted.elapsed()attempt_started.elapsed()responses.rsnon-streaming winnerelapsedAttemptRecord.latency_msresponses.rsverbatim + cross-provider streamingstarted.elapsed()attempt_started.elapsed()attempt_startedis threaded throughdispatch_to_target/responses_to_target/responses_cross_provider_to_target.Untouched by design:
record_request_e2e_latency(SLO histograms) and the access log'slatency_msstay request-scoped — that is what they are for.attempt_kind="panel") are sub-calls rather than attempts of one target; left as-is.The
UsageEventcontract now also states the measurement boundaries: what an attempt's latency does and does not include, that a streamed attempt is measured to end-of-stream (ttft_mscarries the first-token figure), and that summing attempts yields upstream time rather than the user-perceived total.Behavior change worth flagging
cp-api's
MetricsByEnvcomputes request latency percentiles asSUM(latency_ms) GROUP BY request_id. Before this change that sum double-counted failed attempts, so p50/p99 for requests that retried or failed over were inflated; they will drop to the real upstream time. Requests that succeeded first try are unaffected (single event, and the winner's own latency is what it always was minus pre-dispatch work).The sum is not the user-perceived total either way — that figure lives in the access log and has no
UsageEventfield today. cp-api's comment onMetricsByEnvcalls the sum "user-perceived total"; that wording is now the inaccurate part, and whether to add a request-scoped field or restate the metric is a separate call.Tests
Two E2E regressions in
per-attempt-telemetry-e2e.test.ts, one non-streaming (/v1/responses) and one streaming, each failing over from a deliberately slow 502 primary (700ms) to a fast secondary. A correctly scoped winner is far faster than the loser it followed; a request-scoped one is necessarily slower because it contains it.Both assertions fail before this change (734ms and 705ms observed against a 350ms bound) and pass after. The OTLP receiver recovers each event's
latency_msfrom its span duration, since the sink derives span start asoccurred_at - latency_ms.Also run:
cargo test --workspace,cargo clippy --workspace --all-targets,cargo fmt --check, and the 17 usage/streaming/latency/retry E2E files (26 tests).🤖 Generated with Claude Code