Uh oh!
There was an error while loading. Please reload this page.
fix(telemetry): emit usage event on streaming /v1/responses (#808) - #613
Merged
Conversation
The verbatim streaming `/v1/responses` path forwarded SSE bytes without parsing them, returned `usage: None`, and the handler only emitted a UsageEvent when usage was `Some` — so a successful streamed request emitted no usage event at all, while a 4xx/5xx still produced a zero-token row via the error path. Clients that always stream (e.g. the OpenAI Codex CLI) were therefore invisible to the dashboard Logs and the budget ledger on every success, but visible on failure. Wrap the forwarded byte stream so the terminal `response.completed` event's `usage` block (also `response.incomplete` / `response.failed`, which carry the same counts on truncation/cancellation) is parsed in-flight, and emit the UsageEvent from the stream's Drop guard at end-of-stream (or client-disconnect) — the same end-of-stream emission the Anthropic `/v1/messages` streaming path already uses. Bytes still forward verbatim. The buffered output-guardrail path parses usage from its held buffer and emits from the handler. The SSE framing helpers are shared with the `/v1/messages` passthrough. Tests: integration test asserting a streamed 200 emits one UsageEvent with the terminal-event token counts (fails before, passes after); DP standalone E2E (`responses-streaming-usage-e2e`) driving a real `aisix` binary + mock upstream + OTLP receiver to assert the emitted token counts. Docs: Responses API usage-accounting note.
Warning Review limit reached
More reviews will be available in 3 hours, 41 minutes, and 9 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughFixes missing usage log emission for streaming ChangesStreaming Usage Emission for /v1/responses
Sequence Diagram(s)sequenceDiagram
participant Client
participant Gateway as aisix-proxy /v1/responses handler
participant Upstream as OpenAI Responses API
participant UsageGuard as ResponsesUsageGuard (Drop)
participant Telemetry as UsageEvent emitter
Client->>Gateway: POST /v1/responses (stream: true)
Gateway->>Upstream: upstream streaming request
Upstream-->>Gateway: SSE: response.created
Upstream-->>Gateway: SSE: response.output_text.delta (×N)
Upstream-->>Gateway: SSE: response.completed { usage }
Upstream-->>Gateway: SSE: [DONE]
Gateway-->>Client: verbatim SSE bytes (forwarded in-flight)
Note over Gateway,UsageGuard: Stream ends or client disconnects
UsageGuard->>Telemetry: emit UsageEvent(input_tokens, output_tokens, ...)
Telemetry-->>Gateway: usage_handled_by_stream = true (skip handler emission)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/e2e/src/cases/responses-streaming-usage-e2e.test.ts (1)
92-94: ⚡ Quick winDon’t silently swallow OTLP parse failures in the test receiver.
Line 92 ignores malformed payloads, which turns exporter regressions into opaque timeouts. Persist the last parse error (with a short payload snippet) and include it in the timeout error to keep failures actionable.
Suggested patch
interface OtlpReceiver { url: string; spanAttrs: Array<Record<string, string>>; + parseErrors: string[]; close(): Promise<void>; } @@ async function startOtlpReceiver(): Promise<OtlpReceiver> { const spanAttrs: Array<Record<string, string>> = []; + const parseErrors: string[] = []; @@ - } catch {- // ignore malformed bodies — assertions fail on missing spans+ } catch (err) {+ parseErrors.push(+ `parse error: ${(err as Error).message}; body=${raw.slice(0, 256)}`,+ ); } @@ url: `http://127.0.0.1:${port}/v1/traces`, spanAttrs, + parseErrors, @@ - throw new Error(`no usage span for request_id=${requestId}`);+ const parseHint = recv.parseErrors.at(-1);+ throw new Error(+ `no usage span for request_id=${requestId}` ++ (parseHint ? `; last otlp receiver error: ${parseHint}` : ""),+ ); }As per coding guidelines, "Handle errors gracefully with meaningful error messages."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/src/cases/responses-streaming-usage-e2e.test.ts` around lines 92 - 94, The catch block at lines 92-94 silently swallows OTLP parse failures, which masks exporter regressions and causes tests to fail with opaque timeouts. Instead of ignoring the error, capture the parse failure and store it (along with a short snippet of the malformed payload) in a variable that persists across iterations. Then, when the test times out waiting for assertions to pass due to missing spans, include the stored parse error details in the timeout error message so developers can quickly diagnose what went wrong in the exporter.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/e2e/src/cases/responses-streaming-usage-e2e.test.ts`:
- Around line 119-123: The test uses find() to retrieve the first span matching
the request ID, which does not validate the dedup contract or ensure exactly one
usage span exists. Replace the weak first-match lookup by filtering to
usage-bearing spans for the request and asserting cardinality equals 1 before
accessing token values. This validation is needed at multiple locations in the
test file (the primary location at the recv.spanAttrs.find() call and also at a
similar pattern that occurs later in the file) to properly verify that duplicate
UsageEvent spans are not emitted for the same request.
---
Nitpick comments:
In `@tests/e2e/src/cases/responses-streaming-usage-e2e.test.ts`:
- Around line 92-94: The catch block at lines 92-94 silently swallows OTLP parse
failures, which masks exporter regressions and causes tests to fail with opaque
timeouts. Instead of ignoring the error, capture the parse failure and store it
(along with a short snippet of the malformed payload) in a variable that
persists across iterations. Then, when the test times out waiting for assertions
to pass due to missing spans, include the stored parse error details in the
timeout error message so developers can quickly diagnose what went wrong in the
exporter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 89cc6b80-97ca-42c3-a516-5939075b2986
📒 Files selected for processing (4)
crates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/responses.rsdocs/integration/responses.mdtests/e2e/src/cases/responses-streaming-usage-e2e.test.ts
Uh oh!
There was an error while loading. Please reload this page.
…equest Strengthen the #808 E2E to assert cardinality (=== 1) of usage-bearing spans for the measured request_id instead of a first-match lookup, so a duplicate-emit regression of the usage_handled_by_stream dedup guard is caught at the E2E layer too (the unit test already pins single emission).
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
A successful streaming
/v1/responsesrequest emitted no usage event. The verbatim streaming path forwarded the upstream SSE bytes without parsing them, returnedusage: None, and the handler only emits aUsageEventwhen usage isSome. So a streamed 200 produced no row indpmgr_usage_events(invisible in the dashboard Logs and the budget ledger), while a 4xx/5xx on the same endpoint still produced a zero-token row via the error path.This hit clients that always stream — the OpenAI Codex CLI talks to
/v1/responsesand always streams, so every successful Codex call was unlogged while its failures were logged.Fix
Wrap the forwarded byte stream so the terminal
response.completedevent'susageblock is parsed in-flight, and emit theUsageEventfrom the stream's Drop guard at end-of-stream (or client-disconnect).response.incomplete/response.failedcarry the same counts on truncation/cancellation and are handled too. Bytes still forward verbatim — the client sees the exact upstream SSE shape.usage_handled_by_streamguards against a double-emit).input_tokens,output_tokens, plusreasoning_tokens/cached_tokenssub-counts./v1/messagespassthrough rather than duplicated.This mirrors the end-of-stream emission the Anthropic
/v1/messagesstreaming path already does.Behavior change
Streaming
/v1/responses200s now emit one usage event per request (parity with non-streaming and with chat completions). No config or wire-shape change.Tests
UsageEventwith the terminal-event token counts (reasoning + cached included). Fails before the fix (no event), passes after.responses-streaming-usage-e2e): realaisixbinary + mock upstream streaming aresponse.completed+ mock OTLP receiver, asserting the emittedgen_ai.usage.input_tokens/output_tokens/status_code. Existingresponses-endpointandper-attempt-telemetryE2E pass unchanged.Docs
Added a Responses API usage-accounting note covering streaming vs non-streaming.
Fixes api7/AISIX-Cloud#808
Summary by CodeRabbit
Bug Fixes
Documentation
/v1/responsesendpoint, clarifying how token counts are tracked for streaming requests.Tests