Conversation
|
Warning Review limit reached
Next review available in: 36 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 Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (22)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
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
GitHub Actions
Inactive
GitHub Actions
Inactive
|
@coderabbitai review Note: this comment was posted by Claude via back and forth with @njbrake. Auto review was skipped on this PR because it targets |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
Adds an Activity (per-request log) surface to the standalone dashboard and extends the backend usage-log pipeline to record and expose per-request latency plus additional list/count filtering needed for pagination.
Changes:
- Dashboard: add Activity page UI with filters, expandable error details, and pagination backed by a new
/v1/usage/countendpoint. - Backend: record
usage_logs.latency_msend-to-end (success, failure, streaming settlement, passthrough), add status/timestamp index, and extend/v1/usagewithstatus,model,endpointfilters pluslatency_msin responses. - Tests/spec: add unit + integration coverage for latency and usage filters; update OpenAPI and Postman collection; commit updated dashboard bundle entrypoint.
Reviewed changes
Copilot reviewed 20 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/pages/ActivityPage.tsx | New Activity page UI: filters, table, expandable details, pager logic |
| web/src/pages/ActivityPage.test.tsx | Vitest coverage for Activity UI behavior, filters, pager fallback |
| web/src/components/AppShell.tsx | Add “Observability” nav section and Activity entry |
| web/src/App.tsx | Register /activity route |
| web/src/api/types.ts | Add UsageEntry, UsageFilters, UsageCount types for Activity feature |
| web/src/api/hooks.ts | Add useUsageLogs + useUsageCount data hooks |
| tests/unit/test_usage_latency.py | Unit tests for _elapsed_ms helper |
| tests/unit/test_pipeline_settlement.py | Ensure RequestContext in tests includes started_at |
| tests/integration/test_usage_tracking.py | Integration coverage: latency recorded on success + failure paths |
| tests/integration/test_usage_endpoint.py | Integration coverage: new filters, bare-array contract, /count |
| src/gateway/static/dashboard/index.html | Update built dashboard asset references |
| src/gateway/models/entities.py | Add latency_ms column + status,timestamp index + expose in to_dict() |
| src/gateway/api/routes/users.py | Include latency_ms in per-user usage responses |
| src/gateway/api/routes/usage.py | Add filters + latency_ms, and implement /v1/usage/count |
| src/gateway/api/routes/batches.py | Document why batch usage rows keep latency_ms NULL |
| src/gateway/api/routes/_pipeline.py | Thread monotonic start time, compute/store latency_ms across settlement paths |
| src/gateway/api/routes/_passthrough.py | Record latency for passthrough usage logs |
| docs/public/otari.postman_collection.json | Document new filters and add “Count Usage” request |
| docs/public/openapi.json | Regenerate OpenAPI for new fields/endpoint |
| alembic/versions/e2f5a7c9b1d4_add_usage_logs_latency_ms.py | Migration for nullable latency_ms and (status,timestamp) index |
khaledosman
left a comment
khaledosman
left a comment
There was a problem hiding this comment.
Reviewed against the repo standards (backend, frontend, security, performance instructions). This is a clean, thorough, well-tested change: migration chains correctly to the current single head with a reversible downgrade; the default newest-first view is index-served (timestamp already carries index=True) and the new (status, timestamp) composite serves error triage; both /v1/usage and the new /v1/usage/count are master-key gated and share one filter helper so the count can't drift from the rows; no request/response content is stored or logged; front/back types match and OpenAPI + Postman are regenerated. The two review-follow-up regressions (batch endpoints reachable, Next surviving a failed count) both have tests.
Two minor, non-blocking notes left inline. Nice work.
Review generated by Claude Code.
GitHub Actions
Inactive
Adds the Activity page: a per-request log of what the gateway served (tokens, cost, latency, status, errors), plus the backend it reads from. Backend - Record `usage_logs.latency_ms`: a monotonic start is captured once at the top of the shared handler preamble (`resolve_request_context`) and carried on `RequestContext`, then threaded into `log_usage` via `_elapsed_ms`. Covers non-streaming success, the failure/refund path, all three streaming settlement callbacks, and the two direct `UsageLog` constructors in `_passthrough.py`. Batch rows keep a NULL latency on purpose: a batch is an asynchronous job, so it has no synchronous request duration. - For streaming, settlement runs after the stream drains, so the value is total server-side time. It is labelled "Total time" rather than a bare "latency" so it is not mistaken for time to first token. - `/v1/usage` gains additive `status`, `model`, and `endpoint` filters and returns `latency_ms`. The response stays a bare JSON array so the existing billing/analytics export contract is unchanged; the paginator's total is served by a new `GET /v1/usage/count` that shares one filter helper, so the count cannot drift from the list. - Migration adds the nullable column and an `(status, timestamp)` index for the primary "show errors, newest first" query. `model` is high cardinality and left unindexed. Dashboard - New Activity page: time range presets (24h default), status / endpoint / user / model filters, error rows surfaced in the red status treatment, an expandable detail row with the full provider error text and a copy control, a pager backed by the count endpoint, and distinct empty states for "no matches" versus "nothing recorded yet". - Activity leads a new "Observability" sidebar section. "Access" covers who may call the gateway; a request log is what it did. The section is where usage analytics and an overview dashboard will live. No request or response content is captured. `UsageLog` stores counts, timing, and provider-reported status only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…sable Review follow-ups on the activity log viewer. - The endpoint filter's curated list omitted `/v1/batches` and `/v1/batches/results`, which `log_batch_usage` writes, so batch rows could not be reached by that filter at all. Adds both, notes that the list has to grow with new billable routes, and covers it with a regression test. - The pager derived `hasNext` from the total alone, which is zero when the count request fails. A failed count therefore disabled Next and stranded the operator on the first page with rows they could not reach. Falls back to "a full page came back, so there is probably more" when the count is absent. - Documents why the vision describe side-call records a NULL latency: the row bills the describe model, so the enclosing request's duration would be misattributed to it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…available The pager derived its label from the total, which is zero when the count request fails. It therefore read "0 of 0" while a full page of rows was on screen, contradicting what the operator could see. The range is now anchored on the rows actually rendered, and the "of N" suffix is dropped when no total is available. Covered by the existing count-failure test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-up from @Copilot on #330. `count.data` is not a test for "the current count succeeded": TanStack keeps the last successful data when a refetch errors, and `keepPreviousData` serves the previous filters' total while a new one loads. Either case would show a stale total and could disable Next while more rows were reachable. The pager now gates the exact-total path on `count.isSuccess && !count.isPlaceholderData` and falls back to the visible row range otherwise. Also corrects the model filter's hint. The filter is an exact match on the model recorded in the log (the resolved target, with no provider prefix), so the placeholder now shows a realistic stored value and says so, rather than implying a family name like "gpt-4o" would match a version suffixed row. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rebasing onto main put this migration alongside e5a7c9d1b3f4 (index the budget foreign keys), which arrived with #322 and declares the same parent, d1e4b8c6f0a3. That left two sibling heads, so `alembic upgrade head` would fail with "Multiple head revisions are present" on startup and in the integration tests. Re-chains this revision after e5a7c9d1b3f4 so the history is linear again. Index-only and column-only additions that do not touch the same objects, so the order between them carries no behavioral meaning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
GitHub Actions
Inactive
Description
Adds the Activity page: a per-request log of what the gateway served (tokens, cost, latency, status, errors), plus the backend read path it uses.
Stacked on #322 (
feat/dashboard-budgets), so this targets that branch rather thanmain. It should be reviewed and merged after #322.Latency (
usage_logs.latency_ms)resolve_request_context), carried onRequestContext, and threaded intolog_usagethrough a single_elapsed_mshelper. That covers non-streaming success, the failure/refund path, all three streaming settlement callbacks, and the two directUsageLogconstructors in_passthrough.py.Read path (
/v1/usage)status,model, andendpointfilters, andlatency_mson the response.GET /v1/usage/countthat shares one filter helper with the list, so the count cannot drift from the rows. A regression test guards the bare array shape.(status, timestamp)index for the primary "show errors, newest first" query.modelis high cardinality and is left unindexed on purpose.Dashboard
Privacy: no request or response content is captured anywhere in this change.
UsageLogstores counts, timing, and provider reported status only.Review follow-ups (second commit)
/v1/batchesand/v1/batches/results, whichlog_batch_usagewrites, so batch rows could not be reached by that filter. Both added, with a regression test.hasNextfrom the total alone, which is zero when the count request fails; a failed count therefore disabled Next and stranded the operator on the first page. It now falls back to "a full page came back, so there is probably more", with a regression test.PR Type
Relevant issues
Closes #304. Part of #299.
Checklist
tests/unit,tests/integration).make lint,make typecheck; 726 unit tests; web typecheck and 97 Vitest tests; OpenAPI and Postman checks).On CI: because this PR targets
feat/dashboard-budgetsrather thanmain, the four quality workflows (otari-tests,otari-lint,otari-typecheck,otari-dashboard) do not run automatically on it. All four are gated withpull_request: branches: [ main ], so only the PR title, template, and SDK codegen checks fire here. CodeRabbit's auto review is limited tomainfor the same reason. All four workflows have been dispatched manually against branch304instead; their results are on the Actions tab and are the authoritative signal for this PR.On the integration suite: Docker and Postgres are unavailable in the sandbox this was built in, and the shared conftest teardown uses
DROP TABLE ... CASCADE, which SQLite rejects and which then corrupts state for the following test. The new integration tests were therefore validated locally one at a time against a fresh SQLite database (10 of 11 passing). The eleventh,test_list_usage_response_shape, is a pre-existing Postgres oriented test that compares an exact timestamp string; SQLite does not preserve the timezone offset, so it reports09:30:00against the expected09:30:00+00:00. Thelatency_msvalue it now asserts matches. The manually dispatched Postgres run covers that file properly.AI Usage
AI Model/Tool used: Claude Code (Opus 4.8, 1M context)
Any additional AI details you'd like to share: Planned and built through back and forth with @njbrake, who drove the decisions: reusing
/v1/usagerather than adding a dedicated logs endpoint, deferring TTFT to keep this change focused, building the full filter and drill-down set rather than a thinner first cut, confirming that no call content should ever be captured, and moving Activity out of "Access" into its own Observability section at the top of the sidebar. The plan was reviewed by independent strategy, architecture, and design agents before implementation; that review caught a response envelope that would have broken the export contract, and corrected where latency actually had to be threaded. The reasoning and product calls are his; the code and prose are the agent's.🤖 Generated with Claude Code