fix(telemetry): per-PK attribution tags on all non-chat usage events - #647
Conversation
…sage events Provider attribution (provider_kind / provider_featured / branded_provider / pk_label / byo_label) was wired for /v1/chat/completions and /v1/messages, then /v1/responses (AISIX-Cloud#867). The remaining billing endpoints — /v1/completions, /v1/embeddings, /v1/rerank, /v1/audio/* and /v1/images/generations — left these five UsageEvent fields at default, so their dashboard Logs rows showed no upstream vendor / PK label. Same oversight as #867, replicated across the rest of the handler family. Extract the snapshot lookup + wire-field mapping into one shared helper (usage_attr::apply_pk_telemetry) so the handler family can't drift again, and resolve the target ProviderKey's telemetry_tags on each handler's emit path. Each handler already resolves the PK for dispatch; this threads its id to the emitter and points the existing /v1/responses lookup at the shared helper too. Integration tests per handler assert the emitted UsageEvent carries the tags; each fails before the fix (empty tags) and passes after.
📝 WalkthroughWalkthroughThe proxy now resolves provider-key telemetry tags through a shared helper and threads ChangesProvider-key telemetry attribution
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/aisix-proxy/src/responses.rs (1)
1631-1664: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider using
apply_pk_telemetryto avoid re-duplicating the 5-field mapping.This path now imports
provider_telemetry_tagsfor the lookup but still hand-rolls the identicalprovider_kind/provider_featured/branded_provider/pk_label/byo_labelmapping (Lines 1655-1659). That is precisely the per-field drift theusage_attrmodule set out to eliminate (usage_attr.rslines 6-10: "Centralising the snapshot lookup AND the wire-field mapping here keeps the handler family ... from drifting apart again"). Building the event first and then stamping it keeps/v1/responseson the same single source of truth as the non-chat handlers.♻️ Proposed refactor
let snap = state.snapshot.load(); - let tags = provider_telemetry_tags(&snap, provider_key_id); - let event = UsageEvent { + let mut event = UsageEvent { request_id: request_id.to_string(), occurred_at: chrono::Utc::now().to_rfc3339_opts(chrono::SecondsFormat::Secs, true), model_id: model_id.to_string(), api_key_id: api_key_id.to_string(), requested_model: requested_model.to_string(), prompt_tokens: usage.prompt_tokens, completion_tokens: usage.completion_tokens, cached_prompt_tokens: usage.cached_prompt_tokens, reasoning_tokens: usage.reasoning_tokens, cache_creation_tokens: usage.cache_creation_tokens, cache_read_tokens: usage.cache_read_tokens, latency_ms: elapsed.as_millis().min(u32::MAX as u128) as u32, status_code, inbound_protocol: "openai".to_string(), attempt_index: attempt.index, attempt_kind: attempt.kind, attempt_model: attempt.model, error_class: attempt.error_class, error_message: attempt.error_message, - provider_kind: sanitize_tag(tags.kind.map(|k| k.as_str().to_owned()).unwrap_or_default()), - provider_featured: tags.featured, - branded_provider: sanitize_tag(tags.branded_provider.unwrap_or_default()), - pk_label: sanitize_tag(tags.pk_label.unwrap_or_default()), - byo_label: sanitize_tag(tags.byo_label.unwrap_or_default()), client_source_ip: client.source_ip.clone(), client_user_agent: client.user_agent.clone(), guardrail_blocked, ..Default::default() }; + crate::usage_attr::apply_pk_telemetry(&mut event, &snap, provider_key_id);This would also let the
provider_telemetry_tagsimport at Line 36 be dropped in favor ofapply_pk_telemetry.🤖 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/responses.rs` around lines 1631 - 1664, The `/v1/responses` event assembly is still duplicating the provider telemetry field mapping instead of using the shared helper. Update the `UsageEvent` construction in `responses.rs` to build the event first, then call `apply_pk_telemetry` so the `provider_kind`, `provider_featured`, `branded_provider`, `pk_label`, and `byo_label` fields come from the same single source of truth as the other handlers. This should remove the manual `provider_telemetry_tags` lookup and keep the `UsageEvent` wiring aligned with the `usage_attr` module.
🤖 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/responses.rs`:
- Around line 1631-1664: The `/v1/responses` event assembly is still duplicating
the provider telemetry field mapping instead of using the shared helper. Update
the `UsageEvent` construction in `responses.rs` to build the event first, then
call `apply_pk_telemetry` so the `provider_kind`, `provider_featured`,
`branded_provider`, `pk_label`, and `byo_label` fields come from the same single
source of truth as the other handlers. This should remove the manual
`provider_telemetry_tags` lookup and keep the `UsageEvent` wiring aligned with
the `usage_attr` module.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6724764a-1238-480a-a627-5c1fb8cffefd
📒 Files selected for processing (8)
crates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/completions.rscrates/aisix-proxy/src/embeddings.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/lib.rscrates/aisix-proxy/src/rerank.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/usage_attr.rs
Problem
Provider attribution in the dashboard Logs — 服务商类型 (
provider_kind), 品牌服务商 (branded_provider), PK 标签 (pk_label), plusprovider_featured/byo_label— was wired into/v1/chat/completionsand/v1/messages, and recently/v1/responses(#646 / AISIX-Cloud#867). The remaining billing endpoints left all five UsageEvent fields at their defaults, so their Logs rows showed no upstream vendor or PK label:/v1/completions/v1/embeddings/v1/rerank/v1/audio/transcriptions,/translations,/speech/v1/images/generationsThis is the same oversight #867 fixed for
/v1/responses, replicated across the rest of the handler family (each handler's emit even carried a "wired for chat only / follow-up for the non-chat handlers" comment).Fix
usage_attr::apply_pk_telemetry(&mut event, &snap, provider_key_id)— one source of truth for both the snapshottelemetry_tagslookup AND the five-field wire mapping (withsanitize_tag), so the handler family can't drift apart again.pk_entry.id) through the dispatch-success struct to the emitter and call the shared helper. Empty/unknown id → all-empty tags → wire NULL, matching the chat/messages contract./v1/responseslookup at the shared helper too (removes the duplicate copy added in fix(responses): attribute per-PK telemetry tags on /v1/responses usage events #646).No wire/schema change — the cp-api columns already exist; they were simply never filled by these endpoints.
Tests
Per-handler integration tests (real router + bridge + mock upstream + usage sink) assert the emitted
UsageEventcarries the resolved PK's tags. Verified each fails before the fix (empty tags) and passes after. Fullaisix-proxysuite green (484), clippy + fmt clean.E2E note: these attribution tags don't traverse the OTLP fan-out the standalone DP E2E harness observes (the OTLP encoder is an intentional allowlist, shared across endpoints, that excludes them) — they only reach cp-api via the usage-sink POST. Same testing approach as the original chat/messages attribution feature and #646.
Origin: surfaced by a cross-API consistency audit after AISIX-Cloud#867.
Summary by CodeRabbit
New Features
Bug Fixes