feat(spend): warn when spend-attribution metadata diverges from the resolved key - #31005
feat(spend): warn when spend-attribution metadata diverges from the resolved key#31005yassin-berriai wants to merge 1 commit into
Conversation
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
7a3305e to
10e98ac
Compare
fedbe0e to
c9cef3c
Compare
c9cef3c to
9933c50
Compare
Greptile SummaryThis PR adds Phase 1 of a "Resolve Once, Consume Everywhere" identity design: a divergence guard that emits a structured warning whenever the
Confidence Score: 5/5Safe to merge — the change is additive and zero-behavior-changing; the only externally visible effect is a new warning log on divergence, and the guard is wrapped so it can never disrupt spend tracking. All three files contain self-contained, well-isolated additions. The divergence comparator is a pure function with no DB access or global state. The wiring in the enrichment hook is inside its own try/except and does not alter any existing fill logic. Unit tests are mock-only and cover the three critical paths. No files require special attention.
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/resolvers/divergence.py | New pure module introducing SpendIdentity, FieldDivergence, spend_identity_divergence, and log_identity_divergence; correctly frozen/typed, no side-effects beyond the warning log. |
| litellm/proxy/hooks/proxy_track_cost_callback.py | Wires the divergence guard into _enrich_failure_metadata_with_key_info before the existing fill; guarded by its own try/except so any failure is non-fatal; existing fill logic is unchanged. |
| tests/test_litellm/proxy/auth/test_resolvers_divergence.py | New unit tests (no network calls) covering silent match, all-None consumed vs populated resolved, and single-field user_id divergence; all mock-only as required by repo rules. |
Reviews (2): Last reviewed commit: "feat(spend): warn when spend-attribution..." | Re-trigger Greptile
| verbose_proxy_logger.warning( | ||
| "Spend attribution metadata diverges from the resolved key identity " | ||
| "[credential_ref=%s]: %s", | ||
| resolved_key.token, | ||
| fields, | ||
| ) |
There was a problem hiding this comment.
WARNING volume for valid-key 401 failures
For auth errors where the bearer token resolves to a real key in the DB but the request failed before UserAPIKeyAuth fields were populated (e.g., expired key, revoked key that still exists, early rate-limit checks), all three metadata fields will be None while the resolved key_obj carries actual user_id/team_id/org_id. This fires a WARNING for every such request. In deployments with many invalid auth attempts against valid-but-revoked keys the log can become very noisy. Consider adding a counter metric or sampling, or downgrading to DEBUG for the "all-fields-None-consumed vs populated-resolved" case (i.e., total metadata absence) vs the "some fields present but wrong" case (genuine staleness).
|
Thanks for the review. On the gate and the log-volume points, both are deliberate and narrower than they look. The alias-populated path is intentionally not covered because there is nothing independent to compare against there. The key object (the authoritative reference) is only fetched inside the alias-None branch. In the alias-populated post-auth-failure path the consumed user_api_key_* metadata was itself built from the same resolved UserAPIKeyAuth in async_post_call_failure_hook, so comparing the two would be tautological. The only place an independent identity exists without a new lookup is the empty-metadata branch, where a fresh key fetch can genuinely disagree with what the request carried, which is exactly where the guard sits. Extending it to the alias-populated path would require a second key fetch purely for the guard, the redundant identity lookup this initiative is removing. Fetch-free coverage of every failure path arrives in Phase 2, when identity is resolved once at the seam and carried on the request. On volume: the warning is a no-op unless the freshly fetched key actually disagrees with the metadata, and it only compares user_id, team_id, and org_id. The common fallback case is a post-auth failure where only team_alias is missing from the SQL view while user/team/org are present; there the guard finds no divergence and stays silent. It fires only when user/team/org identity itself was missing or different, which is the real misattribution this is meant to surface, not per-request noise |
Greptile SummaryThis PR introduces Phase 1 of the "Resolve Once, Consume Everywhere" design: a divergence guard that emits a structured
Confidence Score: 4/5The change is purely additive: new module, new import, new try/except-wrapped log call. The existing fill path is untouched, and a guard failure cannot affect cost tracking. The warning emitted by log_identity_divergence fires whenever consumed fields are None and the resolved key has them populated — which is the normal state for every 401 auth error, not just genuine misattributions. High-auth-failure deployments will see constant three-field warnings on each failed unauthenticated request, making the signal harder to act on in production. Everything else (comparison logic, wiring, non-fatality, tests) looks correct. litellm/proxy/auth/resolvers/divergence.py — specifically the log level decision in log_identity_divergence for the all-None consumed case
|
| Filename | Overview |
|---|---|
| litellm/proxy/auth/resolvers/divergence.py | New pure comparator module: SpendIdentity model, FieldDivergence dataclass, and log_identity_divergence helper. Logic is correct; warning fires for both absent and wrong consumed fields including all-None cases from 401 errors. |
| litellm/proxy/hooks/proxy_track_cost_callback.py | Divergence guard wired into _enrich_failure_metadata_with_key_info after key lookup; wrapped in its own try/except so it is non-fatal. Existing fill behavior is unchanged. |
| tests/test_litellm/proxy/auth/test_resolvers_divergence.py | Three unit tests covering: matching identity (silent), empty consumed metadata (warns all three fields), differing user_id (warns correctly). No network calls; consistent with existing test patterns in this directory. |
Reviews (3): Last reviewed commit: "feat(spend): warn when spend-attribution..." | Re-trigger Greptile
| divergences = spend_identity_divergence( | ||
| resolved_identity_from_key(resolved_key), consumed | ||
| ) | ||
| if not divergences: | ||
| return | ||
| fields = ", ".join( | ||
| f"{d.field} (resolved={d.resolved_value!r}, consumed={d.consumed_value!r})" | ||
| for d in divergences | ||
| ) | ||
| verbose_proxy_logger.warning( | ||
| "Spend attribution metadata diverges from the resolved key identity " | ||
| "[credential_ref=%s]: %s", | ||
| resolved_key.token, | ||
| fields, | ||
| ) |
There was a problem hiding this comment.
Warning fires on every 401 auth error, not just genuine misattributions
In scenario 1 from the docstring (auth errors where UserAPIKeyAuth is created with only api_key set), the consumed metadata will always have all three identity fields as None while the looked-up key has them populated. This means every 401 auth failure will emit a 3-field divergence warning — even when nothing is actually wrong with spend attribution.
There is currently no way to distinguish "metadata was absent because request never authenticated" (expected) from "metadata holds a stale/wrong identity" (the actual misattribution signal this guard is designed to surface). High-auth-failure deployments will see constant noise, making the signal hard to act on. Consider logging at DEBUG for the all-None consumed case and reserving WARNING for when consumed_value is a non-None value that differs from resolved_value.
Relevant issues
Linear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
make test-unit@greptileaiand received a Confidence Score of at least 4/5 before requesting a maintainer reviewCI (LiteLLM team)
Branch creation CI run
Link:
CI run for the last commit
Link:
Merge / cherry-pick CI run
Links:
Screenshots / Proof of Fix
Verified against a live proxy on
localhost:4000backed by Postgres, hitting the real Anthropic API, running this branch withPYTHONPATHpinned to the worktree (the divergence module was confirmed loaded from the worktree, not the main checkout).The guard sits in the failure-path fallback
_enrich_failure_metadata_with_key_info, so it only evaluates when a failed request reaches spend logging. On a team-scoped key carrying a user, team, and org, one successful completion and one deliberately failing call (a model the key is not allowed to use, a real 403) both write spend rows with the correct identity:The failure row carries the resolved user, team, and org, so the enrichment fallback found identity already present and the guard correctly stayed silent: no
Spend attribution metadata diverges from the resolved key identitywarnings and no guard errors in the proxy log (the only traceback is the expectedkey_model_access_deniedProxyException from the 403). This shows the guard is non-disruptive on healthy traffic and that failure-path attribution is intact. The firing behavior, where the consumed metadata identity is missing or different from the freshly fetched key (the empty-metadata misattribution case the design cites), is pinned by the unit tests intests/test_litellm/proxy/auth/test_resolvers_divergence.py: an empty consumed identity against a populated key warns naming exactly user_id, team_id, and org_id, and a matching identity stays silentType
🆕 New Feature
Changes
Phase 1 of the internal Caller Identity design ("Resolve Once, Consume Everywhere"); the first consumer of the Phase 0 resolved identity work (PR #30887)
The design's failure table calls out spend being mis- or un-attributed when the flattened
user_api_key_*request metadata is empty or stale, which the spend path silently papers over by falling back to a key lookup. That fallback lives in_enrich_failure_metadata_with_key_info; it fetches the key object and fillsmetadata["user_api_key_user_id"/"team_id"/"org_id"]whenever they are None, with no signal that the attribution was missing in the first place. This change turns that silent fill into an observable warning so the divergence surfaces before deploy instead of hiding in the spend logslitellm/proxy/auth/resolvers/divergence.pyholds a pure, fully typed comparator.spend_identity_divergencecompares a resolvedSpendIdentityagainst the consumed one and returns the fields that are missing-in-consumed or hold a different value (user_id, team_id, org_id) as an immutable tuple of frozen records.resolved_identity_from_keyprojects the authoritative identity off the fetched key object,SpendIdentityis a frozen model so the untyped metadata values are validated at the boundary, andlog_identity_divergenceemits a single structuredverbose_proxy_logger.warningnaming the diverging fields (resolved value vs consumed value) keyed by the hashed token, and is a no-op when nothing divergesThe guard is wired into
_enrich_failure_metadata_with_key_inforight after the key object is fetched and before the existing fill runs, comparing the pre-fill metadata against the key. The existing fill behavior is unchanged; this only adds the warning, and it is wrapped in its own try/except so a guard failure can never affect the enrichment or cost tracking. The change is additive and zero-behavior-change; the only externally observable effect is a warning log on divergenceTests in
tests/test_litellm/proxy/auth/test_resolvers_divergence.pycover matching identity (silent), empty consumed metadata against a populated key naming exactly the missing fields (the misattribution case), and a consumed user_id that differs from the key. The enrichment fallback is not cleanly injectable (get_key_objectand the proxy-server globals it reads are module-level rather than injected), so the warning is asserted through the thinlog_identity_divergencewrapper rather than by mocking that dependency