From 00195b1788dab2637990770a4b63710b4e59c0f1 Mon Sep 17 00:00:00 2001 From: yucheng Date: Thu, 24 Sep 2026 10:27:55 +0000 Subject: [PATCH] fix(otel v2): compare the effective span scope across callback entries Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com> --- litellm/integrations/otel/plumbing/context.py | 1 - .../callback_config_validation.py | 41 +++++++------------ .../team_callback_endpoints.py | 11 ++--- .../test_otel_tenant_span_scope.py | 17 ++++++++ .../test_callback_config_validation.py | 20 +++++++-- .../test_team_callback_endpoints.py | 41 +++++++++++++++++++ 6 files changed, 93 insertions(+), 38 deletions(-) diff --git a/litellm/integrations/otel/plumbing/context.py b/litellm/integrations/otel/plumbing/context.py index 8dcfcf9b3b6..240528d3515 100644 --- a/litellm/integrations/otel/plumbing/context.py +++ b/litellm/integrations/otel/plumbing/context.py @@ -406,7 +406,6 @@ _SCOPES: Final[tuple[OtelSpanScope, ...]] = get_args(OtelSpanScope) def tenant_span_scope_default() -> OtelSpanScope: - """The ``span_scope`` a tenant destination gets when its own callback vars name none.""" import litellm configured: Final = litellm.otel_tenant_span_scope or os.environ.get(OTEL_TENANT_SPAN_SCOPE_ENV) diff --git a/litellm/proxy/common_utils/callback_config_validation.py b/litellm/proxy/common_utils/callback_config_validation.py index 53e417d3086..435c95f110a 100644 --- a/litellm/proxy/common_utils/callback_config_validation.py +++ b/litellm/proxy/common_utils/callback_config_validation.py @@ -106,11 +106,6 @@ def _otel_span_scope_error(callback_name: str | None, callback_vars: Mapping[str def _alias_conflict_error(callback_vars: Mapping[str, str]) -> str | None: - """Reject one entry that names both scope aliases with different values. - - ``otel_span_scope`` outranks ``langfuse_span_scope`` when they disagree, so - storing both would export the one and silently drop the other. - """ langfuse_scope: Final = callback_vars.get(_LANGFUSE_SPAN_SCOPE_VAR) otel_scope: Final = callback_vars.get(_OTEL_SPAN_SCOPE_VAR) if langfuse_scope is None or otel_scope is None or langfuse_scope == otel_scope: @@ -211,36 +206,28 @@ def cross_entry_family_error( ) +def _effective_span_scope(callback_name: str | None, callback_vars: Mapping[str, str]) -> str | None: + langfuse_scope: Final = ( + callback_vars.get(_LANGFUSE_SPAN_SCOPE_VAR) if callback_name == _LANGFUSE_OTEL_CALLBACK else None + ) + return callback_vars.get(_OTEL_SPAN_SCOPE_VAR) or langfuse_scope + + def conflicting_span_scope_error( + callback_name: str | None, callback_vars: Mapping[str, str] | None, stored_vars_by_entry: Sequence[Mapping[str, str]], ) -> str | None: - return _conflicting_var_error(_LANGFUSE_SPAN_SCOPE_VAR, callback_vars, stored_vars_by_entry) - - -def conflicting_otel_span_scope_error( - callback_vars: Mapping[str, str] | None, - stored_vars_by_entry: Sequence[Mapping[str, str]], -) -> str | None: - """``stored_vars_by_entry`` must hold only the entries of the same callback: the - request merges the var per backend, so two backends may legitimately disagree.""" - return _conflicting_var_error(_OTEL_SPAN_SCOPE_VAR, callback_vars, stored_vars_by_entry) - - -def _conflicting_var_error( - var: str, - callback_vars: Mapping[str, str] | None, - stored_vars_by_entry: Sequence[Mapping[str, str]], -) -> str | None: - incoming: Final = None if callback_vars is None else callback_vars.get(var) + incoming: Final = None if callback_vars is None else _effective_span_scope(callback_name, callback_vars) if incoming is None: return None return next( ( - f"{var} is already set to {stored!r} by another callback entry. " + f"span scope is already set to {stored!r} by another {callback_name} callback entry " + f"({_LANGFUSE_SPAN_SCOPE_VAR} and {_OTEL_SPAN_SCOPE_VAR} name the same setting). " f"Every entry shares one value: remove that entry or send the same value." for entry in stored_vars_by_entry - if (stored := entry.get(var)) not in (None, incoming) + if (stored := _effective_span_scope(callback_name, entry)) not in (None, incoming) ), None, ) @@ -260,9 +247,9 @@ def logging_metadata_config_error(metadata: Mapping[str, object] | None) -> str error for error in ( *(_logging_entry_error(entry) for entry in entries), - *(conflicting_span_scope_error(entry_vars[i], entry_vars[:i]) for i in range(len(entry_vars))), *( - conflicting_otel_span_scope_error( + conflicting_span_scope_error( + entry_names[i], entry_vars[i], tuple(vars_ for vars_, name in zip(entry_vars[:i], entry_names[:i]) if name == entry_names[i]), ) diff --git a/litellm/proxy/management_endpoints/team_callback_endpoints.py b/litellm/proxy/management_endpoints/team_callback_endpoints.py index ce3c9a58d37..15e0e8bdf68 100644 --- a/litellm/proxy/management_endpoints/team_callback_endpoints.py +++ b/litellm/proxy/management_endpoints/team_callback_endpoints.py @@ -31,7 +31,6 @@ from litellm.proxy._types import ( from litellm.proxy.auth.user_api_key_auth import user_api_key_auth from litellm.proxy.common_utils.callback_config_validation import ( callback_config_error, - conflicting_otel_span_scope_error, conflicting_span_scope_error, cross_entry_family_error, ) @@ -354,10 +353,8 @@ async def add_team_callbacks( stored_entry_vars: Final = [ # mutable-ok: read-only input to the checks, never stored entry.get("callback_vars") or {} for entry in stored_entries ] - scope_error: Final = conflicting_span_scope_error(data.callback_vars, stored_entry_vars) - if scope_error is not None: - raise _callback_config_error(scope_error) - otel_scope_error: Final = conflicting_otel_span_scope_error( + scope_error: Final = conflicting_span_scope_error( + data.callback_name, data.callback_vars, tuple( entry_vars @@ -365,8 +362,8 @@ async def add_team_callbacks( if entry.get("callback_name") == data.callback_name ), ) - if otel_scope_error is not None: - raise _callback_config_error(otel_scope_error) + if scope_error is not None: + raise _callback_config_error(scope_error) # One entry has to own a credential family end to end. The entries are # flattened into one dict before a request reads them, so an entry # naming only a destination would pair with a key written on another diff --git a/tests/integration/observability/test_otel_tenant_span_scope.py b/tests/integration/observability/test_otel_tenant_span_scope.py index bd98f192895..26f8e21a362 100644 --- a/tests/integration/observability/test_otel_tenant_span_scope.py +++ b/tests/integration/observability/test_otel_tenant_span_scope.py @@ -579,6 +579,23 @@ def test_langfuse_alias_and_otel_scope_disagree_rejected(gateway: Gateway, langf assert "span_scope" in response.text, response.text +def test_split_scope_aliases_on_two_entries_rejected(gateway: Gateway, langfuse_vars: dict[str, JsonValue]) -> None: + response: Final = gateway.request( + "POST", + "/team/new", + { + "metadata": { + "logging": [ + *_key_logging_entry({**langfuse_vars, "langfuse_span_scope": "full"}), + *_key_logging_entry({**langfuse_vars, SPAN_SCOPE_VAR: "llm_only"}), + ] + } + }, + ) + assert response.status_code == 400, f"expected 400, got {response.status_code}: {response.text}" + assert "span_scope" in response.text, response.text + + def test_additive_mode_operator_full_tenant_no_internal(gateway: Gateway, audit_sinks: SpanSinks, langfuse_vars: dict[str, JsonValue], otel_audit_config: AuditConfigWriter, tmp_path: Path) -> None: with _candidate(gateway, tmp_path, audit_sinks, otel_audit_config, settings={"otel_tenant_destination_mode": "additive"}) as candidate: with candidate.scenario() as scenario: diff --git a/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py b/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py index 7bedd3de97b..42e6d1becdd 100644 --- a/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py +++ b/tests/test_litellm/proxy/common_utils/test_callback_config_validation.py @@ -2,7 +2,6 @@ import pytest from litellm.proxy.common_utils.callback_config_validation import ( callback_config_error, - conflicting_otel_span_scope_error, conflicting_span_scope_error, cross_entry_family_error, logging_metadata_config_error, @@ -60,7 +59,7 @@ def test_a_bad_span_scope_is_reported_even_when_the_environment_is_fine(): def test_one_span_scope_per_team(new_vars, stored, rejected): """The entries flatten last-wins, so a second scope would export whichever entry was stored last. An entry that names no scope leaves the stored one in charge.""" - error = conflicting_span_scope_error(new_vars, stored) + error = conflicting_span_scope_error("langfuse_otel", new_vars, stored) assert (error is not None) is rejected if rejected: assert "langfuse_span_scope" in error and stored[-1]["langfuse_span_scope"] in error @@ -166,7 +165,22 @@ def test_key_logging_entries_of_one_backend_may_not_disagree_on_span_scope(): ], ) def test_one_span_scope_value_per_backend(new_vars, stored, rejected): - error = conflicting_otel_span_scope_error(new_vars, stored) + error = conflicting_span_scope_error("arize", new_vars, stored) + assert (error is not None) is rejected + + +@pytest.mark.parametrize( + "callback_name, new_vars, stored, rejected", + [ + ("langfuse_otel", {"otel_span_scope": "llm_only"}, [{"langfuse_span_scope": "full"}], True), + ("newrelic", {"otel_span_scope": "llm_only"}, [{"otel_span_scope": "full"}], True), + ("langfuse_otel", {"otel_span_scope": "llm_only"}, [{"langfuse_span_scope": "llm_only"}], False), + ("langfuse_otel", {"otel_span_scope": "llm_only"}, [{"otel_span_scope": "llm_only"}], False), + ("langfuse_otel", {"langfuse_span_scope": "llm_only"}, [{"otel_span_scope": "llm_only"}], False), + ], +) +def test_split_span_scope_aliases_conflict_across_entries(callback_name, new_vars, stored, rejected): + error = conflicting_span_scope_error(callback_name, new_vars, stored) assert (error is not None) is rejected diff --git a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py index e8389611de4..38fae3420a4 100644 --- a/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py +++ b/tests/test_litellm/proxy/management_endpoints/test_team_callback_endpoints.py @@ -1730,6 +1730,47 @@ async def test_a_second_entry_may_not_flip_span_scope_but_another_backend_may(pa assert [entry["callback_vars"]["otel_span_scope"] for entry in saved["logging"]] == ["no_internal", "full"] +@pytest.mark.asyncio +async def test_split_span_scope_aliases_across_entries_are_rejected(patched_prisma): + """langfuse_span_scope and otel_span_scope name the same setting, so a stored + full under one name may not be flipped to llm_only under the other.""" + patched_prisma.get_data = AsyncMock( + return_value=_team_row( + metadata={ + "logging": [ + { + "callback_name": "langfuse_otel", + "callback_type": "success", + "callback_vars": { + "langfuse_public_key": "pk", + "langfuse_secret_key": "sk", + "langfuse_span_scope": "full", + }, + } + ] + } + ) + ) + with pytest.raises(HTTPException) as exc: + await add_team_callbacks( + data=AddTeamCallback( + callback_name="langfuse_otel", + callback_type="failure", + callback_vars={ + "langfuse_public_key": "pk", + "langfuse_secret_key": "sk", + "otel_span_scope": "llm_only", + }, + ), + http_request=Mock(spec=Request), + team_id="team-victim", + user_api_key_dict=_admin_auth(), + ) + assert exc.value.status_code == 400 + assert "span_scope" in str(exc.value.detail) and "'full'" in str(exc.value.detail) + patched_prisma.db.litellm_teamtable.update.assert_not_called() + + @pytest.mark.asyncio async def test_add_team_callbacks_rejects_out_of_range_arize_sampling_rate(patched_prisma): data = AddTeamCallback(