Uh oh!
There was an error while loading. Please reload this page.
feat(obs): prometheus counters for UsageEvent emission (#408) - #422
Merged
Conversation
PR #402 audit (L1) flagged that the e2e harness has no observable for UsageEvent emission — no cp-api in the loop, no OTLP receiver, no admin endpoint. A regression that dropped emission would be caught only by line-by-line code review, not by tests. This PR wires two DP-side prometheus counters so emission becomes externally observable on the gateway's own `/metrics` scrape: aisix_usage_events_emitted_total{handler, status_code, inbound_protocol} aisix_usage_event_drops_total{reason} Labels: - handler: "chat", "embeddings", "messages" (today). Fixed-set enumeration extended by the #226 follow-up endpoints (#403-#407: completions / responses / rerank / audio / images). - status_code: bucketed as `2xx` / `3xx` / `4xx` / `5xx` / `other` rather than raw u16 — avoids the ~1000-value cardinality blowup. - inbound_protocol: mirrors the wire-level UsageEvent field ("openai" / "anthropic"). - reason: "sink_full" / "sink_closed" — distinguishes worker overload from a clean worker shutdown. The drops counter has existed as `M_USAGE_EVENT_DROPS_TOTAL` since #302 but was never wired into `UsageSink::try_emit`; this PR finishes that wiring so operators see both halves of the invariant `emitted = delivered + dropped`. Architecture: `UsageSink` now optionally carries a `Metrics` handle via the `with_metrics(metrics)` builder method. The server bootstrap calls it after the shared `Metrics` is constructed. The sink's `try_emit` signature gains a `handler: &'static str` label arg — all three production callers (chat / embeddings / messages) updated to pass their fixed label. Tests: - Unit: 2 new tests in `aisix-obs::usage::tests`: - `emits_counter_increments_per_call` — pins the emit counter labels (handler, bucketed status_code, inbound_protocol). - `dropped_event_records_reason_and_keeps_emit_count` — pins the drop counter reason on a full-channel scenario, and the invariant that emit bumps even on drop. - E2E: 1 new test in `prometheus-metrics-e2e.test.ts` — drives a real /v1/chat/completions through the harness, scrapes /metrics, asserts the counter increment delta and label shape. Pins that status_code MUST be bucketed (`2xx`, not raw `200`).
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughThis PR adds Prometheus telemetry to track usage event emission and drops. It introduces a new ChangesUsage Event Telemetry
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 #422 audit raised 2 HIGH + 3 MEDIUM findings on the #408 UsageEvent counter wiring. All addressed: HIGH-1 — `emitted == delivered + dropped` invariant broke when a `disabled()` sink had a `Metrics` handle attached. Emit bumped but the disabled-channel return path didn't bump drops. Fixed: the `tx=None` branch now records a drop with `reason="sink_disabled"` so the invariant holds strictly. Operators see "DP intended to emit N but no sink wired" via `drops_total{reason=sink_disabled}` instead of silent zeros. Doc comment + test pin this contract. HIGH-2 — The drop-event test asserted metric names only ("counter appears in scrape"), not values. A regression that double-bumped emit on drop would pass that test. Fixed: added a numeric counter parser; the test now asserts `emit_total == 2` and `drops_total == 1` after one delivered + one dropped call. Helper function `parse_counter_value` is reused by the new `disabled_sink` test. MEDIUM-1 — `sink_closed` reason path had no coverage. Only `sink_full` was exercised. Added test that drops the receiver before any send, asserts `drops_total{reason=sink_closed} == 1`. MEDIUM-2 — `status_bucket` boundary cases untested; a typo like `200..299` (excluding 299) wouldn't be caught. Added boundary test covering 199 / 200 / 299 / 300 / 399 / 400 / 499 / 500 / 599 / 600 / 0 — all five buckets including the dead-code `3xx` and `other` arms with no live caller today. MEDIUM-3 — `inbound_protocol` was accepted as `&str` and copied via `.to_string()` on the request hot path. Two costs: heap alloc per call, and the type system permitted user-controlled label values. Normalised to `&'static str` at the `UsageSink::try_emit` boundary: any wire value other than "openai"/"anthropic" buckets to "other". Removes the alloc AND pins prometheus cardinality at the type-system level. Test pins the boundary defence. Also tightened the doc-comment invariant in usage.rs to match the PR body (`emitted == delivered + dropped`, exact equality, not the softer `≈`).
Uh oh!
There was an error while loading. Please reload this page.
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.
Summary
Fixes#408. PR #402 audit L1.
Pre-fix, the DP emitted `UsageEvent`s to the sink + OTLP fan-out but had no DP-side prometheus counter. Result: the e2e harness (no cp-api / no OTLP receiver in the loop) couldn't observe emission, so a regression that dropped emission was invisible to tests. This PR closes that gap.
Wiring
Two counters on the gateway's own `/metrics` scrape:
```
aisix_usage_events_emitted_total{handler, status_code, inbound_protocol}
aisix_usage_event_drops_total{reason}
```
Invariant: `emitted = delivered + dropped`. Operators can compute delivery rate as `1 - drops/emitted`.
What changed structurally
`UsageSink` now optionally carries a `Metrics` handle via `with_metrics(metrics)`. The server bootstrap calls it after the shared `Metrics` is built (`main.rs:389`). `try_emit` signature gains a `handler: &'static str` arg — all 3 production callers updated to pass their fixed label.
The `M_USAGE_EVENT_DROPS_TOTAL` counter has existed since #302 but was never wired into `try_emit`; this PR finishes that wiring so both halves of the invariant are visible.
Test plan
References
Summary by CodeRabbit
New Features
Tests