feat(usage): record a status code on usage logs so failures can be classified - #470
Conversation
GitHub Actions
Inactive
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (18)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
GitHub Actions
Inactive
|
@coderabbitai review Note: this re-trigger was posted by Claude at @njbrake's request. The earlier review attempt hit the plan's PR review limit and CodeRabbit does not retry on its own. |
|
✅ Action performedReview finished.
|
|
@coderabbitai full review Note: posted by Claude at @njbrake's request. The plain |
|
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 46 minutes. |
There was a problem hiding this comment.
Pull request overview
Adds a failure-classification dimension to usage logging by recording a nullable status_code on usage_logs for error rows (preferring upstream provider status when available), then exposes it via new filters on usage read endpoints and a grouped breakdown in the usage summary. This supports operational monitoring and error taxonomy work tied to #433 (and future dashboard/alerting follow-ups).
Changes:
- Add nullable
UsageLog.status_code(ORM + Alembic) and populate it across pipeline, passthrough, streaming settlement, and batch logging error paths. - Expose
status_codeas a filter on/v1/usage,/v1/usage/count,/v1/usage/summary, and/v1/usage/summary.csv. - Add
errors_by_status_codeto/v1/usage/summary, including a coarseerror_classderived from status codes; update OpenAPI, Postman collection, and docs; add unit + integration coverage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/test_usage_error_class.py | Unit tests for error_class_for mapping (including unknown/garbage codes). |
| tests/unit/test_streaming_generator.py | Updates streaming settlement callback contract (on_error now receives exception) and tests it. |
| tests/unit/test_provider_error_classification.py | Adds unit coverage for failure_status_code classification logic. |
| tests/integration/test_usage_status_code.py | End-to-end tests asserting status_code recording, filtering, and summary grouping behavior. |
| tests/integration/test_usage_endpoint.py | Extends usage list response shape test to include status_code. |
| tests/integration/test_require_pricing.py | Asserts missing-pricing rejections log status_code=402 (pipeline + passthrough). |
| src/gateway/streaming.py | Changes streaming_generator to pass the exception into on_error for classification. |
| src/gateway/models/entities.py | Adds UsageLog.status_code field and includes it in to_dict(). |
| src/gateway/api/routes/usage.py | Adds status_code filter, ErrorClass enum, and errors_by_status_code summary breakdown. |
| src/gateway/api/routes/batches.py | Adds status_code support to batch usage logging and stamps failures via failure_status_code. |
| src/gateway/api/routes/_pipeline.py | Implements failure_status_code and wires status_code into pipeline + streaming + tool-cap + pricing rejection paths. |
| src/gateway/api/routes/_passthrough.py | Stamps status_code for passthrough pricing rejection and provider failures. |
| docs/public/otari.postman_collection.json | Updates request docs/params for new status_code filters and summary description. |
| docs/public/openapi.json | Regenerated OpenAPI to include status_code and errors_by_status_code. |
| docs/api-reference.md | Documents new usage filters and the new summary taxonomy field. |
| alembic/versions/d7b9f1a3c5e8_add_usage_logs_status_code.py | Migration adding nullable usage_logs.status_code. |
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
Merge-order note first: #472 replaces every rejection-row construction this PR stamps (the 402 in resolve_request_context and the pass-through one) with a shared log_gateway_rejection that takes no status_code. Whichever of the two lands second has to thread the code through that writer, or all four of #472's new gates write NULL and land in errors_by_status_code as unknown — the gateway-side drops both PRs exist to surface.
The 401-behind-a-502 case is the right thing to pin, and checking MaxToolIterationsExceeded before the upstream shape in failure_status_code is a non-obvious ordering the docstring earns. Rest inline.
🤖 Reviewed with Claude Code
2b60684 to
9cdc303
Compare
GitHub Actions
Inactive
…assified UsageLog carried status (success/error) and a free-text error_message, so failures could be counted but not broken down. Any breakdown had to be built on substring matching over provider-specific error prose, which differs per provider and changes without notice. Add a nullable usage_logs.status_code and populate it on every path that already writes an error row: the pipeline's non-streaming and streaming provider failures, the tool-loop cap (422), the missing-pricing rejections (402) in both the pipeline and the pass-through gate, the pass-through provider failure, and batch creation. The recorded code prefers the status the provider returned, which is deliberately not always the status the caller saw. An upstream 401/403 is a provider rejecting the gateway's credentials, so the response stays a generic 502 and never says so; the log keeps the 401, which is what makes "how much of my error rate is my own misconfiguration" answerable. A provider that never answered carries no status, so the row records the gateway's own classification (504 timeout, 502 unreachable) rather than staying unclassifiable. Success rows stay NULL: there is no failure to classify, and a constant 200 would dilute every GROUP BY over the column. Expose it as a status_code filter on the four /v1/usage read endpoints and as errors_by_status_code on the summary, scoped to failures and carrying a coarse error_class (pricing / rate_limit / auth / provider_error / client_error) for display alongside the raw code. Mid-stream failures settle through streaming_generator's on_error, which received a rendered message. It now receives the exception itself so the callback can classify it; a string cannot carry a status. Two things the issue lists are left out. Budget and blocked-user rejections (403) and auth rejections (401) write no usage-log row at all today, so there is nothing to stamp; logging them is #317's scope, not a status-code change. The OTLP import path hardcodes success and drops token-less events, so populating it would change what gets imported rather than just classifying it. Fixes #433
…er fault The non-streaming path stamps 422 on the tool-loop cap so callers can tell a runaway loop from a real outage, but the streaming path raises the cap while the SSE body is already in flight (run_tool_loop_stream is an async generator, so the raise surfaces mid-iteration). It therefore settles through on_error and took failure_status_code's generic fallback, recording 502. The same gateway-owned cap was landing as client_error on non-streaming traffic and provider_error on streaming traffic, so an operator seeing the 502 spike would go chase a provider outage. Check the cap first, matching how _platform.py already treats it as a gateway-side limit rather than an upstream failure. Also from review: - Type UsageErrorCodeRow.error_class as a closed Literal so the set lands in the OpenAPI schema as an enum and a consumer can switch on it exhaustively, matching the module's existing Bucket literal. - Document the status_code filter and the errors_by_status_code taxonomy in docs/api-reference.md, which enumerates the usage filters. - Stop the UsageSummary comment overclaiming: the taxonomy counts sum to error_count only under the top-N cap, and the tail is omitted rather than folded because a null key would collide with the real "no code recorded" group.
`status_code` is documented as classifying a failure, but the filter applied it on its own, so the query did not carry the invariant its own description states. A bare `status_code=429` now also restricts to `status='error'`: it cannot pick up a non-error row if a future write path ever stamps a code on one, and it is served by the existing (status, timestamp) index instead of scanning the window. An explicit `status` still wins, so passing both stays a literal query rather than a silently contradictory one. That is also why the migration adds no index on the column, which the comment there now records: the filter is error-scoped and every aggregate over it is range-bounded, so a (status, status_code) index would tax every write on an append-heavy table to save a scan over one window's failures. Also corrects the `error_class_for` note: gateway-side budget and blocked-user rejections write no row today, and when #465 starts recording them they will arrive with no status code and read as `unknown`, so the change that stamps a code on those rows owns deciding what they classify as (a 403 for an over-budget user would otherwise be filed as `auth`). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ants The two literal codes recorded on a usage row in `_pipeline.py` (402 for the missing-pricing rejection, 422 for the tool-loop cap) now use the `fastapi.status` constants, matching `_passthrough.py`, which already writes `status.HTTP_402_PAYMENT_REQUIRED` for the same rejection. The two scaffolds are read side by side, so the same value should not be spelled two ways. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Two of the three writers that stamp a code were unexercised: the batch create failure (`log_batch_usage`) and the pass-through provider failure (`_passthrough.py`), both of which settle outside the chat pipeline the rest of these tests drive. Either could have stopped recording a code without failing anything. Adds an embeddings provider failure (upstream 429, caller still sees the generic 502) and a batch create failure (upstream 503, which must reach the taxonomy as `provider_error` rather than `unknown`), plus a test that a bare `status_code` filter returns only failures while an explicit `status` still wins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`error_class_for` still described the pre-#465 world, where budget and blocked-user refusals wrote no row at all. They do now, and after this branch they carry the status they returned, so the docstring records what actually reaches the column: an upstream status or one of the gateway's own rejection codes (403 for a blocked or over-budget user, a user/key mismatch, or a model outside a key's allow-list; 402 for missing pricing; 400 for a selector that no longer resolves). It also states plainly that a budget denial currently files as `auth`, and why splitting it out is a deliberate follow-up: the code alone cannot separate the gateway refusing the caller from a provider refusing the gateway, `provider` is NULL only on the gates that refuse before the selector resolves, and the class names are dashboard-visible. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#465 routes ten gateway-side rejections through one writer, so a missed `status_code` there is invisible: the row still appears in the activity log and still counts as a failure, it just classifies as `unknown`, which reads exactly like a row written before the column existed. These pin the code per gate, on both request scaffolds: over budget (403, and asserted to reach the taxonomy as `auth`), user/key mismatch (403), key allow-list (403), unresolvable selector (400, reaching the taxonomy as `client_error`), and the pass-through counterparts of all three. The gates are set up by importing the helpers from the tests that own them, so the two files cannot drift apart. The best-effort unit test supplies the new required argument and asserts it lands on the row, which is the control for the parameter being load-bearing rather than decorative. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#469 made every summary breakdown opt-out through `dimensions`, because each one is its own GROUP BY pass and the dashboard's tiles, timelines, and typeaheads read none of them. The failure taxonomy is another such pass, so the rebase put it behind the same selector (`dimensions=status_code`, the one dimension whose field is not `by_<name>`); this pins that: present by default and by name, absent for a totals-only caller, with the failure still counted in `totals` either way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#473 converged search's refusals onto the shared rejection writer, which now requires the status the refusal returns, so thread it through search's local `log_rejection` helper: 400 for a tool that does not resolve or resolves ambiguously, 403 for a key allow-list denial. The row shape is otherwise untouched, including `counts_toward_budget` pinned True by the shared writer. Search's provider-failure row gets `failure_status_code(exc)` too, the same stamp chat, the pass-through routes, and batches use, so search is not the one billed surface whose outages classify as `unknown`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
9cdc303 to
fb9df5e
Compare
GitHub Actions
Inactive
Description
UsageLogcarriedstatus(success/error) and a free-texterror_message, so failures could be counted but not broken down. Any breakdown had to be built on substring matching over provider-specific error prose, which differs per provider and changes without notice.This adds a nullable
usage_logs.status_codeand populates it on every path that already writes an error row: the pipeline's non-streaming and streaming provider failures, the tool-loop cap (422), the missing-pricing rejections (402) in both the pipeline and the pass-through gate, the pass-through provider failure, and batch creation.The recorded code prefers the status the provider returned, which is deliberately not always the status the caller saw. An upstream 401/403 is a provider rejecting the gateway's credentials, so the response stays a generic 502 and never says so; the log keeps the 401, which is what makes "how much of my error rate is my own misconfiguration" answerable. A provider that never answered carries no status, so the row records the gateway's own classification (504 timeout, 502 unreachable) rather than staying unclassifiable. Success rows stay NULL: there is no failure to classify, and a constant 200 would dilute every
GROUP BYover the column.New read surface:
status_codefilter on all four/v1/usageread endpoints (list, count, summary, summary.csv).errors_by_status_codeon/v1/usage/summary, scoped to failures and carrying a coarseerror_class(pricing/rate_limit/auth/provider_error/client_error) alongside the raw code. Counts reconcile withtotals.error_count.One shared-utility change: mid-stream failures settle through
streaming_generator'son_error, which received a rendered message. It now receives the exception itself so the callback can classify it; a string cannot carry a status. Without this, the streaming half of the traffic would log NULL.Two parts of the issue deliberately left out
usage_logs.modelis non-nullable.status="success"and drops events with no token counts, so populating a status code there would change what gets imported, not just classify it. The issue framed this as a "could", not part of the proposal.No dashboard changes, so the committed bundle is not stale. The Activity-log and alerting consumers of this data are follow-ups (#317, #410).
PR Type
Relevant issues
Fixes #433
Checklist
tests/unit,tests/integration).make lint,make typecheck,make test).uv run python scripts/generate_openapi.py).Test notes
New
tests/integration/test_usage_status_code.pycovers: the upstream status landing per code; the 401-recorded-behind-a-502 case (the one that makes the column load-bearing); timeouts recording 504; a streaming failure recording its upstream status; success recording NULL; the filter agreeing between list and count; and the summary grouping with its coarse classes.tests/unit/test_usage_error_class.pypins the class mapping including garbage codes.failure_status_codeunit tests live alongside the existingclassify_provider_errorones.Migration
d7b9f1a3c5e8applies and reverses cleanly against PostgreSQL, andalembic checkreports no drift forusage_logs.Full unit suite passes (1068). Integration: 830 passed, 2 failed, both pre-existing and environmental in my sandbox (
test_streaming_error_eventandtest_error_detail_leakage::test_provider_error_does_not_leak_detailsmake a real provider call and get a 404 where they expect the generic 502). I confirmed both fail identically on cleanorigin/mainin a separate worktree, so they are not a regression from this change; worth a sanity check that they pass in CI.AI Usage
AI Model/Tool used: Claude Code (Opus 5)
Any additional AI details you'd like to share:
Implemented by Claude Code via back-and-forth with @njbrake. The design decisions (recording the upstream status rather than the client-facing one, keeping success rows NULL, scoping out the 401/403 and OTLP paths) were reviewed by him; the prose is Claude's.
NOTE:
When responding to reviewer questions, please respond yourself rather than copy/pasting reviewer comments into an AI and pasting back its answer. We want to discuss with you, not your AI :)