Self-referential attribute value crashes the instrumented application with RecursionError - #5565
Open
dwin-gharibi wants to merge 4 commits into
Open
Conversation
`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.
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.
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.
Pull request dashboard statusWaiting on the author · refreshed 2026-08-24 13:18 UTC Respond to 1 review item (e.g. link a commit, explain why not, ask a follow-up):
Status above doesn't look right?
|
| # 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 |
Contributor
There was a problem hiding this comment.
100 seems too deep. Maybe we should cap this at like 20 ? @xrmx WDYT ?
DylanRussell
approved these changes
Aug 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #5564.
Description
_clean_attribute_valuewalks nestedSequenceandMappingattribute values recursively with no cycle detection and no depth bound. A self-referential list or dict recurses until the interpreter stack is exhausted, and the resultingRecursionErrorpropagates out of the public telemetry API into the calling application.This became reachable when
AnyValuewas widened to accept arbitrarily nested values. Previously such an object would simply have been stringified.Root cause
opentelemetry-api/src/opentelemetry/attributes/__init__.py: theSequencebranch at line 65 and theMappingbranch at line 87 recurse unconditionally. Recursion is bounded only bysys.getrecursionlimit().Approach
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 of empty nesting.
Rejection is deliberately scoped to a single attribute value. A new
_clean_attributeshandles the attributes container, cleaning each value independently so one bad entry becomesNonewithout discarding its siblings. Key validation moves into a shared_clean_key.Measurement.__post_init__previously passed the whole attributes mapping through the single-value path, so a rejected value would have dropped every attribute on the measurement. It now uses_clean_attributes, matchingBoundedAttributes.Files changed
opentelemetry-api/src/opentelemetry/attributes/__init__.pyopentelemetry-sdk/src/opentelemetry/sdk/metrics/_internal/measurement.pyopentelemetry-api/tests/attributes/test_attributes.pyopentelemetry-sdk/tests/test_attribute_nesting_depth.py.changelog/5562.fixedTesting
Unit coverage in
opentelemetry-apifor cyclic sequences, cyclic mappings, excessive depth, and - importantly - that nesting within the limit is still cleaned normally, so the guard cannot silently swallow legitimate data.Integration coverage in
opentelemetry-sdkexercises all three signals plusResource.create, and asserts that a sibling attribute recorded alongside a rejected one survives. All seven fail withRecursionErrorbefore the change; the sibling-preservation test is what caught theMeasurementcontainer bug.Result: 279 passed in
opentelemetry-api(273 baseline plus 6 new), 862 passed inopentelemetry-sdk(855 baseline plus 7 new).Risk / compatibility
Behavioural change only for values that previously crashed. The 100 level limit is far above any realistic attribute: OTLP consumers and storage backends are not designed for deeply nested attribute values, and the semantic conventions do not define any. A rejected value is logged at WARNING so it is diagnosable rather than silent.