Uh oh!
There was an error while loading. Please reload this page.
fix(telemetry): emit a UsageEvent for failed non-chat requests - #650
Merged
Conversation
chat / messages / responses emit a zero-token UsageEvent on a failed attempt (#655), so failures show up in the dashboard Logs and the budget ledger. The single-attempt non-chat handlers — /v1/completions, /v1/embeddings, /v1/rerank, /v1/audio/* and /v1/images/generations — dropped the event entirely on the error path, so a failed request was invisible: it appeared in neither Logs nor the ledger, only in metrics + the access log. Add a shared `usage_attr::emit_error_usage_event` helper and call it from each handler's error arm: one zero-token event carrying status_code, a bounded error_class (ProxyError::kind), the requested model name, api_key, and client IP/UA. model_id is left empty (the resolved id isn't threaded out of dispatch on the error path) — requested_model + status + error_class are enough to surface the row. The 501 NotImplemented path still emits nothing (no upstream call), unchanged. Two existing tests pinned the old "no event on 5xx" behavior (completions, rerank) — updated to assert the new zero-token error event. New equivalent tests added for embeddings, images and audio. Full aisix-proxy suite green (487), clippy + fmt clean. Origin: cross-API consistency audit after AISIX-Cloud#867.
📝 WalkthroughWalkthroughThe proxy now emits zero-token error UsageEvents for failed ChangesError UsageEvent emission
Sequence Diagram(s)sequenceDiagram
participant completions
participant emit_error_usage_event
participant ProxyState
participant usage_sink
participant exporters
completions->>emit_error_usage_event: upstream failure metadata
emit_error_usage_event->>usage_sink: emit zero-token UsageEvent
emit_error_usage_event->>ProxyState: load current snapshot
ProxyState-->>emit_error_usage_event: exporters
emit_error_usage_event->>exporters: fan out UsageEvent
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…age-events # Conflicts: # crates/aisix-proxy/src/audio.rs
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aisix-proxy/src/usage_attr.rs (1)
66-95: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider recording
latency_mson the error event.Every caller already computes
elapsedbefore invoking this helper (e.g.completions.rsLine 130,audio.rsLine 287), but the emitted error event leaveslatency_msat itsDefaultof 0. For a feature whose purpose is surfacing failed requests in Logs, time-to-failure is useful signal (distinguishes a fast 4xx reject from a slow upstream timeout that mapped to 502). Threading anelapsed: Durationparam through here and the six call sites is mechanical.♻️ Sketch
pub(crate) fn emit_error_usage_event( state: &ProxyState, label: &'static str, request_id: &str, requested_model: &str, api_key_id: &str, status_code: u16, error_class: &str, + elapsed: std::time::Duration, client: &ClientContext, ) { let event = UsageEvent { request_id: request_id.to_string(), occurred_at: chrono::Utc::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true), api_key_id: api_key_id.to_string(), requested_model: requested_model.to_string(), status_code, + latency_ms: elapsed.as_millis().min(u32::MAX as u128) as u32, inbound_protocol: "openai".to_string(), error_class: error_class.to_string(), client_source_ip: client.source_ip.clone(), client_user_agent: client.user_agent.clone(), ..Default::default() };🤖 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 `@crates/aisix-proxy/src/usage_attr.rs` around lines 66 - 95, The error usage event currently omits `latency_ms`, leaving failed requests recorded with the default value even though callers already compute `elapsed`. Update `emit_error_usage_event` in `usage_attr.rs` to accept the elapsed duration and populate `latency_ms` on `UsageEvent`, then thread that new argument through each caller that invokes this helper (such as the error paths in `completions.rs` and `audio.rs`) so the emitted error event carries the actual time-to-failure.
🤖 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.
Nitpick comments:
In `@crates/aisix-proxy/src/usage_attr.rs`:
- Around line 66-95: The error usage event currently omits `latency_ms`, leaving
failed requests recorded with the default value even though callers already
compute `elapsed`. Update `emit_error_usage_event` in `usage_attr.rs` to accept
the elapsed duration and populate `latency_ms` on `UsageEvent`, then thread that
new argument through each caller that invokes this helper (such as the error
paths in `completions.rs` and `audio.rs`) so the emitted error event carries the
actual time-to-failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aa240605-4d41-4fb4-bc6f-70ff49a44d05
📒 Files selected for processing (6)
crates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/completions.rscrates/aisix-proxy/src/embeddings.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/rerank.rscrates/aisix-proxy/src/usage_attr.rs
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.
Problem
chat/messages/responsesemit a zero-tokenUsageEventon a failed attempt (#655), so failures appear in the dashboard Logs and the budget ledger. The single-attempt non-chat handlers —/v1/completions,/v1/embeddings,/v1/rerank,/v1/audio/*,/v1/images/generations— dropped the event entirely on the error path. A failed request to those endpoints was invisible in Logs and the ledger (only the access log + a metrics counter recorded it).Fix
Add a shared
usage_attr::emit_error_usage_eventhelper and call it from each handler's error arm: one zero-token event carryingstatus_code, a boundederror_class(ProxyError::kind), the requested model name,api_key, and client IP/UA.model_idis left empty (the resolved id isn't threaded out of dispatch on the error path) —requested_model+status_code+error_classare enough to surface the row. The501 NotImplementedpath still emits nothing (no upstream call), unchanged.Tests
Two existing tests pinned the old "no event on 5xx" behavior (
completions,rerank) — updated to assert the new zero-token error event (status, zero tokens, non-emptyerror_class, exactly one event). New equivalent tests added forembeddings,images, andaudio/speech. Fullaisix-proxysuite green (487), clippy + fmt clean.Origin: cross-API consistency audit after AISIX-Cloud#867.
Summary by CodeRabbit