fix(obs): emit the detailed request metrics on every endpoint, not just chat and messages - #888
Merged
Merged
Conversation
…st chat and messages `aisix_proxy_requests_total`, `aisix_proxy_failed_requests_total`, `aisix_llm_requests_total` and their duration histograms were emitted by the chat and messages handlers only. Every other endpoint recorded just the legacy `aisix_requests_total`, so `/v1/responses` traffic (Codex and friends) was absent from every request-count and success-rate query built on the detailed families while still appearing in the legacy one — which made the gap read as a bad query rather than missing instrumentation. Ten endpoints were affected: /v1/responses, /v1/completions, /v1/embeddings, /v1/rerank, /v1/messages/count_tokens, the three audio routes, /v1/images/generations and /v1/videos, plus the non-inference surfaces (/mcp, /a2a, /v1/realtime, passthrough, files/batches/ fine-tuning) and the pre-dispatch rejections. All of them now emit through one chokepoint, `request_metrics::record`, which writes the legacy series and the detailed families together. The LLM-vs-proxy split is a property of the route (`LLM_ENDPOINTS`), not of the call site, so a request lands in the same families however it ended: a 413 refused before dispatch now sits in the same denominator as the model-not-found the handler itself records. Tool calls, the opaque passthrough tunnel and the management routes stay out of the LLM families — counting them there would corrupt every per-request token and cost average. `/v1/realtime` stays out for the same reason: it feeds none of the `aisix_llm_*_tokens_total` families. chat and messages move onto the same helper so the family cannot drift apart again; their emitted labels are unchanged. Fixed alongside, all in the paths this touches: - `normalize_endpoint_label` was missing `/v1/videos*`, so video traffic reported `endpoint="other"`. - The passthrough error path used the caller-supplied `:provider` path segment verbatim as the `provider` label, letting `/passthrough/<random>/x` mint unbounded series (#451 on the provider axis). It now collapses to `unresolved` unless a configured model uses that provider. - `/v1/embeddings` and `/v1/images/generations` hardcoded `status = 200` on a success arm that also carries the 501 NotImplemented response, mislabelling it in the access log and booking it as `outcome="success"`. Same fix #426 made for completions/responses/rerank. - `/v1/responses` and the rest of the family now report a real `upstream_model` label instead of leaving it unresolved.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 7 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: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (20)
Comment |
…rics-endpoint-coverage
…ared emit #887 landed a new `record_request` call for non-WebSocket requests to /v1/realtime while this branch was open — exactly the drift the shared chokepoint exists to prevent. Route it through `request_metrics::record` so the refusal reaches the detailed proxy families like every other pre-dispatch rejection.
The struct now carries the `AuthenticatedKey` for the caller labels, which already owns the key id.
GitHub did not dispatch a workflow run for 513b1ea (no check-runs on the commit, and close/reopen did not re-fire it).
Conflict in chat.rs' error tail, resolved by deleting both sides: - #886 moved rate-limit rejection counting to `quota::reject`, which every endpoint funnels through and which knows the offending layer, and removed it from chat's `record_error` to avoid double-booking. Keeping this branch's `note_ratelimit_rejection` would have reintroduced exactly that double count. - `record_error`'s remaining job — the legacy `record_request` — is now done by `request_metrics::record` further down the same arm. So neither helper has anything left to do.
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
kilb
pushed a commit
to kilb/aisix
that referenced
this pull request
Aug 19, 2026
…ecur A third review pass over the areas the first two did not reach — credential minting, the telemetry sinks, inbound JWT, the metric label path — plus a class-based sweep of all 18 crates. Every finding is fixed here, and the classes that have now recurred more than once are guarded by tests rather than by remembering. Correctness - Serialise token minting per credential on the Azure AAD and Vertex minters. Both released the cache read lock and then let every caller that missed go to the identity provider, so a cold cache — and every expiry after it, roughly hourly — sent one POST per in-flight request. Both providers throttle their token endpoints, and a throttled mint fails the request rather than slowing it. Measured before the fix: 16 concurrent callers produced 16 mints. - Cache a short-lived token for half its life instead of not at all. `expires_in - 60s` saturates to zero below the refresh margin, and a zero-lifetime entry never satisfies a lookup, so every request re-minted. - Validate `ProviderKey.api_base` on the OpenAI-compatible and Anthropic bridges. `https://api.openai.com@evil.example/v1` reads as one host and resolves to another, which then receives the key. Vertex and Azure grew this check in api7#390; their siblings did not. The check now lives in `aisix-gateway` and all four call it. - Validate an MCP server's `url` and `token_url` on the write path. The A2A sibling constrains its `url` as a URI; MCP accepted any string, for a row whose `auth` credential is sent to exactly that address. - Canonicalize IPv4-mapped addresses in a passthrough route's `source_cidrs`, and reject a malformed entry on write. Same shape as the `Model::allowed_cidrs` fix, on the gate that is the whole boundary for an anonymous route: on a `[::]` listener it rejected every IPv4 caller. - Saturate upstream-reported token counts instead of wrapping them (12 sites). `as u32` understates — 4_294_967_297 became 1 — and the upstream is not always a trusted party, since `api_base` points wherever the operator says. - Compare admin keys through a digest, without an early exit. The proxy's own API-key path already looks callers up under `hash_bearer`; the admin path compared plaintext with `==`, which stops at the first differing byte. Observability - Emit `aisix_otlp_fanout_drops_total` / `_failures_total`, and `aisix_redis_failures_total`. All three existed with no caller, so the series could never appear in a scrape — telemetry and rate-limit degradation were visible only through the heartbeat, which a deployment with no control plane does not have. The rate-limit store reaches metrics through an injected sink (`RateLimitMetricsSink`, mirroring `GuardrailMetricsSink`) rather than a new crate edge. - Warn once per policy that a sub-minute or hourly `max_tokens` is inert, not once per request. The gap is real and tracked (api7#396); the telling was a warn line per request forever. - Publish the rate-limit window from `/v1/audio/*`, `/v1/videos` and `/v1/messages/count_tokens`, which dispatch a model and reserve quota but were missed when the other seven endpoints were wired. Performance - Keep a wildcard index on `ResourceTable`, maintained beside `by_name` so it cannot drift. Resolving a name no exact row serves used to walk the whole model table — and materialise a `Vec` of it — on the model-resolution path and on the metric-label path every endpoint reaches, including for names that resolve to nothing. - Reclaim health, runtime-status and routing-cursor state for config rows the snapshot no longer carries, through the publication hook that already drives exporter reconciliation. A target still holding a concurrency permit is kept. - State the realtime WebSocket frame bounds instead of inheriting them from a transitive dependency's defaults. Same values, now a decision. Guards for the recurring classes Each of these fails the build on the next occurrence, which is what the prose rules did not do: - `every_emit_has_a_caller` — a `Metrics` emit with no caller. Third occurrence (api7#888, api7#972, this pass); it found the Redis one immediately. - `endpoint_family_parity` — a model-dispatch handler missing a shared per-request mechanism. - `api_base_validation_parity` — a bridge resolving an operator-supplied base without validating its shape. - `readiness-gate-lint` — an e2e gate that waits only on an admin-key probe, or a spec that seeds a resource after the caller key. Both halves of the rule in `tests/e2e/AGENTS.md`; five specs carried the first and eleven the second, each failing as a load-dependent 401 or a missing limit that reads like an infrastructure hiccup. All four carry a staleness check, so a list entry that no longer applies fails too. Verified: 3322 Rust tests, 212/212 e2e files (647 tests) against real etcd and Redis, clippy clean workspace-wide, tsc clean, schemas unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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 free
to 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.
Fixes api7/AISIX-Cloud#1234
Problem
aisix_llm_requests_total/aisix_proxy_requests_total/aisix_proxy_failed_requests_totaland their duration histograms were emitted by the chat and messages handlers only. Every other endpoint recorded just the legacyaisix_requests_total.So
/v1/responsestraffic (Codex and friends) never appeared in any request-count or success-rate query built on the detailed families — while still showing up in the legacy series, which made the gap read as a bad query rather than missing instrumentation.The reported endpoint was
/v1/responses, but it is the whole handler family:/v1/completions,/v1/embeddings,/v1/rerank,/v1/messages/count_tokens, the three/v1/audio/*routes,/v1/images/generationsand/v1/videos*had the identical gap, as did the non-inference surfaces (/mcp,/a2a,/v1/realtime, passthrough, files/batches/fine-tuning) and the pre-dispatch rejections.Metrics::record_proxy_request— the proxy-only variant, no LLM series — had no call sites at all, which is the tell that the two-tier design was intended from the start and simply never wired past chat and messages.Implementation
Every handler now emits through one chokepoint,
request_metrics::record, which writes the legacy series and the detailed families from a single call. CallingMetrics::record_requestdirectly is what produced a request present in one family and absent from the others, so nothing does that any more (outside the admin API, which is not proxy traffic).The LLM-vs-proxy split is a property of the route (
LLM_ENDPOINTS), not of the call site. That matters for correctness, not just tidiness: a 413 refused before dispatch now lands in the same denominator as the model-not-found the handler itself records, instead of the two disagreeing about whether the endpoint had a failure.Non-inference surfaces get the proxy families only — counting an MCP tool call, a batch-file upload or a 413 as an LLM request would corrupt every per-request token and cost average.
/v1/realtimestays out for the same reason: it does reach a model but feeds none of theaisix_llm_*_tokens_totalfamilies, so counting it would inflate the denominator without contributing tokens.chat and messages move onto the same helper so the family cannot drift apart again. Their emitted labels are unchanged.
Fixed alongside
All in the code paths this change touches:
normalize_endpoint_labelwas missing/v1/videos*, so all video traffic reportedendpoint="other"on the in-flight gauge.:providerpath segment verbatim as theproviderlabel, so/passthrough/<random>/xminted one series per random value — security: prevent unauthenticated metric-label cardinality DoS #451's unbounded-cardinality hole on the provider axis. It now collapses tounresolvedunless a configured model uses that provider. (TheAisixPathrejection path passes the rawparts.uri.path(), so normalizing the endpoint label inrejectis load-bearing too, not just defensive.)/v1/embeddingsand/v1/images/generationshardcodedstatus = 200on a success arm that also carries the 501 NotImplemented response — mislabelling it in the access log and booking it asoutcome="success". Same fix feat(completions): emit UsageEvent on /v1/completions 200 (#403) #426 already made for completions/responses/rerank./v1/responsesand the rest of the family now report a realupstream_modellabel instead of leaving it unresolved, matching chat and messages.Behavior change
New series appear for the previously-missing endpoints. Existing chat/messages series are byte-identical. Dashboards that group
aisix_proxy_requests_totalbyendpointwill now see the tool/management/tunnel surfaces too.Baseline
LiteLLM increments
litellm_proxy_total_requests_metric/litellm_proxy_failed_requests_metricfrom a single central callback (async_log_success_event+async_post_call_failure_hook) carrying aroutelabel, so all routes are covered uniformly by construction. This change converges on that shape; no divergence.Tests
tests/e2e/src/cases/request-metrics-endpoint-coverage-e2e.test.ts— verified failing before the fix and passing after:/v1/responsesreachesaisix_llm_requests_total(empty before), with the detailed labels actually populated (upstream_model="gpt-4o-mini", notunknown)./v1/responseslands in the same denominator, withmodel="unresolved"rather than the caller's text.Rust unit tests in
request_metricspin the classification: no route falls through to"other", noLLM_ENDPOINTSentry is unreachable (a typo there fails silently — the endpoint just stops appearing), and the tier split holds for both sides.Full DP E2E suite passes (483 tests).