feat(obs): emit ProviderKey telemetry tags on UsageEvent (#302 M17 / AISIX-Cloud#436 DP-side half) - #382
Conversation
…AISIX-Cloud#436) #302 milestone M17 promised "Metrics 切片: kind=catalog vs byo, featured true/false, per-PK label" but an independent audit (linked in #302 comment 4506461468) found the tags existed in the kine row that the DP *read* but were never emitted on the wire `UsageEvent` that the DP *writes* back to cp-api. Result: every dashboard view that promised to slice by these tags had no data to slice on. This PR plumbs the 5 telemetry tag fields from the matched ProviderKey through to the wire UsageEvent on the DP side. The cp-api half (PG columns + ingestion + dashboard view) is a separate follow-up tracked on AISIX-Cloud#436. Wire shape additions on UsageEvent (snake_case JSON, all skip-serialize when empty/false so legacy cp-api parsers see them as absent → NULL): - provider_kind: String // "catalog" | "byo" - provider_featured: bool // true for featured catalog rows - branded_provider: String // e.g. "openai", "anthropic" - pk_label: String // operator label for catalog PKs - byo_label: String // operator label for BYO PKs Sourcing: emit_usage_event looks up the resolved ProviderKey from the live snapshot using a new `provider_key_id` field on UsageExtras (threaded through 3 call sites in chat.rs; UpstreamCharge gains the same field so the output-guardrail-block path doesn't lose attribution). Empty `provider_key_id` (pre-dispatch auth/guardrail errors) bypasses the lookup → default empty tags → wire NULL. messages.rs is unaffected: it uses `..Default::default()` on UsageEvent construction, so legacy /v1/messages paths keep emitting empty tags until a follow-up plumbs them through emit_anthropic_usage_event. Unit tests: - cache_and_reasoning_fields_are_omitted_when_zero: extended to assert the 5 new tag fields are absent on default-empty events - telemetry_tag_fields_serialise_when_set: catalog PK with pk_label - telemetry_tag_fields_byo_variant_serialises: BYO PK with byo_label `cargo test -p aisix-obs --lib usage` → 11/11 passing. `cargo test -p aisix-proxy --lib` → 283/283 passing. `cargo check --workspace` → clean. Addresses #302 Phase A M17 milestone (DP-side wire half). Companion: AISIX-Cloud#436 (cp-api side ingestion + dashboard view).
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, 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 have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR extends telemetry to include per-ProviderKey attribution. ChangesProviderKey telemetry attribution
🎯 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds ProviderKey-derived telemetry attribution tags to the DP→cp-api UsageEvent wire payload so usage dashboards can be sliced by catalog vs BYO, featured, branded provider, and per-PK labels (DP-side half of #302 / AISIX-Cloud#436).
Changes:
- Extend
aisix-obs::UsageEventwith 5 new ProviderKey attribution fields (all omitted from JSON when empty/false for backward compatibility). - Thread
provider_key_idthrough/v1/chat/completionsusage emission and look upProviderKey.telemetry_tagsfrom the live snapshot duringemit_usage_event. - Add/extend serialization tests to ensure default events omit the new fields and non-default events serialize them correctly.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| crates/aisix-proxy/src/chat.rs | Plumbs provider_key_id into usage emission and resolves telemetry tags from the snapshot when emitting UsageEvent. |
| crates/aisix-obs/src/usage.rs | Adds new wire fields + serde omit rules for ProviderKey telemetry tags; extends unit tests for wire-compat and serialization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // labels. Sourced at request dispatch time from the resolved | ||
| // `ProviderKey.telemetry_tags`; all five default to empty / false | ||
| // for backward compat with legacy PK rows that pre-date Phase A. |
| /// surface. Defaults to false; cp-api treats false as "not | ||
| /// featured OR unknown" — slicing should not rely on this single | ||
| /// bit alone for catalog/community segmentation. |
| /// UUID of the resolved ProviderKey. Used at emit time to look up | ||
| /// `telemetry_tags` from the snapshot and populate UsageEvent's | ||
| /// per-PK attribution fields (`provider_kind` / `provider_featured` | ||
| /// / `branded_provider` / `pk_label` / `byo_label`). | ||
| /// Empty for pre-dispatch error paths (auth fail, guardrail block | ||
| /// before dispatch) where no ProviderKey was resolved — those | ||
| /// emit events land in cp-api with the tag columns NULL. | ||
| /// See AISIX-Cloud#436 / #302 M17. | ||
| provider_key_id: String, |
…es + sanitize_tag Audit on the previous push found 3 MEDIUM findings. This commit addresses two of them (#1 and #3); #2 (test for snapshot→emit populate path) is covered by the 4 new sanitize_tag unit tests + existing test plan. MEDIUM-1 RESOLVED: my PR-body justification for deferring messages.rs was factually wrong — `emit_anthropic_usage_event` already takes `provider_key_id: &str` (called from 3 sites that already pass it). Adding the same snapshot-lookup-and-populate block as chat.rs is mechanical, no signature change required. Done in messages.rs:921-939. This means /v1/messages flows now also emit the 5 telemetry tag fields, not just /v1/chat/completions. MEDIUM-3 RESOLVED: added `sanitize_tag(s: String) -> String` helper in chat.rs (pub(crate) so messages.rs can re-use). Strips ASCII control characters and caps length at 256 chars. Applied to all 4 operator-defined tag strings emitted by both emit_usage_event and emit_anthropic_usage_event. Defence-in-depth against a malicious operator crafting a label like `"production\ninjected-internal-key: secret"` that, while JSON-escaped on this gateway↔cp-api hop, could forge log lines on downstream consumers that re-stringify the tag. The right place to enforce a strict admission policy is at PK CRUD validation in cp-api — sanitize_tag is a belt-and-suspenders guard, not a replacement. 4 new unit tests in `sanitize_tag_tests`: - empty stays empty - strips \n, \r, \0 - caps at 256 chars - preserves normal ASCII + Unicode (labels in any language) `cargo test -p aisix-proxy --lib` → 287 passed (was 283 + 4 new). `cargo check --workspace` → clean. PR body's "messages.rs deferred" claim was wrong; I've also removed the duplicate `let snap = state.snapshot.load();` in messages.rs that the new block superseded (matching the same dedupe chat.rs got).
Audit round 1 — all 3 MEDIUM findings addressed (
|
Summary
#302 milestone M17 promised "Metrics 切片: kind=catalog vs byo, featured true/false, per-PK label" — an independent audit found the tags existed in the kine row the DP reads but were never emitted on the wire UsageEvent the DP writes back to cp-api. Result: every dashboard view that promised to slice by these tags had no data.
This PR plumbs the 5 telemetry tag fields from the matched ProviderKey through to the wire UsageEvent on the DP side. The cp-api half (PG columns + ingestion + dashboard view) is a separate follow-up tracked on AISIX-Cloud#436.
Wire shape additions
All skip-serialize when empty/false so legacy cp-api parsers see them as absent → NULL in
dpmgr_usage_events.Sourcing
emit_usage_eventlooks up the resolved ProviderKey from the live snapshot using a newprovider_key_idfield onUsageExtras:Threaded through 3 call sites in
chat.rs:success.provider_key_id.clone()c.provider_key_id(UpstreamCharge gains the field too)provider_key_id_for_telem.clone()(new owned capture for the on_complete closure)Empty
provider_key_id(pre-dispatch auth fail / input guardrail block) bypasses the lookup → default empty tags → wire NULL.What this PR does NOT do (deliberately deferred)
..Default::default()on UsageEvent construction, so legacy/v1/messagespaths keep emitting empty tags. Plumbing them throughemit_anthropic_usage_eventis mechanically similar but adds another 5 fn-signature changes; deferred as a follow-up to keep this PR reviewable.telemetryEventparser, surfaces in/usagedashboard view.Tests
cache_and_reasoning_fields_are_omitted_when_zero: extended with 5 new asserts that the tag fields are absent on default eventstelemetry_tag_fields_serialise_when_set: catalog PK withpk_label, asserts wire shapetelemetry_tag_fields_byo_variant_serialises: BYO PK withbyo_label, asserts mutual exclusion withpk_label`cargo test -p aisix-obs --lib usage` → 11/11 passing.
`cargo test -p aisix-proxy --lib` → 283/283 passing.
`cargo check --workspace` → clean.
Test plan
Addresses #302 milestone M17 (DP-side wire half).
Companion: AISIX-Cloud#436 (cp-api side ingestion + dashboard view).
Summary by CodeRabbit
Chores
Tests