feat(completions): emit UsageEvent on /v1/completions 200 (#403)#426
Conversation
Pre-#403, /v1/completions dropped the UsageEvent entirely. Customers using the legacy completions endpoint (still in production at significant volume despite deprecation) had spend invisible to cp-api's budget ledger and customer-facing /logs analytics. This PR mirrors PR #402 (embeddings) and PR #425 (responses). The handler now extracts the upstream `usage` block and emits a UsageEvent with: - `prompt_tokens` = usage.prompt_tokens - `completion_tokens` = usage.completion_tokens - `status_code`, `model_id`, `api_key_id`, `latency_ms` - `inbound_protocol` = "openai" - `handler` = "completions" (matches #408 enumeration) Emit semantics (mirrors #402 / #425, with PR #425 audit MEDIUM-1 applied preemptively): - 200 with `usage.prompt_tokens` present → emit - 200 with `usage: {}` or missing `prompt_tokens` → no emit (upstream-malformed; avoids zero-everything noise rows) - 200 with no `usage` block → no emit - 501 NotImplemented (provider lacks completions) → no emit - 4xx/5xx error path → no emit The `usage.prompt_tokens` gate is tightened per audit MEDIUM-1 on PR #425: per the OpenAI spec the field is required on every legitimate completion response, so its absence is upstream- malformed rather than a real zero-spend reply. Tests (3 new): - `emits_usage_event_on_200_with_tokens_issue_403` — pins all fields against a canonical legacy completions response - `skips_usage_event_when_upstream_usage_block_is_empty` — pins the `usage: {}` edge (audit-precedent from #425 MEDIUM-1) - `upstream_5xx_does_not_emit_usage_event` — negative pinning, ensures 4xx/5xx paths never reach the emit site (audit MEDIUM-2 from #425 applied preemptively) References: - Parent: #226 (non-chat handlers don't emit) - Sibling MVPs: #402 (embeddings), #425 (responses) - Spec: <https://platform.openai.com/docs/api-reference/completions/object>
|
Warning Review limit reached
More reviews will be available in 4 minutes and 28 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ 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: Free Run ID: 📒 Files selected for processing (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. Comment |
PR #426 audit raised 3 MEDIUM + 3 LOW findings. Addressed inline; LOW-2 filed as #429 (cross-handler tightening that touches #425 too). MEDIUM-1 — Doc comment on `CompletionDispatchSuccess.model_id` referenced a non-existent `upstream_called` field (stale copy from embeddings.rs where it does exist). Corrected to describe the actual gating channel (`usage.is_some()`). MEDIUM-2 — 501 NotImplemented path was recording `status=200` in the access log and prometheus metrics (hardcoded `200u16`), making it impossible for operators to distinguish real successes from "provider does not support completions". Same systemic bug PR #404 and PR #405 fix in their own handlers. Switched to `success.response.status().as_u16()` + `RequestOutcome::from_status` to mirror the convention. UsageEvent emission was already correctly skipped via `usage: None`, so billing wasn't affected — only observability. MEDIUM-3 — No test exercised the 501 path. Added `provider_lacking_complete_returns_501_without_emit`: registers an `AnthropicBridge` (which doesn't override `Bridge::complete()` so the trait default returns `BridgeError::Config` → 501), routes a `/v1/completions` request at an Anthropic model, and pins both the 501 response status AND the absence of any UsageEvent on the sink. LOW-1 — Added `skips_usage_event_when_upstream_omits_usage_block_entirely` test that exercises the outer `body.get("usage")?` short-circuit in `extract_completion_usage` (the existing `*_when_upstream_usage_block_is_empty` test only covered the inner `prompt_tokens` missing case). LOW-3 — Comment referenced `#404` PR as if merged; on this branch it's now true (#425 landed) so the reference is correct. LOW-2 (gate `completion_tokens` on presence) deliberately deferred to #429 for cross-handler symmetry with #425's responses.rs.
1 participant
Summary
Fixes #403. Sibling of PR #402 (embeddings) and PR #425 (responses).
Pre-fix, `/v1/completions` dropped the `UsageEvent` entirely. Despite OpenAI deprecating the legacy completions endpoint, customer traffic on it is still non-trivial in production (every `gpt-3.5-turbo-instruct` integration, every old OpenAI Python SDK path) — and all of it was invisible to cp-api's budget ledger.
This PR mirrors the #402 pattern: extract the upstream `usage` block, emit a UsageEvent on success.
Emit semantics
Per-PR audit-precedent fixes applied preemptively
The audit on the parallel PR #425 (responses) raised two MEDIUMs that apply structurally to this handler too:
Both lessons baked in from the start rather than addressed post-merge.
Test plan
References