fix(otel): mark v2 server spans as failed for pre-call errors - #34546
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Greptile SummaryUpdates malformed-request handling to authenticate before re-raising parse errors and marks affected OTel v2 server spans as failed without duplicating exception events
Confidence Score: 5/5The PR appears safe to merge because no blocking failures remain No blocking failures remain
|
| Filename | Overview |
|---|---|
| litellm/integrations/otel/logger.py | Marks pre-call server-span failures as errors and conditionally records an exception event |
| litellm/proxy/auth/user_api_key_auth.py | Authenticates malformed requests for trace identity before re-raising the original parse failure |
| tests/test_litellm/integrations/otel/test_otel_v2_logger.py | Covers server-span error status and exception-event deduplication |
| tests/test_litellm/proxy/auth/test_user_api_key_auth.py | Covers malformed-body authentication, identity seeding, skipped authorization, and preserved parse-error responses |
Reviews (4): Last reviewed commit: "Merge remote-tracking branch 'origin/lit..." | Re-trigger Greptile
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
… (LIT-4780) Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
…4780 Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
@gerptile run again |
|
@greptileai review latest head |
|
bugbot run |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 626e1ee. Configure here.
| ) | ||
| if body_parse_exception is not None: | ||
| raise body_parse_exception | ||
| return recovered_auth_obj |
There was a problem hiding this comment.
Recovered auth skips identity seed
Low Severity
When _handle_authentication_error recovers and body_parse_exception is set, the parse error is re-raised inside the phase_span before seed_request_identity runs. That leaves the rejected trace without identity and marks the auth span as failed for a parse error after auth already recovered.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 626e1ee. Configure here.
|
Ran a base vs head A/B on a live proxy (real Postgres, real provider calls, OTel v2 console exporter). The span fix works as described: a malformed-body 400 goes from Two things worth a look before merge. Budget reservation is orphaned on model-in-path routes. Deferring the parse failure means the full auth phase now runs, and its last step reserves budget by incrementing the live spend counters. The re-raise at the end of Three malformed requests exhaust the budget and the key stays locked out, while recorded DB spend is 0.0000031. Repro: It does not reproduce on The client-visible status code changes. Malformed body plus an invalid key returns 400 on base and 401 on head, since Scoping note: the deferral and both re-raise sites carry no OTel condition, so this applies with v1, with v2, and with OTel disabled. The PR reads as v2-scoped; the auth change underneath is global. That one I confirmed by reading the code, not by running with OTel off. |
Deferring the parse failure ran the full auth phase, including budget reservation, whose reserved amount is only released by the endpoint's post call path; the endpoint never runs, so malformed requests leaked reservations and locked a budgeted key out. Authorization now runs only when the body parsed, and a parse failure with a rejected key keeps returning the 400 it returned before. Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
|
Both fixed in cfc1866: authorization now skipped for unparsed bodies, so no reservation, and a rejected key still returns 400. Description corrected, budget A/B re-run clean |
…itellm_otel_v2_error_status_lit_4780
|
|
|
@greptileai review latest head |
|
Ran a dependency blast-radius pass on Two side effects of authenticating before rejecting that are not mentioned in the description or covered by a test. Both follow from the design rather than being defects, but they are worth recording so the trade is made on purpose. An operator's Anyone whose custom auth does an IdP round trip, an audit write, or a per-caller counter picks up new volume on unparseable input. Unauthenticated requests now do database work. Twenty requests carrying an unknown key and a malformed body: That is roughly four transactions per request from a caller who is not authenticated, against essentially none before. It is a cheap amplifier, though a rate limiter in front covers the realistic case. Which raises the design question worth answering before merge: how much of the identity goal needs the full resolution? The API key is already in the header, so hashing it would put Separately, and not a blocker for this PR: LIT-4780 is fixed on the 585 routes that go through the dependency and still live on that one. Worth a follow-up ticket so the inconsistency is tracked. |
|
Hashing the key alone misses what the ticket asked for, team name, id and metadata, which need the lookup. Filing a follow-up for the MCP OAuth copy |
|
Filed LIT-5363 for the MCP OAuth copy: https://linear.app/litellm-ai/issue/LIT-5363/bug-mcp-oauth-token-route-misses-the-otel-v2-error-span-fix |


TLDR
Problem this solves:
How it solves it:
record_error_attributes_on_spannow sets the ERROR statuserror.codestays the response statusRelevant issues
Linear ticket
Resolves LIT-4780
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Delays in PR merge?
If you're seeing a delay in your PR being merged, ping the LiteLLM Team on Slack (#pr-review).
Screenshots / Proof of Fix
Every image below is generated from a recorded run against live proxies; no numbers are typed by hand. The suite is 42 scenarios run in identical order against three trees on separate ports and separate Postgres instances, with real Gemini calls and the OTel v2 console exporter: the merge-base
b6e3ff63, the previous head626e1eeff8, and the current head613ea891.The bug this PR fixes
A malformed body used to leave the SERVER span UNSET with no exception event, on an anonymous trace. It now reads as a failure, carries the caller's identity, and has the
authchild span.The gate: authorization is skipped only when the body never parsed
Deferring the parse failure meant the full auth phase ran, and its last step reserves budget by incrementing the live spend counters. Those are released by the endpoint, which never runs for a malformed body, so the previous head leaked a reservation per request and locked the key out. Three malformed requests exhausted a 1.00 USD key while its recorded spend was 0.0000031.
Azure-style model-in-path route, six malformed requests against a 1.00 USD key, then one legitimate call:
Same on the Gemini-compatible route:
A malformed body sent with a rejected key returns the 400 it returned before the PR; the auth failure is still recorded on the
authchild span:The gate in the other direction
The risk of a skip-authorization gate is that it skips too much. For a well-formed body every authorization check still runs, and head matches base on all of them:
Concurrency: 15 malformed and 15 valid requests fired simultaneously on one budgeted key return 400 and 200 respectively, and the following legitimate call returns 200, so the gate does not race.
The dedup gate
The exception event is recorded only when nothing stamped the span yet, so a failure that already went through
async_post_call_failure_hookis not double counted. Exactly one event on every path:Happy paths and edge cases
The three 500s and the 422 in that matrix reproduce at the merge-base as well, so they are pre-existing and not introduced here.
Full matrix
42 of 42 scenarios on the current head are identical to the merge-base. Seven differed on the previous head; all seven are fixed.
Type
🐛 Bug Fix
Changes
The proxy's global exception handlers turn a failure into a
JSONResponse, so the FastAPI instrumentor never sees the exception and never statuses the span it owns. For failures that reachasync_post_call_failure_hook(auth rejection, unknown model, empty body) that hook stamps status plus the exception event, so the trace looks right. A body that fails to parse dies earlier, inside theuser_api_key_authdependency, so the only OTel hook that runs isrecord_error_attributes_on_span, which deliberately passedrecord_event=False, set_status=False; the span then carrieserror.messagewhile reading as a non-error span with no event. v1 set the ERROR status for the same request from_close_dangling_otel_server_span, which is why this only shows up on v2.record_error_attributes_on_spannow sets the status, and records the exception event only when the span has noerror.typeon it yet, so the paths that already recorded one don't get a second. Attributes are restamped either way, keepingerror.codepinned to the real response status.Separately,
user_api_key_authread and parsed the body before authenticating, so a parse failure short-circuited auth entirely; identity was never resolved andseed_request_identitynever ran, which is why the root span had no key / team / user on it and no children. It now catches the parse failure, authenticates the caller with an empty body, and re-raises the originalProxyExceptiononce identity is seeded, so the trace gets theauthchild span plus identity attributes like every other failure. This part of the change is not conditional on OTel, it reorders the dependency for every deploymentA request whose body never parsed is authenticated but deliberately not authorized: the
common_checksgate is skipped, since there is no model to check it against and its budget reservation increments live spend counters that only the endpoint's post-call path releases. The endpoint never runs here, so running it would leak a reservation per malformed request and lock a budgeted key out. The client-visible response is unchanged from base in every case, including a malformed body sent with a key that fails auth, which still gets the 400 rather than the auth statusNote that such a request still has no LLM child span; there was no provider call to represent, and inventing one would put a phantom CLIENT span in the trace.
Final Attestation
Link to Devin session: https://app.devin.ai/sessions/a132d58d757c4b1395d3ddce219d8d6b
Requested by: @shivamrawat1