fix(logging): fall back to litellm_metadata when metadata is empty - #36105
Conversation
Greptile SummaryThe PR restores metadata propagation when only
Confidence Score: 5/5The PR appears safe to merge because no blocking failure remains within the eligible follow-up scope No blocking failure remains
|
| Filename | Overview |
|---|---|
| litellm/litellm_core_utils/get_litellm_params.py | Resolves metadata from a guarded copy of litellm_metadata and uses it for identifier derivation and returned parameters |
| litellm/litellm_core_utils/litellm_logging.py | Copies dictionary metadata before merging logging fields, preventing top-level mutation of caller-owned request data |
| tests/test_litellm/litellm_core_utils/test_get_litellm_params.py | Adds focused regression coverage for fallback precedence, invalid types, identifier derivation, and copy isolation |
| tests/test_litellm/litellm_core_utils/test_litellm_logging.py | Verifies malformed metadata handling and confirms that logging merges do not mutate the caller’s dictionary |
Reviews (4): Last reviewed commit: "fix(logging): fall back to litellm_metad..." | Re-trigger Greptile
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 78aaa52. Configure here.
a33983d to
4a87735
Compare
78aaa52 to
070877c
Compare
|
@greptileai please review the current head 070877c. Since your last review the test docstrings were dropped and the branch was rebased onto the updated #35866 |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 070877c. Configure here.
070877c to
c884213
Compare
|
@greptileai please review the current head c884213. Fixes a TypeError when a caller sends metadata null alongside litellm_metadata |
|
bugbot run |
|
@greptileai please re-check head c884213. The docstring finding looks stale, the diff has no added docstring lines |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit c884213. Configure here.
| if isinstance(kwargs.get("metadata"), dict): | ||
| base_litellm_params["metadata"] = kwargs["metadata"].copy() | ||
| if "litellm_metadata" in kwargs and isinstance(kwargs["litellm_metadata"], dict): | ||
| base_litellm_params["litellm_metadata"] = kwargs["litellm_metadata"] | ||
| if "metadata" not in base_litellm_params: |
There was a problem hiding this comment.
🟡 Requests that send an empty metadata object still lose their tracking information in logs
An empty metadata object sent by the caller is treated as real metadata (isinstance(kwargs.get("metadata"), dict) at litellm/litellm_core_utils/litellm_logging.py:588-593) instead of falling back to the tracking data the proxy stored separately, so callbacks and spend logs for those requests still see nothing.
Impact: On routes like batches, videos, and vector stores, a client that sends "metadata": {} gets logs and spend records without the proxy key/session information, while the same request with no metadata field at all is logged correctly.
Inconsistent emptiness check between the two fallback sites
This PR changed get_litellm_params to fall back when metadata is falsy (if not metadata and _litellm_metadata_dict at litellm/litellm_core_utils/get_litellm_params.py:119), but update_from_kwargs still keys the backfill off key presence: {} is a dict, so base_litellm_params["metadata"] = {} is set and the if "metadata" not in base_litellm_params guard at litellm/litellm_core_utils/litellm_logging.py:592 skips the litellm_metadata copy. Callers that also pass metadata inside litellm_params (e.g. litellm/responses/main.py:1092) recover via the merge below, but callers that don't (litellm/batches/main.py:193, litellm/videos/main.py:229, litellm/vector_stores/main.py:230) end up with an empty metadata dict in logging_obj.litellm_params.
Using truthiness in both places (if not base_litellm_params.get("metadata")) makes the two fallback sites agree.
(Refers to lines 588-593)
Was this helpful? React with 👍 or 👎 to provide feedback.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
c884213 to
2146318
Compare
get_litellm_params returned metadata=None whenever only litellm_metadata was supplied, which overwrote the fallback function_setup had already applied and left litellm_params["metadata"] empty. On the /v1/responses completion-transformation bridge, used by every provider without a native Responses API config, and on /v1/messages, that discarded the caller's trace fields a second time after the proxy had promoted them. Resolve metadata to a copy of litellm_metadata when metadata is empty, guarding on isinstance because the proxy leaves an unparseable litellm_metadata string in place and a null metadata would otherwise suppress the backfill and break the merge. update_from_kwargs copies rather than aliases for the same reason: on these routes it is handed the caller's provider-bound dict and would otherwise write user_api_key_auth into it.
2146318 to
749b823
Compare
|
@greptileai please review the current head 749b823. #35866 merged, so this was rebased onto staging and is now just the get_litellm_params and update_from_kwargs changes |
|
bugbot run |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 749b823. Configure here.
f3f72c4
into
litellm_internal_staging
TLDR
Problem this solves:
get_litellm_paramsreturnedmetadata=Nonewhenever a caller supplied onlylitellm_metadata, overwriting a fallbackfunction_setuphad already applied correctly and leavinglitellm_params["metadata"]empty/v1/responsesthrough the chat-completions bridge, and on that path this discarded the caller's trace fields a second time, after the proxy had already promoted them in fix(proxy): promote caller metadata trace fields into litellm_metadata #35866Logging.update_from_kwargsmerges proxy-internal fields into whatever dict it is handed and was aliasing rather than copying it. On these routes that dict is the caller's provider-boundmetadata, so once the merged value stopped beingNoneit would writeuser_api_key_hashanduser_api_key_authinto an outbound request bodyHow it solves it:
metadatato a copy oflitellm_metadatawhenmetadatais empty, which is the same fallbacklitellm/utils.pyalready applies two frames upisinstance, since the proxy deliberately leaves an unparseable stringlitellm_metadatain place andstrhas nocopyupdate_from_kwargsinstead of aliasing, so the merge cannot reach the caller's dictRelevant issues
Second half of the fix for #34226. Stacked on #35866, which promotes the caller's trace fields into
litellm_metadatafor routes that track proxy state there. #35866 covers providers with a native Responses API config; this covers the rest, so both are needed to close the issueLinear ticket
Pre-Submission checklist
Please complete all items before asking a LiteLLM maintainer to review your PR
@greptileaito re-request a review after pushing changes)Screenshots / Proof of Fix
Live proxy, real Postgres, real Gemini traffic, Langfuse pointed at a local ingestion endpoint so the raw
trace-createbody is visible./v1/responsesagainst a provider with no native Responses API config, so the request takes the completion-transformation bridge:The aliasing half, reproduced directly against the resolved params rather than through a route, since
_aresponses_websocketis the reachable entry point and takesmetadatathrough**kwargs:And the malformed-input guard, which is the regression the isinstance check prevents:
Without the guard the first three raise
AttributeError: 'str' object has no attribute 'copy', which surfaces as a 500 on a request that previously succeededType
🐛 Bug Fix
Changes
get_litellm_paramsresolvesmetadatato a copy oflitellm_metadatawhenmetadatais empty andlitellm_metadatais a dict. The same value feeds thelitellm_session_idandlitellm_trace_idderivation directly above it, so call chaining now works on these routes too.Logging.update_from_kwargscopieskwargs["metadata"]rather than aliasing it. The merge below it writes proxy-internal fields into that dict, and onlitellm_metadataroutes it is the caller's provider-bound object.base_modelstill derives from the rawmetadatarather than the resolved one. The final value is unchanged either way because_get_base_model_from_metadatainlitellm/utils.pyalready falls back tolitellm_metadata, so it is left alone to keep the change scoped.Behavior changes
All three below follow from one thing: on the routes that pass
litellm_metadatawith nometadata, meaning the/v1/responsesbridge,/files, metadata-less/batchesand bedrock passthrough,litellm_params["metadata"]changes fromNoneto a copy of thelitellm_metadatadict. Code that reads that dict had been failing into a swallowingtryand now runs. Each was confirmed on a base-vs-head proxy A/B rather than reasoned about.Worth a release note.
max_budget_per_sessionstarts being enforced on these routes. Its pre-call gate already read both metadata dicts, but the spend increment readslitellm_params["metadata"]["session_id"], which never existed there, so the counter never incremented and the limit was inert. With an agent-scoped request the handler seessession_id=None agent_id=Noneon base and returns early; on head it sees both and increments. An operator who configured this months ago has been running without it on these routes, and after this change requests that used to succeed can start returning 429 with no config change on their side. Correct behavior, but it arrives without warning.Data change, no action needed. The SpendLogs
session_idcolumn changes from a per-request UUID to the caller's session id when one is supplied, becauselitellm_session_idandlitellm_trace_idcan now be derived on these routes. The column is named for what it now holds, and a random per-request value made session grouping useless there, so this is the intended repair. Thex-litellm-session-idheader already produced this same value on every route. Dashboards that group by that column will see the grouping start working.Strict repair. Callbacks on these routes now receive the same proxy metadata that
/chat/completionsand the native/v1/responsespath have always sent, including theuser_api_key_*fields. A Lago-style bare index onlitellm_params["metadata"]["user_api_key_user_id"]raisesTypeErroron base and returns the value on head, which is the shape PromptLayer and Slack alerting use too. These integrations were simply broken on these routes before.Correction to an earlier revision of this description, recorded rather than silently removed: it also listed the v1 parallel-request limiter's success handler as newly running. That was wrong. Only
_PROXY_MaxParallelRequestsHandler_v3is registered in a default deployment,parallel_request_limiter.pynever appears in the proxy log, and the v3 handler does not readlitellm_params["metadata"].QA runbook
Run the curl above against a model whose provider has no native Responses API config, with the langfuse success callback enabled and
LANGFUSE_HOSTpointed at any server that acceptsPOST /api/public/ingestionand returns 207. The capturedtrace-createbody should carry the caller'strace_idwithsessionId,userIdandmetadatapopulated. Then repeat with"litellm_metadata": "not-json"in the body and confirm the request still succeeds rather than returning a 500Final Attestation
Note
Medium Risk
Behavior change on litellm_metadata-only routes: spend/session limits, SpendLogs session_id, and callbacks may start seeing metadata that was previously null; logging no longer mutates caller dicts but downstream code that relied on that side effect could differ.
Overview
Fixes trace/metadata loss and accidental mutation on routes that only pass
litellm_metadata(e.g./v1/responseschat bridge).get_litellm_paramsnow setsmetadatato a copy oflitellm_metadatawhenmetadatais missing or empty andlitellm_metadatais a dict (non-dicts are ignored so unparseable proxy strings do not 500). That resolved dict driveslitellm_params["metadata"]and session/trace ID derivation, aligning with the fallback already applied higher inlitellm/utils.py.Logging.update_from_kwargsonly treatskwargs["metadata"]as metadata when it is a dict and copies it before merging proxy fields, so logging setup no longer writesuser_api_key_*into the caller’s outbound provider metadata object.New unit tests cover fallback precedence, malformed
litellm_metadata, copy-vs-alias behavior, and logging merge edge cases.Reviewed by Cursor Bugbot for commit 749b823. Bugbot is set up for automated code reviews on this repo. Configure here.