Uh oh!
There was an error while loading. Please reload this page.
feat(observability): instrument the capture seams — events flow end to end (#117) - #465
Conversation
Warning Review limit reached
Next review available in:53 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 selected for processing (10)
📝 WalkthroughWalkthroughThe PR adds metadata-only observability events across authentication, middleware errors, document processing, learning sessions, chat, quizzes, and notes, with a pinned taxonomy and backend/E2E coverage for payload privacy, failure isolation, and event delivery. ChangesApplication observability
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ApplicationRoutes
participant RequestIDMiddleware
participant events_service
participant EventsTable
Client->>ApplicationRoutes: authentication or feature request
ApplicationRoutes->>events_service: log_event(domain.action)
RequestIDMiddleware->>events_service: log_event(error.4xx/error.5xx)
events_service->>EventsTable: enqueue event row
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | e256aa7 | Commit Preview URL Branch Preview URL | Jul 30 2026, 06:55 AM |
- chat.message_sent now also emitted inside _legacy_chat — fallback turns (agent guardrails tripped / unexpected agent failure, on BOTH chat routes) are fully persisted turns and must count; emitting there is exactly-once since both routes' fallback branches exit before the main-path emissions. request_id rides request.state (reliable inside the SSE generator where the contextvar is not). Mirrors the documents D6 treatment; red-first test forces the fallback. - document.upload moved BELOW the idempotency short-circuit on both upload routes — an X-Request-ID replay of an already-persisted upload no longer inflates the attempt count (red-first test). - auth_guard module comment rewritten: the 403 dual emission (auth.permission_denied audit row + the middleware's error.4xx) is deliberate and now documented as such — the audit row carries the denial reason and actor, which the HTTP-level row cannot know; the old comment's double-count rationale only applies to 401s. Backend 1378 passed, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
AndresL230
commented
Jul 30, 2026
Review pass complete: CLAUDE.md audit clean; three findings from the reviewer fan-out, best-scored 75 (below the 80 posting bar), all fixed in the commit above — (1) legacy-fallback chat turns now emit chat.message_sent (was silently under-counting exactly during model degradation; empirically verified by two reviewers); (2) document.upload moved below the idempotency short-circuit so X-Request-ID replays don't inflate counts; (3) the 403 dual-emission (audit + error rows) documented as deliberate with the rationale the old comment contradicted. Backend 1378 passed + ruff clean. e2e cycle next. |
AndresL230
commented
Jul 30, 2026
Pre-merge e2e gate: full lane 21/21 passed — including the new events.spec.ts journey proving rows flow from real actions through the queue worker into /usage/summary and /errors in the live stack — + oracles clean (0 findings, 1 allowlisted). Merging. |
…o end (#117) The #375 foundation (events/llm_usage tables, queue+worker write path, admin analytics API) shipped with zero producers: grep found no log_event call site outside events_service itself. This activates it. Taxonomy (12 events, domain.action, documented in events_service's docstring + a frozen EVENT_TAXONOMY constant tests pin): - error.4xx/error.5xx (middleware): path/method/status_code/duration_ms + the matched route template (bounded cardinality; raw path kept, query strings never recorded). user_id rides request.state, stamped by get_session_user_id (the shared ASGI scope is the only channel that propagates back out of BaseHTTPMiddleware's downstream task). No 2xx/3xx events by design — curated domain events cover success, and per-request rows would blow the analytics 100k scan cap. - Unhandled crashes are captured too: a non-HTTPException propagates THROUGH dispatch without ever producing a response object, so the except-path emits the error.5xx (real duration, request id) and re-raises — empirically-verified gap from the implementation pass; the middleware comment claiming the exception handler covered it was wrong. - auth.login at the two real session-mint sites (google_callback, test-login) — NOT on every decode, which would fire per-request (documented deviation from the issue's literal wording). - auth.permission_denied at all three guard 403s (not_self / not_admin / missing_role:<slug>); the 401 paths stay uninstrumented (the middleware error.4xx already counts them — no double-emitting). - document.upload / document.processed (agent paths AND the ADR-0001 legacy pipeline), quiz.started / quiz.completed (success path only, after the atomic claim), chat.message_sent (message via content= → fingerprint only), session.started (topic fingerprinted, never in payload) / session.ended, note.created. Never text/titles/bodies in payloads — metadata + SHA-256 fingerprints per the 0035 contract. Call sites are deliberately unwrapped: log_event's body already cannot raise; a test proves a raising logger doesn't break a route. Emitted shapes stay byte-compatible with test_admin_analytics_routes' consumer fixture. 28 red-first tests in test_event_capture_seams.py; new frontend/e2e/events.spec.ts proves rows FLOW in the live stack — student action + a 404 with a secret query string, then the seeded admin polls /usage/summary and /errors (secret asserted absent everywhere). Suites: backend 1376 passed + ruff clean; frontend tsc clean, 277 passed. Closes#117. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- chat.message_sent now also emitted inside _legacy_chat — fallback turns (agent guardrails tripped / unexpected agent failure, on BOTH chat routes) are fully persisted turns and must count; emitting there is exactly-once since both routes' fallback branches exit before the main-path emissions. request_id rides request.state (reliable inside the SSE generator where the contextvar is not). Mirrors the documents D6 treatment; red-first test forces the fallback. - document.upload moved BELOW the idempotency short-circuit on both upload routes — an X-Request-ID replay of an already-persisted upload no longer inflates the attempt count (red-first test). - auth_guard module comment rewritten: the 403 dual emission (auth.permission_denied audit row + the middleware's error.4xx) is deliberate and now documented as such — the audit row carries the denial reason and actor, which the HTTP-level row cannot know; the old comment's double-count rationale only applies to 401s. Backend 1378 passed, ruff clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
6f39edc to
e256aa7CompareAndresL230
commented
Jul 30, 2026
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/routes/auth.py (1)
569-585: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
auth.loginfires even when no session token was actually minted.
auth_tokenis only mintedif SESSION_SECRET:(lines 570-573), but theevents_service.log_event("auth.login", ...)call at lines 579-584 is unconditional. WhenSESSION_SECRETis falsy, this records a false "login" audit event for a request that produced noauth_tokenand thus never actually established a session — contradicting the surrounding comment's own claim that this fires only "at the two real session-mint sites."🐛 Proposed fix
auth_token = "" if SESSION_SECRET: auth_token = mint_session( user_id, ttl=_REDIRECT_TOKEN_TTL_SECONDS, secret=SESSION_SECRET ) + # `#117`: auth.login fires at the two real session-mint sites (here and+ # /test-login), NOT in auth_guard on session decode — decode runs on+ # every authenticated request, which would emit thousands of meaningless+ # "logins" per user per day and blow the analytics scan cap.+ events_service.log_event(+ "auth.login",+ category="audit",+ user_id=user_id,+ payload={"method": "google"},+ )-- # `#117`: auth.login fires at the two real session-mint sites (here and- # /test-login), NOT in auth_guard on session decode — decode runs on- # every authenticated request, which would emit thousands of meaningless- # "logins" per user per day and blow the analytics scan cap.- events_service.log_event(- "auth.login",- category="audit",- user_id=user_id,- payload={"method": "google"},- )Note the existing
test_google_callback_source_emits_auth_logintripwire only checks string presence, not placement, so it won't catch this.🤖 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 `@backend/routes/auth.py` around lines 569 - 585, Move the auth.login events_service.log_event call inside the existing if SESSION_SECRET block after mint_session succeeds, so the event is recorded only when auth_token is actually minted. Preserve the current event payload and the separate /test-login behavior.
🧹 Nitpick comments (1)
backend/services/request_context.py (1)
66-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared payload-building logic between the crash and >=400 branches.
The
crash_payload/payloadconstruction (path/method/status_code/duration_ms + route-template lookup) and the localevents_serviceimport are duplicated verbatim across theexceptbranch and the>=400branch. A future change to the payload shape only needs to be made once if factored into a small helper.♻️ Proposed refactor
+def _route_template(request: Request) -> str | None:+ route = request.scope.get("route")+ return getattr(route, "path_format", None) or getattr(route, "path", None)+++def _base_error_payload(request: Request, status_code: int, dur_ms: float) -> dict:+ payload = {+ "path": request.url.path,+ "method": request.method,+ "status_code": status_code,+ "duration_ms": round(dur_ms, 1),+ }+ template = _route_template(request)+ if template:+ payload["route"] = template+ return payloadAlso applies to: 118-154
🤖 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 `@backend/services/request_context.py` around lines 66 - 104, Extract the duplicated request event payload construction and local events_service import from the exception and >=400 response branches into a shared helper within the request dispatch flow. The helper should accept the request, status code, and duration, preserve path/method/status_code/duration_ms fields and optional route-template lookup, and be reused for both crash_payload and response payload creation.
🤖 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.
Outside diff comments:
In `@backend/routes/auth.py`:
- Around line 569-585: Move the auth.login events_service.log_event call inside
the existing if SESSION_SECRET block after mint_session succeeds, so the event
is recorded only when auth_token is actually minted. Preserve the current event
payload and the separate /test-login behavior.
---
Nitpick comments:
In `@backend/services/request_context.py`:
- Around line 66-104: Extract the duplicated request event payload construction
and local events_service import from the exception and >=400 response branches
into a shared helper within the request dispatch flow. The helper should accept
the request, status code, and duration, preserve
path/method/status_code/duration_ms fields and optional route-template lookup,
and be reused for both crash_payload and response payload creation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 08fc5131-1805-41e5-9066-b26855e05839
📒 Files selected for processing (10)
backend/routes/auth.pybackend/routes/documents.pybackend/routes/learn.pybackend/routes/notes.pybackend/routes/quiz.pybackend/services/auth_guard.pybackend/services/events_service.pybackend/services/request_context.pybackend/tests/test_event_capture_seams.pyfrontend/e2e/events.spec.ts
What
Bundle B4 — activates the observability foundation that shipped in #375 with zero producers. Twelve
domain.actionevents across four seams, taxonomy matched one-to-one against whatroutes/admin_analytics.pyactually consumes (nothing emitted that no endpoint reads; nothing an endpoint reads left unemitted). Full detail in the commit message; the notable engineering points:error.4xx/error.5xxfrom the request middleware with the exact payload contract/errorsparses, plus the matched route template. Per-user attribution ridesrequest.state(a contextvar cannot crossBaseHTTPMiddleware's task boundary). Truly unhandled crashes are captured via a catch-emit-reraise in dispatch — the implementation pass proved empirically that they never produce a response object, so the response-path seam alone (and the pre-existing comment claiming otherwise) misses them.auth.loginat the two real mint sites instead of "on session decode" (which fires per-request and would blow the analytics scan cap);user_idon error events via the state stamp (unlocks/usage/by-usererror buckets).content=→ SHA-256content_fp; encrypted columns (message text, note title/body, document text) never enter payloads; only paths, never query strings.log_eventis a cannot-raise sink by design; a test proves a raising logger can't break a route.Verification
test_event_capture_seams.py): per-seam emission + payload contracts, zero-events-on-2xx, state propagation, all three guard denials, fingerprint-never-raw, resilience, frozen taxonomy pin, and the unhandled-crash capture (red-verified against the pre-fix middleware).frontend/e2e/events.spec.tsjourney proves rows flow end to end in the live stack: a student action + a 404 carrying a secret query string, then the seeded admin polls/usage/summary(event names appear) and/errors(path/method/status_code/duration_ms present; the secret asserted absent from every stored field).Closes#117.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Reliability