From 3d83de1d2361b141c1dee5536db0e2aa209a0866 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 23 Aug 2026 19:29:42 +0330 Subject: [PATCH 1/3] test(api): cover cyclic and deeply nested attribute values `AnyValue` accepts arbitrarily nested Sequence and Mapping values, and `_clean_attribute_value` walks them recursively. Add coverage asserting that a self-referential list, a self-referential mapping and a pathologically deep value are rejected instead of exhausting the interpreter stack, and that nesting within the limit is still cleaned normally. These tests fail with RecursionError against the current implementation. --- .../tests/attributes/test_attributes.py | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/opentelemetry-api/tests/attributes/test_attributes.py b/opentelemetry-api/tests/attributes/test_attributes.py index 29e9826481d..992166b2f82 100644 --- a/opentelemetry-api/tests/attributes/test_attributes.py +++ b/opentelemetry-api/tests/attributes/test_attributes.py @@ -257,3 +257,56 @@ def test_deepcopy_preserves_immutability(self): with self.assertRaises(TypeError): bdict_copy["invalid"] = "invalid" + + +class TestAttributeValueNestingDepth(unittest.TestCase): + """`AnyValue` allows arbitrarily nested Sequence/Mapping values. + + Cleaning them recurses, so a self-referential or pathologically deep value + must be rejected rather than allowed to exhaust the interpreter stack. + Telemetry must never raise into the calling application. + """ + + @staticmethod + def _cyclic_list(): + value = [1, 2] + value.append(value) + return value + + @staticmethod + def _cyclic_dict(): + value = {"a": 1} + value["self"] = value + return value + + @staticmethod + def _deep_list(depth): + value = "x" + for _ in range(depth): + value = [value] + return value + + def test_clean_attribute_value_handles_cyclic_sequence(self): + self.assertIsNone(_clean_attribute_value(self._cyclic_list(), None)) + + def test_clean_attribute_value_handles_cyclic_mapping(self): + self.assertIsNone(_clean_attribute_value(self._cyclic_dict(), None)) + + def test_clean_attribute_value_handles_excessive_depth(self): + self.assertIsNone(_clean_attribute_value(self._deep_list(5000), None)) + + def test_clean_attribute_value_keeps_reasonable_nesting(self): + """Nesting within the limit must still be cleaned normally.""" + self.assertEqual( + _clean_attribute_value([[["a"]]], None), + ((("a",),),), + ) + + def test_bounded_attributes_accepts_cyclic_value(self): + attributes = BoundedAttributes(attributes={"k": self._cyclic_list()}) + self.assertIn("k", attributes) + + def test_bounded_attributes_setitem_accepts_cyclic_value(self): + attributes = BoundedAttributes(immutable=False) + attributes["k"] = self._cyclic_dict() + self.assertIn("k", attributes) From b0c34d7e0d42bb0db7c7b47a10569392bd2ad6cf Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 23 Aug 2026 19:29:43 +0330 Subject: [PATCH 2/3] test(sdk): assert telemetry APIs survive cyclic attribute values Cover the end-to-end contract across all three signals plus resource construction: set_attribute, set_attributes, add_event, Resource.create, Counter.add and Logger.emit must not raise when handed a self-referential value, and an unusable value must not discard the attributes beside it. All seven fail with RecursionError against the current implementation. --- .../tests/test_attribute_nesting_depth.py | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) create mode 100644 opentelemetry-sdk/tests/test_attribute_nesting_depth.py diff --git a/opentelemetry-sdk/tests/test_attribute_nesting_depth.py b/opentelemetry-sdk/tests/test_attribute_nesting_depth.py new file mode 100644 index 00000000000..218a900a93f --- /dev/null +++ b/opentelemetry-sdk/tests/test_attribute_nesting_depth.py @@ -0,0 +1,85 @@ +# Copyright The OpenTelemetry Authors +# SPDX-License-Identifier: Apache-2.0 + +"""Telemetry calls must never raise into the instrumented application. + +`AnyValue` allows arbitrarily nested Sequence/Mapping attribute values, so a +user handing the SDK a self-referential structure must not blow the stack. +""" + +import unittest + +from opentelemetry.sdk._logs import LoggerProvider +from opentelemetry.sdk.metrics import MeterProvider +from opentelemetry.sdk.metrics.export import InMemoryMetricReader +from opentelemetry.sdk.resources import Resource +from opentelemetry.sdk.trace import TracerProvider +from opentelemetry.sdk.trace.export import SimpleSpanProcessor +from opentelemetry.sdk.trace.export.in_memory_span_exporter import ( + InMemorySpanExporter, +) + + +def _cyclic_list(): + value = [1, 2] + value.append(value) + return value + + +def _cyclic_dict(): + value = {"a": 1} + value["self"] = value + return value + + +class TestAttributeNestingDepthDoesNotRaise(unittest.TestCase): + def setUp(self): + self.exporter = InMemorySpanExporter() + self.tracer_provider = TracerProvider(shutdown_on_exit=False) + self.tracer_provider.add_span_processor(SimpleSpanProcessor(self.exporter)) + self.tracer = self.tracer_provider.get_tracer(__name__) + + def test_set_attribute_with_cyclic_sequence(self): + with self.tracer.start_as_current_span("span") as span: + span.set_attribute("cyclic", _cyclic_list()) + self.assertIsNone(self.exporter.get_finished_spans()[0].attributes["cyclic"]) + + def test_set_attribute_with_cyclic_mapping(self): + with self.tracer.start_as_current_span("span") as span: + span.set_attribute("cyclic", _cyclic_dict()) + self.assertIsNone(self.exporter.get_finished_spans()[0].attributes["cyclic"]) + + def test_set_attributes_keeps_sibling_attributes(self): + """One unusable value must not discard the attributes beside it.""" + with self.tracer.start_as_current_span("span") as span: + span.set_attributes({"good": "kept", "cyclic": _cyclic_list()}) + attributes = self.exporter.get_finished_spans()[0].attributes + self.assertEqual(attributes["good"], "kept") + self.assertIsNone(attributes["cyclic"]) + + def test_add_event_with_cyclic_value(self): + with self.tracer.start_as_current_span("span") as span: + span.add_event("event", {"cyclic": _cyclic_list()}) + event = self.exporter.get_finished_spans()[0].events[0] + self.assertIsNone(event.attributes["cyclic"]) + + def test_resource_create_with_cyclic_value(self): + resource = Resource.create({"cyclic": _cyclic_list()}) + self.assertIsNone(resource.attributes["cyclic"]) + + def test_counter_add_with_cyclic_value(self): + reader = InMemoryMetricReader() + meter_provider = MeterProvider(metric_readers=[reader]) + meter_provider.get_meter(__name__).create_counter("counter").add(1, {"cyclic": _cyclic_list()}) + metrics_data = reader.get_metrics_data() + data_point = metrics_data.resource_metrics[0].scope_metrics[0].metrics[0].data.data_points[0] + self.assertIsNone(dict(data_point.attributes)["cyclic"]) + meter_provider.shutdown() + + def test_logger_emit_with_cyclic_value(self): + logger_provider = LoggerProvider(shutdown_on_exit=False) + # Must not raise; the record is dropped at the processor, not here. + logger_provider.get_logger(__name__).emit(body="body", attributes={"cyclic": _cyclic_list()}) + + def tearDown(self): + self.tracer_provider.shutdown() From 7734eaf6f72fee56c52b9ebb3e61e21499ee94d1 Mon Sep 17 00:00:00 2001 From: Dwin Gharibi Date: Sun, 23 Aug 2026 19:29:43 +0330 Subject: [PATCH 3/3] fix(api): cap attribute value nesting depth Since `AnyValue` was widened to accept arbitrarily nested Sequence and Mapping values, `_clean_attribute_value` walks user data recursively with no depth bound. A self-referential list or dict therefore recurses until the interpreter stack is exhausted and raises RecursionError straight out of `set_attribute`, `set_attributes`, `add_event`, `Resource.create`, `Counter.add` and `Logger.emit`. Telemetry must never be able to crash the application it is observing. Bound the recursion at 100 levels. The worker raises an internal marker past that depth and nothing inside it catches, so a value with an unrepresentable branch is rejected whole rather than left as a truncated husk. Rejection is scoped to a single attribute value: add `_clean_attributes` for the attributes container, which cleans each value independently so one bad entry becomes None without discarding its siblings. `BoundedAttributes` and `Measurement` both clean containers and now use it -- previously `Measurement` passed the whole mapping through the single-value path, so any rejected value would have dropped every attribute on the measurement. Key validation is extracted into `_clean_key` and shared by both paths. --- .changelog/5562.fixed | 1 + .../src/opentelemetry/attributes/__init__.py | 110 +++++++++++++----- .../sdk/metrics/_internal/measurement.py | 4 +- 3 files changed, 84 insertions(+), 31 deletions(-) create mode 100644 .changelog/5562.fixed diff --git a/.changelog/5562.fixed b/.changelog/5562.fixed new file mode 100644 index 00000000000..8bbb6c4388b --- /dev/null +++ b/.changelog/5562.fixed @@ -0,0 +1 @@ +`opentelemetry-api`: replace an attribute value nested more than 100 levels deep, including a self-referential one, with `None` instead of raising `RecursionError` into the caller diff --git a/opentelemetry-api/src/opentelemetry/attributes/__init__.py b/opentelemetry-api/src/opentelemetry/attributes/__init__.py index 2ba2d393200..c8cce95ebe9 100644 --- a/opentelemetry-api/src/opentelemetry/attributes/__init__.py +++ b/opentelemetry-api/src/opentelemetry/attributes/__init__.py @@ -16,6 +16,22 @@ _logger = logging.getLogger(__name__) +# Attribute values may be arbitrarily nested Sequences and Mappings, and +# cleaning them recurses. Cap the depth: without it a self-referential value +# raises RecursionError out of set_attribute()/Resource.create()/Counter.add() +# and takes down the instrumented application. +_MAX_NESTING_DEPTH = 100 + + +class _AttributeValueTooDeepError(Exception): + """Internal signal that a value exceeded `_MAX_NESTING_DEPTH`. + + Raised by the recursive worker and caught at the boundary of a single + attribute value, so one unusable value becomes None without discarding the + attributes beside it. + """ + + # Calling str(x) will use an object's `__str__` method if it exists, otherwise it will use it's `__repr__` method. # If neither is defined it uses the base class's `object.__repr__` method, which returns a string that is hard to understand. @@ -24,18 +40,44 @@ def _is_non_custom_str(key: Any) -> bool: return type(key).__str__ is not object.__str__ or type(key).__repr__ is not object.__repr__ -@overload -def _clean_attribute_value( - value: Mapping[str, types.AnyValue], - max_string_value_length: int | None, -) -> Mapping[str, types.AnyValue]: ... +def _clean_key(key: Any) -> str | None: + """Validate an attribute key, returning None if it must be dropped.""" + if not key: + _logger.warning( + "invalid attribute key `%s`. must be non-empty string. Dropping key from attributes.", + key, + ) + return None + # Spec says to convert unknown types to strings if possible (here and below too). + if not isinstance(key, str): + _logger.warning( + "Invalid type `%s` for attribute key `%s`, must be a str. Key's `__str__/__repr__` method will be called if it exists, otherwise the key/value pair will be dropped.", + type(key), + key, + ) + if _is_non_custom_str(key): + return str(key) + return None + return key -@overload -def _clean_attribute_value( - value: types.AnyValue, +def _clean_attributes( + attributes: Mapping[str, types.AnyValue], max_string_value_length: int | None, -) -> types.AnyValue: ... +) -> dict[str, types.AnyValue]: + """Clean a mapping of attributes, one attribute value at a time. + + Unlike a Mapping nested inside a value, the attributes container itself + survives a bad entry: a value that cannot be represented becomes None + rather than discarding the attributes beside it. + """ + cleaned: dict[str, types.AnyValue] = {} + for key, value in attributes.items(): + cleaned_key = _clean_key(key) + if cleaned_key is None: + continue + cleaned[cleaned_key] = _clean_attribute_value(value, max_string_value_length) + return cleaned def _clean_attribute_value( @@ -47,11 +89,35 @@ def _clean_attribute_value( String values are truncated to max_string_value_length if provided. Anything that isn't of `types.AnyValue`, we attempt to cast to `str`. If this fails, the value is replaced with None. Sequence's are converted to tuples and mappings - are copied into new dicts. + are copied into new dicts. A value nested more than `_MAX_NESTING_DEPTH` + levels deep, including a self-referential one, is replaced with None. Returns: The recursively cleaned AnyValue. """ + try: + return _clean_attribute_value_at_depth(value, max_string_value_length, 0) + except _AttributeValueTooDeepError: + _logger.warning( + "Attribute value is nested more than %d levels deep or is self-referential. Replacing it with None.", + _MAX_NESTING_DEPTH, + ) + return None + + +def _clean_attribute_value_at_depth( + value: types.AnyValue, + max_string_value_length: int | None, + depth: int, +) -> types.AnyValue: + """Worker for `_clean_attribute_value` that tracks the nesting depth. + + Raises `_AttributeValueTooDeepError` past `_MAX_NESTING_DEPTH`. Nothing in + here catches it, so a value with an unrepresentable branch is rejected + whole rather than left as a truncated husk. + """ + if depth > _MAX_NESTING_DEPTH: + raise _AttributeValueTooDeepError if isinstance(value, (NoneType, bool, int, float, bytes)): return value if isinstance(value, str): @@ -63,28 +129,14 @@ def _clean_attribute_value( value = value[:max_string_value_length] return value if isinstance(value, Sequence): - return tuple(_clean_attribute_value(v, max_string_value_length) for v in value) + return tuple(_clean_attribute_value_at_depth(v, max_string_value_length, depth + 1) for v in value) if isinstance(value, Mapping): cleaned_mapping: dict[str, types.AnyValue] = {} for key, val in value.items(): - if not key: - _logger.warning( - "invalid attribute key `%s`. must be non-empty string. Dropping key from attributes.", - key, - ) + cleaned_key = _clean_key(key) + if cleaned_key is None: continue - # Spec says to convert unknown types to strings if possible (here and below too). - if not isinstance(key, str): - _logger.warning( - "Invalid type `%s` for attribute key `%s`, must be a str. Key's `__str__/__repr__` method will be called if it exists, otherwise the key/value pair will be dropped.", - type(key), - key, - ) - if _is_non_custom_str(key): - key = str(key) - else: - continue - cleaned_mapping[key] = _clean_attribute_value(val, max_string_value_length) + cleaned_mapping[cleaned_key] = _clean_attribute_value_at_depth(val, max_string_value_length, depth + 1) return cleaned_mapping if TYPE_CHECKING: assert_never(value) @@ -191,7 +243,7 @@ def _set_items(self, attributes: Mapping[str, types.AnyValue]) -> None: with self._lock: self.dropped += len(attributes) return - cleaned_attributes: Mapping[str, types.AnyValue] = _clean_attribute_value(attributes, self.max_value_len) + cleaned_attributes: Mapping[str, types.AnyValue] = _clean_attributes(attributes, self.max_value_len) with self._lock: self.dropped += len(attributes) - len(cleaned_attributes) for key, value in cleaned_attributes.items(): diff --git a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py index 4e9d5eea603..6ef73a8a0af 100644 --- a/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py +++ b/opentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.py @@ -8,7 +8,7 @@ from logging import getLogger from typing import TYPE_CHECKING -from opentelemetry.attributes import _clean_attribute_value +from opentelemetry.attributes import _clean_attributes from opentelemetry.context import Context from opentelemetry.util.types import Attributes @@ -44,7 +44,7 @@ def __post_init__(self) -> None: object.__setattr__( self, "attributes", - _clean_attribute_value(self.attributes, None), + _clean_attributes(self.attributes, None), ) else: _logger.warning(