From 0376db4268e10bc701a9eb6970b6412b7e543eda Mon Sep 17 00:00:00 2001 From: Yassin Kortam Date: Wed, 29 Jul 2026 17:39:33 -0700 Subject: [PATCH] fix(otel): cap metric attributes so series count does not grow with traffic `GenAIMetricRecorder._common_attributes` dumped the whole `hidden_params` object onto every metric datapoint as one label value. That object is per-request by construction: `response_cost`, `litellm_overhead_time_ms`, `cache_key`, `usage_object` and the provider's `additional_headers` rate-limit counters all move on every call. A unique label value is a new time series, and all six GenAI instruments share those attributes, so one request minted up to six series that would never be written to again That is the steady-state behavior of the feature rather than an abuse case, and it is wrong twice over. Hosted backends bill on series count, so recommending metrics be enabled would have meant a bill proportional to traffic. And a histogram whose every datapoint sits in its own series cannot be aggregated, so the dashboards would have looked populated while answering nothing Both paths now cap their attributes at METRIC_ATTRIBUTE_CEILING, which replaces the failure-only allowlist so the two paths cannot drift. The cap runs before the operator's `otel.attributes` filter, so an operator can narrow it and never widen it back to an unbounded label. Client-supplied and per-request metadata (`requester_metadata`, `spend_logs_metadata`, `user_api_key_end_user_id`, `requester_ip_address`) is metric-ineligible and stays on the span, which already carries it and where cardinality is free. `hidden_params` survives as a label but carries only `model_id` and `api_base`, which are bounded by the router's own deployment list and are the part a per-deployment panel reads Four tests fail against the previous behavior, the load-bearing one being that two requests differing only in per-request fields must land in one series rather than two --- litellm/integrations/otel/README.md | 15 ++ litellm/integrations/otel/plumbing/metrics.py | 106 ++++++++--- .../integrations/otel/test_otel_v2_metrics.py | 171 +++++++++++++++--- 3 files changed, 243 insertions(+), 49 deletions(-) diff --git a/litellm/integrations/otel/README.md b/litellm/integrations/otel/README.md index afd593c078e..b4e51b7cdc8 100644 --- a/litellm/integrations/otel/README.md +++ b/litellm/integrations/otel/README.md @@ -230,6 +230,21 @@ lives in [`plumbing/`](./plumbing): cardinality filter, so an `otel.attributes` list cannot merge the failure series back into the success series. A proxy-gate rejection (auth / rate limit) records nothing, for the same reason it gets no span: no upstream call happened. + Both paths cap their attributes at `METRIC_ATTRIBUTE_CEILING` before the + operator's own `otel.attributes` filter runs, so the filter can narrow the set + but never widen it. The ceiling is what keeps series count bounded by the + deployment's own key/team/user/deployment count instead of by its traffic: a + label value that moves per request mints a time series per request, which both + bills per request on a hosted backend and leaves a histogram that cannot be + aggregated. So client-supplied and per-request metadata (`requester_metadata`, + `spend_logs_metadata`, `user_api_key_end_user_id`, `requester_ip_address`) is + metric-ineligible and stays on the span, where cardinality is free, and the + `hidden_params` label carries only `model_id`, the deployment identity a + per-deployment panel joins on. `api_base` is excluded despite naming the same + deployment, because it is a documented per-call parameter and so is caller-chosen + in SDK use. Because the shared validator accepts every span attribute name, a + filter that names a metric-ineligible one logs a warning once when the filter + resolves rather than silently emitting nothing for it. - [`events.py`](./plumbing/events.py) — GenAI client events. Gated on `enable_events` (`LITELLM_OTEL_INTEGRATION_ENABLE_EVENTS`), a failed LLM call records the semconv `gen_ai.client.operation.exception` log event at severity diff --git a/litellm/integrations/otel/plumbing/metrics.py b/litellm/integrations/otel/plumbing/metrics.py index b160480530a..4d3c39e33d4 100644 --- a/litellm/integrations/otel/plumbing/metrics.py +++ b/litellm/integrations/otel/plumbing/metrics.py @@ -10,11 +10,12 @@ from dataclasses import dataclass from datetime import datetime -from typing import Any, Final, FrozenSet, Mapping, Optional +from typing import Any, Final, FrozenSet, Mapping, Optional, TypeAlias from opentelemetry.metrics import Histogram, Meter import litellm +from litellm._logging import verbose_logger from litellm.integrations.opentelemetry import ( METRIC_METADATA_KEYS, TOKEN_TYPE_ATTRIBUTE, @@ -72,20 +73,29 @@ def create_genai_metrics(meter: Meter) -> GenAIMetrics: ) +# A metric datapoint's attributes. Values are the strings the recorder builds, except +# the request model, which is whatever the caller passed and may be absent. +MetricAttributes: TypeAlias = Mapping[str, "str | None"] + ERROR_TYPE_FALLBACK: Final = "_OTHER" -# The attributes a failure datapoint may carry: what operation failed, and whose -# key/team/user it was. Every one is either a fixed enum or an operator-provisioned -# identifier, so the failure series count is bounded by the deployment's own -# key/team/user count. Deliberately excluded is everything the *client* supplies or -# that varies per request -- ``metadata.requester_metadata``, -# ``metadata.spend_logs_metadata``, ``metadata.user_api_key_end_user_id``, -# ``metadata.requester_ip_address`` and the ``hidden_params`` blob (which carries the -# provider's per-request response headers). A failed request costs the caller no -# provider spend, so nothing would rate-limit a caller who minted a fresh series per -# request out of a field they control. ``metadata.user_api_key_user_email`` is left -# out too: it is bounded, but it is PII duplicating the user id already here. -FAILURE_ATTRIBUTE_KEYS: Final[frozenset[str]] = frozenset( +# Every attribute a metric datapoint may carry, on either path. A label value that +# is unique per request is a new time series that will never be written to again, so +# this set is what keeps the series count bounded by the deployment's own +# key/team/user/deployment count rather than by its traffic. Each entry is a fixed +# enum or an operator-provisioned identifier. +# +# Deliberately excluded is everything the *client* supplies or that moves per +# request: ``metadata.requester_metadata`` and ``metadata.spend_logs_metadata`` (both +# free-form from the request body), ``metadata.user_api_key_end_user_id`` (the body's +# ``user`` field), and ``metadata.requester_ip_address``. Those stay on the span, +# where cardinality is free and where they already are. +# ``metadata.user_api_key_user_email`` is left out too: it is bounded, but it is PII +# duplicating the user id already here. +# +# This is a CEILING, applied before the operator's own include/exclude filter, so an +# operator can narrow it but never widen it back to an unbounded attribute. +METRIC_ATTRIBUTE_CEILING: Final[frozenset[str]] = frozenset( ( "gen_ai.operation.name", "gen_ai.system", @@ -97,9 +107,23 @@ def create_genai_metrics(meter: Meter) -> GenAIMetrics: "metadata.user_api_key_team_alias", "metadata.user_api_key_org_id", "metadata.user_api_key_user_id", + "hidden_params", ) ) +# The only ``hidden_params`` field that becomes part of the ``hidden_params`` label. +# The object as a whole is per-request by construction -- ``response_cost``, +# ``litellm_overhead_time_ms``, ``cache_key``, ``usage_object`` and the provider's +# ``additional_headers`` rate-limit counters all move on every call -- so dumping it +# whole made one series per request out of every instrument. +# +# ``model_id`` is the router's own deployment id, so it is bounded by the deployment +# list and is what a per-deployment panel joins on. ``api_base`` is deliberately NOT +# here even though it names the same thing: it is a documented per-call parameter, so +# in SDK use it is chosen by the caller rather than provisioned by the operator, and a +# caller varying it would put the per-request cardinality straight back. +BOUNDED_HIDDEN_PARAM_KEYS: Final[tuple[str, ...]] = ("model_id",) + def resolve_error_type(kwargs: Mapping[str, Any]) -> str: """The ``error.type`` value for a failed request. @@ -147,7 +171,7 @@ def record( start_time: datetime, end_time: datetime, ) -> None: - common_attrs = self._filter_attributes(self._common_attributes(kwargs)) + common_attrs = self._filter_attributes(self._bounded_attributes(kwargs)) duration_s = (end_time - start_time).total_seconds() self._metrics.operation_duration.record(duration_s, attributes=common_attrs) @@ -177,19 +201,18 @@ def record_failure( zeroes ``response_cost`` on failure. Recording them anyway would put a fabricated zero into series that dashboards average. - The attribute set is the bounded ``FAILURE_ATTRIBUTE_KEYS`` allowlist, not - the success path's full set: a failure needs no provider spend, so a caller - who can put a unique value into a client-supplied attribute could mint one - histogram series per request for free. The operator's own filter still - applies on top, so it can narrow this further but never widen it. + The attribute set is :data:`METRIC_ATTRIBUTE_CEILING`, the same cap the + success path uses. A failure needs no provider spend, so a caller who can put + a unique value into a client-supplied attribute could mint one histogram + series per request for free; the cap is what makes that impossible on either + path. ``error.type`` is stamped after both filters, exactly like ``gen_ai.token.type``, so an operator's include/exclude list cannot strip the discriminator and silently merge failures back into the success series. """ - bounded = {k: v for k, v in self._common_attributes(kwargs).items() if k in FAILURE_ATTRIBUTE_KEYS} attributes = { - **self._filter_attributes(bounded), + **self._filter_attributes(self._bounded_attributes(kwargs)), Error.TYPE: resolve_error_type(kwargs), } self._metrics.operation_duration.record((end_time - start_time).total_seconds(), attributes=attributes) @@ -220,11 +243,25 @@ def _common_attributes(self, kwargs: Mapping[str, Any]) -> dict: common_attrs[f"metadata.{key}"] = str(value) hidden_params = getattr(std_log, "hidden_params", None) or (std_log or {}).get("hidden_params", {}) - if hidden_params: - common_attrs["hidden_params"] = safe_dumps(hidden_params) + bounded_hidden_params = { + key: hidden_params[key] + for key in BOUNDED_HIDDEN_PARAM_KEYS + if isinstance(hidden_params, Mapping) and hidden_params.get(key) is not None + } + if bounded_hidden_params: + common_attrs["hidden_params"] = safe_dumps(bounded_hidden_params) return common_attrs + def _bounded_attributes(self, kwargs: Mapping[str, Any]) -> MetricAttributes: + """The datapoint attributes, capped at :data:`METRIC_ATTRIBUTE_CEILING`. + + The cap runs BEFORE the operator's include/exclude filter so the filter can + only narrow it. An operator who names an excluded attribute in an include + list gets nothing for it rather than reintroducing an unbounded label. + """ + return {k: v for k, v in self._common_attributes(kwargs).items() if k in METRIC_ATTRIBUTE_CEILING} + def _ensure_filter(self) -> None: if self._filter_resolved: return @@ -241,8 +278,29 @@ def _ensure_filter(self) -> None: # without reconstructing the recorder. self._include, self._exclude = _resolve_metric_attribute_filter(attributes) self._filter_resolved = True + self._warn_about_metric_ineligible_names() - def _filter_attributes(self, attrs: dict) -> dict: + def _warn_about_metric_ineligible_names(self) -> None: + """Say so when the operator's filter names an attribute the ceiling removes. + + The shared validator accepts every span attribute name, so a name that is + legal on a span but metric-ineligible would otherwise be a silent no-op: an + ``include_list`` naming it emits nothing for it and an ``exclude_list`` naming + it looks like it worked. Logged once, when the filter resolves, rather than + per request. + """ + named = (self._include or frozenset()) | (self._exclude or frozenset()) + ineligible = sorted(named - METRIC_ATTRIBUTE_CEILING - {TOKEN_TYPE_ATTRIBUTE}) + if ineligible: + verbose_logger.warning( + "OTel metrics: %s cannot be a metric attribute and is being ignored; it varies " + "per request or is client-supplied, so it would make one time series per request. " + "It is still on the span. Metric attributes are limited to: %s", + ", ".join(ineligible), + ", ".join(sorted(METRIC_ATTRIBUTE_CEILING)), + ) + + def _filter_attributes(self, attrs: MetricAttributes) -> MetricAttributes: self._ensure_filter() if self._include is not None: return {k: v for k, v in attrs.items() if k in self._include} diff --git a/tests/test_litellm/integrations/otel/test_otel_v2_metrics.py b/tests/test_litellm/integrations/otel/test_otel_v2_metrics.py index d0974fa12f1..56607414de4 100644 --- a/tests/test_litellm/integrations/otel/test_otel_v2_metrics.py +++ b/tests/test_litellm/integrations/otel/test_otel_v2_metrics.py @@ -22,6 +22,7 @@ """ import asyncio +import json from datetime import datetime, timedelta import pytest @@ -68,14 +69,13 @@ TOKEN_TYPE = "gen_ai.token.type" MODEL_KEY = "gen_ai.request.model" -# Each is a member of VALID_METRIC_ATTRIBUTE_NAMES and is stamped on the metric -# by default (proven by the no-filter test below). -HIGH_CARDINALITY_KEYS = ( +# Keys inside the ceiling that an operator's filter must still be able to remove. +# Every one is bounded, so it survives the ceiling and only the operator's own +# exclude_list takes it off; that is what makes the filter tests non-vacuous. +FILTERABLE_KEYS = ( "hidden_params", "metadata.user_api_key_hash", - "metadata.requester_ip_address", - "metadata.requester_metadata", - "metadata.applied_guardrails", + "metadata.user_api_key_team_id", ) PROMPT_TOKENS = 137 @@ -103,6 +103,7 @@ def _build_call(stream: bool = True): "standard_logging_object": { "metadata": { "user_api_key_hash": "hash-abc123", + "user_api_key_team_id": "team-1", "requester_ip_address": "10.0.0.7", "requester_metadata": {"team": "alpha", "tier": "gold"}, "applied_guardrails": ["pii", "toxicity"], @@ -215,14 +216,14 @@ def test_metrics_off_by_default_records_nothing(): def test_exclude_list_strips_high_cardinality_across_metrics(): - """exclude_list set AFTER construction (the proxy path) removes every - high-cardinality key from more than one metric while the low-cardinality - model attribute survives.""" + """exclude_list set AFTER construction (the proxy path) removes every listed + key from more than one metric while the low-cardinality model attribute + survives.""" metrics = _drive_success( InMemoryMetricReader(), - callback_settings_attributes={"exclude_list": list(HIGH_CARDINALITY_KEYS)}, + callback_settings_attributes={"exclude_list": list(FILTERABLE_KEYS)}, ) - excluded = set(HIGH_CARDINALITY_KEYS) + excluded = set(FILTERABLE_KEYS) for name in (OPERATION_DURATION, TOKEN_USAGE): points = metrics[name] @@ -251,18 +252,132 @@ def test_include_list_allows_only_listed_attributes(): assert set(dp.attributes.keys()) - {TOKEN_TYPE} == allowed -def test_no_filter_keeps_high_cardinality_keys(): - """Backward compatibility: without an attributes config every high-cardinality - key the call carries is still stamped, so the filter tests above prove a real - removal rather than a key that was never present.""" +def test_no_filter_still_keeps_the_filterable_keys(): + """Without an attributes config every key the filter tests remove is present, + so those tests prove a real removal rather than a key that was never there.""" metrics = _drive_success(InMemoryMetricReader()) - expected = set(HIGH_CARDINALITY_KEYS) + expected = set(FILTERABLE_KEYS) for name in (OPERATION_DURATION, TOKEN_USAGE): for dp in metrics[name]: assert expected.issubset(set(dp.attributes.keys())) +def test_a_metric_ineligible_filter_name_is_reported_not_silently_dropped(caplog): + """Naming a metric-ineligible attribute in a filter has to say so. + + The shared validator accepts every span attribute name, so an operator can put + one in an ``include_list``, get nothing for it, and have no way to tell that from + a value that happened to be absent. The ceiling is deliberate, but silent is what + makes it a support ticket. + """ + with caplog.at_level("WARNING"): + _drive_success( + InMemoryMetricReader(), + callback_settings_attributes={ + "include_list": [MODEL_KEY, "metadata.requester_ip_address"] + }, + ) + + reported = [ + r.getMessage().split(" cannot be a metric attribute")[0].removeprefix("OTel metrics: ") + for r in caplog.records + if r.levelname == "WARNING" and "cannot be a metric attribute" in r.getMessage() + ] + assert reported == ["metadata.requester_ip_address"], reported + + +def test_two_calls_differing_only_per_request_share_one_series(): + """The whole point of the ceiling: metric cardinality must not grow with traffic. + + Every field here moves on every real request -- the response cost, the call id, + the cache key, the provider's remaining-rate-limit headers -- and each one used + to reach the datapoint inside a single ``hidden_params`` label. A unique label + value is a new time series, so each of the six instruments minted one series per + request, which is both a Grafana Cloud bill proportional to traffic and a + histogram that cannot be aggregated. Identical attribute sets is what "one + series" means to the SDK. + """ + reader = InMemoryMetricReader() + logger = _logger(reader, enable_metrics=True) + + for index, cost in enumerate((RESPONSE_COST, RESPONSE_COST * 3)): + kwargs, response_obj, start, end = _build_call() + kwargs["response_cost"] = cost + kwargs["standard_logging_object"]["hidden_params"] = { + "model_id": "m-1", + # A documented per-call parameter, so it varies here on purpose: the same + # deployment reached under a caller-chosen base must not split the series. + "api_base": f"https://proxy-{index}.example.com/v1", + "litellm_call_id": f"call-{index}", + "cache_key": f"cache-{index}", + "response_cost": cost, + "litellm_overhead_time_ms": 1.5 + index, + "usage_object": {"prompt_tokens": index, "completion_tokens": index}, + "additional_headers": {"x_ratelimit_remaining_requests": 100 - index}, + } + asyncio.run(logger.async_log_success_event(kwargs, response_obj, start, end)) + + for name in ALL_METRICS: + attribute_sets = { + tuple(sorted((k, v) for k, v in dp.attributes.items() if k != TOKEN_TYPE)) + for dp in _metrics_by_name(reader)[name] + } + assert len(attribute_sets) == 1, f"{name} split into {len(attribute_sets)} series across 2 requests" + + +def test_hidden_params_label_carries_only_bounded_deployment_fields(): + """``hidden_params`` survives the ceiling, but only as the deployment identity. + + ``model_id`` is the router's deployment id, bounded by the deployment list, and is + what a per-deployment dashboard reads. Everything else in the object is + per-request or caller-chosen and belongs on the span, which already carries it. + ``api_base`` is excluded despite naming the same deployment: it is a documented + per-call parameter, so a caller varying it would restore the per-request + cardinality this cap exists to remove. + """ + kwargs, response_obj, start, end = _build_call() + kwargs["standard_logging_object"]["hidden_params"] = { + "model_id": "m-1", + "api_base": "https://api.openai.com/v1", + "litellm_call_id": "abc", + "cache_key": "ck-1", + "response_cost": RESPONSE_COST, + } + reader = InMemoryMetricReader() + logger = _logger(reader, enable_metrics=True) + asyncio.run(logger.async_log_success_event(kwargs, response_obj, start, end)) + + label = _metrics_by_name(reader)[OPERATION_DURATION][0].attributes["hidden_params"] + assert json.loads(label) == {"model_id": "m-1"} + + +def test_success_attributes_are_capped_at_the_ceiling(): + """The success path carries exactly the ceiling, no client-supplied attributes. + + The fixture deliberately sets every excluded key, so this asserts a real removal + rather than keys that were never present. + """ + kwargs, response_obj, start, end = _build_call() + metadata = kwargs["standard_logging_object"]["metadata"] + metadata.update( + { + "spend_logs_metadata": {"cost_center": "abc"}, + "user_api_key_end_user_id": "end-user-1", + "user_api_key_user_email": "someone@example.com", + } + ) + reader = InMemoryMetricReader() + logger = _logger(reader, enable_metrics=True) + asyncio.run(logger.async_log_success_event(kwargs, response_obj, start, end)) + metrics = _metrics_by_name(reader) + + for name in ALL_METRICS: + for dp in metrics[name]: + leaked = set(dp.attributes) - set(BOUNDED_KEYS) - {TOKEN_TYPE} + assert not leaked, f"{name} leaked {leaked}" + + def test_metrics_reach_operator_configured_global_provider(monkeypatch): """Regression: with no meter provider injected, the six gen_ai.client.* histograms must record through the operator's globally configured @@ -353,8 +468,7 @@ def test_token_type_rejected_from_either_list(attributes, monkeypatch): # caller (so a caller could mint a fresh series per request, and a failure costs # them no provider spend) or varies per request, or is PII duplicating an id that # is already on the series. -UNBOUNDED_FAILURE_KEYS = ( - "hidden_params", +UNBOUNDED_KEYS = ( "metadata.requester_metadata", "metadata.requester_ip_address", "metadata.spend_logs_metadata", @@ -362,9 +476,10 @@ def test_token_type_rejected_from_either_list(attributes, monkeypatch): "metadata.user_api_key_user_email", ) -# The exact set a failure datapoint may carry: the operation, plus -# operator-provisioned identity. -BOUNDED_FAILURE_KEYS = ( +# The exact set a datapoint may carry on either path: the operation, the +# operator-provisioned identity, and the deployment that served it. +BOUNDED_KEYS = ( + "hidden_params", "gen_ai.operation.name", "gen_ai.system", "gen_ai.request.model", @@ -413,7 +528,11 @@ def _build_failure( "requester_metadata": {"trace": "caller-supplied-unique-value"}, "spend_logs_metadata": {"ticket": "caller-supplied-unique-value"}, }, - "hidden_params": {"litellm_call_id": "abc"}, + "hidden_params": { + "litellm_call_id": "abc", + "model_id": "m-1", + "api_base": "https://api.openai.com/v1", + }, } if error_information is not None: standard_logging_object["error_information"] = error_information @@ -519,11 +638,13 @@ def test_failure_attributes_are_a_bounded_allowlist(): succeeded = next(dp for dp in points if ERROR_TYPE not in dp.attributes) failed = next(dp for dp in points if ERROR_TYPE in dp.attributes) - missing = set(UNBOUNDED_FAILURE_KEYS) - set(succeeded.attributes) + supplied = set(kwargs["standard_logging_object"]["metadata"]) + missing = {key for key in UNBOUNDED_KEYS if key.removeprefix("metadata.") not in supplied} assert not missing, f"fixture never carried {missing}, so the exclusion below proves nothing" - leaked = set(UNBOUNDED_FAILURE_KEYS) & set(failed.attributes) + leaked = set(UNBOUNDED_KEYS) & set(failed.attributes) assert not leaked, f"failure datapoint leaked unbounded attributes: {leaked}" - assert set(failed.attributes) == set(BOUNDED_FAILURE_KEYS) | {ERROR_TYPE} + assert set(failed.attributes) == set(BOUNDED_KEYS) | {ERROR_TYPE} + assert json.loads(failed.attributes["hidden_params"]) == {"model_id": "m-1"} def test_operator_filter_can_still_narrow_the_failure_allowlist():