mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-09 03:18:44 +00:00
fix(otel/v2): address review — move destinations to presets/, stash on litellm_metadata
Two adjustments from Yassin's review on #30873: 1. Move integrations/otel/destinations.py into integrations/otel/presets/. The module dispatches across per-backend presets and reads no request data, so it belongs in the presets package. Importers updated; tests renamed to match. 2. Do not stamp otel_destinations as a top-level kwarg on the proxy request data. Top-level unknown keys get forwarded to the provider request body (the "Extra inputs are not permitted" 400 hit during injection testing was this exact path). Stash on data["litellm_metadata"]["otel_destinations"] instead; litellm_metadata is in all_litellm_params and is scrubbed from the body before reaching the provider. The dynamic-params resolver reads from litellm_metadata only; a top-level otel_destinations value is now ignored. Pre-call wipe extended to pop the client value from both locations. Tests: 240 backend tests pass; new test guards that a top-level otel_destinations is ignored; assertion updated to look under litellm_metadata.
This commit is contained in:
parent
1c6a3b2e64
commit
ffcea908a6
8 changed files with 60 additions and 23 deletions
|
|
@ -9,7 +9,7 @@ the factory in ``litellm_logging`` can resolve a name and build a single
|
|||
|
||||
Per-key/team routing does not live here. A trace destination is admin-owned
|
||||
infrastructure config, resolved server-side from a named credential into an
|
||||
``OtelDestination`` (see ``litellm.integrations.otel.destinations`` and
|
||||
``OtelDestination`` (see ``litellm.integrations.otel.presets.destinations`` and
|
||||
``plumbing.routing``); nothing in this package reads vendor credentials or a
|
||||
host off a request.
|
||||
"""
|
||||
|
|
|
|||
|
|
@ -109,11 +109,19 @@ def initialize_standard_callback_dynamic_params(
|
|||
standard_callback_dynamic_params[param] = _param_value # type: ignore
|
||||
|
||||
# Admin-owned OTEL v2 destinations, resolved server-side by the proxy from the
|
||||
# exporters assigned to the request's identity chain and stamped onto top-level
|
||||
# kwargs (the proxy strips any client-supplied value first). Read from top-level
|
||||
# only, never from request metadata, and never via _supported_callback_params,
|
||||
# so a request body/metadata cannot set or select a trace destination.
|
||||
otel_destinations = kwargs.get("otel_destinations")
|
||||
# exporters assigned to the request's identity chain. The proxy stamps them
|
||||
# onto ``data["litellm_metadata"]["otel_destinations"]`` -- ``litellm_metadata``
|
||||
# is in ``all_litellm_params``, so it is scrubbed from the body before it reaches
|
||||
# the provider. ``otel_destinations`` is intentionally NOT a top-level key so an
|
||||
# unknown field cannot leak to provider APIs. Read from ``litellm_metadata`` only,
|
||||
# never from request ``metadata``, and never via ``_supported_callback_params``,
|
||||
# so a request body cannot set or select a trace destination.
|
||||
proxy_metadata = kwargs.get("litellm_metadata") or {}
|
||||
otel_destinations = (
|
||||
proxy_metadata.get("otel_destinations")
|
||||
if isinstance(proxy_metadata, dict)
|
||||
else None
|
||||
)
|
||||
if isinstance(otel_destinations, list):
|
||||
standard_callback_dynamic_params["otel_destinations"] = cast(
|
||||
"list[OtelDestinationParams]", otel_destinations
|
||||
|
|
|
|||
|
|
@ -630,7 +630,7 @@ async def _resolve_logging_exporters(
|
|||
headers). The request never names or supplies a destination. Returns ([], []) only
|
||||
when nothing is selected (default-deny).
|
||||
"""
|
||||
from litellm.integrations.otel.destinations import build_destination
|
||||
from litellm.integrations.otel.presets.destinations import build_destination
|
||||
|
||||
names = await _union_logging_exporter_names(user_api_key_dict)
|
||||
team_id, org_id = user_api_key_dict.team_id, user_api_key_dict.org_id
|
||||
|
|
@ -685,12 +685,21 @@ async def _apply_admin_logging_exporters(
|
|||
data: dict, user_api_key_dict: UserAPIKeyAuth
|
||||
) -> None:
|
||||
"""Stamp the resolved fan-out destinations onto ``data`` and activate their
|
||||
backends. A client value was already stripped by the caller; default-deny means
|
||||
an identity with no assignment gets no per-tenant destination here."""
|
||||
backends.
|
||||
|
||||
The destinations live under ``data["litellm_metadata"]`` (in
|
||||
``all_litellm_params``, so scrubbed from the provider request body), not a
|
||||
top-level key, so an unknown field cannot leak to the provider. Default-deny
|
||||
means an identity with no assignment gets no per-tenant destination here.
|
||||
"""
|
||||
destinations, backends = await _resolve_logging_exporters(user_api_key_dict)
|
||||
if not destinations:
|
||||
return
|
||||
data["otel_destinations"] = destinations
|
||||
proxy_metadata = data.get("litellm_metadata")
|
||||
if not isinstance(proxy_metadata, dict):
|
||||
proxy_metadata = {}
|
||||
proxy_metadata["otel_destinations"] = destinations
|
||||
data["litellm_metadata"] = proxy_metadata
|
||||
existing = data.get("success_callback") or []
|
||||
data["success_callback"] = list(dict.fromkeys([*existing, *backends]))
|
||||
|
||||
|
|
@ -1983,8 +1992,13 @@ async def add_litellm_data_to_request(
|
|||
|
||||
# Team Callbacks controls
|
||||
# A client must never set or override OTEL destinations; they are admin-owned and
|
||||
# resolved server-side below, so drop any value carried in the request.
|
||||
# resolved server-side below. Drop any value carried in the request at either the
|
||||
# top level OR inside litellm_metadata (the resolver stashes admin-resolved values
|
||||
# in litellm_metadata.otel_destinations; we wipe the client's first so injection
|
||||
# via either spot is inert).
|
||||
data.pop("otel_destinations", None)
|
||||
if isinstance(data.get("litellm_metadata"), dict):
|
||||
data["litellm_metadata"].pop("otel_destinations", None)
|
||||
callback_settings_obj = _get_dynamic_logging_metadata(
|
||||
user_api_key_dict=user_api_key_dict, proxy_config=proxy_config
|
||||
)
|
||||
|
|
|
|||
|
|
@ -50,7 +50,9 @@ def test_destination_routable_presets_tag_exporter_with_matching_owner(monkeypat
|
|||
that integration's admin destination at its own exporter only and never
|
||||
rewrites a co-configured backend's exporter.
|
||||
"""
|
||||
from litellm.integrations.otel.destinations import OTEL_V2_DESTINATION_CALLBACKS
|
||||
from litellm.integrations.otel.presets.destinations import (
|
||||
OTEL_V2_DESTINATION_CALLBACKS,
|
||||
)
|
||||
from litellm.integrations.otel.presets import PRESET_BY_CALLBACK
|
||||
|
||||
monkeypatch.setenv("ARIZE_SPACE_ID", "S")
|
||||
|
|
|
|||
|
|
@ -11,7 +11,7 @@ import sys
|
|||
|
||||
sys.path.insert(0, os.path.abspath("../../../.."))
|
||||
|
||||
from litellm.integrations.otel.destinations import (
|
||||
from litellm.integrations.otel.presets.destinations import (
|
||||
OTEL_V2_DESTINATION_CALLBACKS,
|
||||
build_destination,
|
||||
)
|
||||
|
|
@ -36,10 +36,11 @@ def test_resolves_plain_values_from_metadata():
|
|||
assert params.get("langfuse_host") == "https://test.langfuse.com"
|
||||
|
||||
|
||||
def test_otel_destinations_read_from_top_level_only():
|
||||
"""The admin-resolved OTEL destinations are carried server-side on top-level
|
||||
kwargs (the proxy strips any client value first). They must surface on the
|
||||
dynamic params so the v2 logger can fan out to them."""
|
||||
def test_otel_destinations_read_from_litellm_metadata_only():
|
||||
"""The admin-resolved OTEL destinations are carried server-side under
|
||||
``litellm_metadata`` (a known internal key, scrubbed from the provider body) --
|
||||
NOT as a top-level kwarg, since an unknown top-level key would leak to providers.
|
||||
They must surface on the dynamic params so the v2 logger can fan out to them."""
|
||||
destinations = [
|
||||
{
|
||||
"callback_name": "langfuse_otel",
|
||||
|
|
@ -49,12 +50,23 @@ def test_otel_destinations_read_from_top_level_only():
|
|||
]
|
||||
|
||||
params = initialize_standard_callback_dynamic_params(
|
||||
{"otel_destinations": destinations}
|
||||
{"litellm_metadata": {"otel_destinations": destinations}}
|
||||
)
|
||||
|
||||
assert params.get("otel_destinations") == destinations
|
||||
|
||||
|
||||
def test_otel_destinations_top_level_kwarg_is_ignored():
|
||||
"""A top-level ``otel_destinations`` kwarg is intentionally NOT read. The proxy
|
||||
stashes admin-resolved destinations under ``litellm_metadata`` to keep unknown
|
||||
keys out of the body forwarded to the provider; reading the top-level key would
|
||||
re-open that surface and is therefore ignored."""
|
||||
params = initialize_standard_callback_dynamic_params(
|
||||
{"otel_destinations": [{"callback_name": "langfuse_otel"}]}
|
||||
)
|
||||
assert params.get("otel_destinations") is None
|
||||
|
||||
|
||||
def test_otel_destinations_never_read_from_request_metadata():
|
||||
"""A request body/metadata must not be able to inject OTEL destinations:
|
||||
otel_destinations is deliberately absent from the request-read whitelist, so a
|
||||
|
|
|
|||
|
|
@ -4933,11 +4933,12 @@ async def test_apply_admin_logging_exporters_stamps_and_activates(
|
|||
data: dict = {}
|
||||
await _apply_admin_logging_exporters(data, _auth(team_exporters=["langfuse-eu"]))
|
||||
|
||||
assert data["otel_destinations"][0]["callback_name"] == "langfuse_otel"
|
||||
assert (
|
||||
data["otel_destinations"][0]["endpoint"]
|
||||
== "https://cloud.langfuse.com/api/public/otel"
|
||||
)
|
||||
# destinations live under litellm_metadata so the body does not leak an
|
||||
# unknown top-level key to the provider; the top-level key stays absent
|
||||
assert "otel_destinations" not in data
|
||||
destinations = data["litellm_metadata"]["otel_destinations"]
|
||||
assert destinations[0]["callback_name"] == "langfuse_otel"
|
||||
assert destinations[0]["endpoint"] == "https://cloud.langfuse.com/api/public/otel"
|
||||
# the backend is activated for the request
|
||||
assert "langfuse_otel" in data["success_callback"]
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue