[Prototype] sdk: limit depth of nested attribute values - #8534
Draft
pellared wants to merge 14 commits into
Draft
Conversation
Add SDK attribute value depth limiting for trace, log, and SDK-owned metric attributes. Rename the internal attrdedup helpers to attrnorm and split deduplication from depth limiting with explicit helper names. Apply the default depth limit to trace/log attributes and to metric resource and instrumentation scope attributes, while leaving metric measurement attributes and log record bodies unchanged.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8534 +/- ##
=======================================
+ Coverage 83.9% 84.3% +0.3%
=======================================
Files 323 323
Lines 25804 26469 +665
=======================================
+ Hits 21665 22328 +663
- Misses 3759 3760 +1
- Partials 380 381 +1
🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Introduces attribute value depth limiting across the SDK to constrain deeply nested attribute.Value structures, and refactors internal attribute “dedup” helpers into a more explicit “attrnorm” normalization layer used by trace/log/metric.
Changes:
- Add depth limiting for nested attribute values (default depth 64), applied to trace/log attributes and to metric resource + instrumentation scope attributes (but not metric measurement attrs or log bodies).
- Rename/split internal attribute helpers from
attrdeduptoattrnorm, adding dedicated helpers for dedup vs. depth limiting (and combinations). - Add/adjust tests and generated code to validate depth limiting behavior and option precedence across packages.
Reviewed changes
Copilot reviewed 35 out of 35 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/trace/trace_test.go | Adds trace coverage for depth limiting across span/event/link/scope/resource and option edge cases. |
| sdk/trace/span.go | Applies attr normalization with depth limit when recording span/event/link attributes. |
| sdk/trace/span_limits.go | Adds DefaultAttributeValueDepthLimit and SpanLimits.AttributeValueDepthLimit. |
| sdk/trace/span_limits_test.go | Adds option precedence tests for the new span depth limit. |
| sdk/trace/provider.go | Adds WithAttributeValueDepthLimit, applies depth limiting to scope/resource attrs, and wires depth limit into normalization. |
| sdk/resource/resource.go | Switches resource normalization helper import to attrnorm (dedup-only). |
| sdk/metric/provider.go | Applies depth limiting to meter instrumentation scope attributes; stores depth limit config on provider. |
| sdk/metric/provider_test.go | Adds metric coverage ensuring resource/scope are limited while measurement/unsafe attrs are unchanged. |
| sdk/metric/internal/gen.go | Regenerates metric internal helpers under attrnorm/ instead of attrdedup/. |
| sdk/metric/internal/attrnorm/dedup.go | New generated normalization implementation (dedup + depth limiting helpers). |
| sdk/metric/internal/attrnorm/dedup_test.go | New generated tests for normalization helpers (dedup/depth limiting). |
| sdk/metric/internal/attrdedup/dedup.go | Removes old generated metric attr dedup package. |
| sdk/metric/internal/attrdedup/dedup_test.go | Removes old generated metric attr dedup tests. |
| sdk/metric/instrument.go | Renames metric attribute normalization calls to attrnorm (dedup-only). |
| sdk/metric/config.go | Adds metric provider option/config for attribute value depth limiting and applies it to resource. |
| sdk/log/record.go | Adds record-level depth limit handling for log attributes; keeps log body depth-unlimited. |
| sdk/log/record_test.go | Adds tests for log record depth limiting (including allow-dup-keys) and verifies body is unaffected. |
| sdk/log/provider.go | Adds logger provider depth limit option and applies it to scope/resource/record attributes. |
| sdk/log/provider_test.go | Adds tests for provider depth limit behavior and option precedence. |
| sdk/log/logtest/factory.go | Updates log test factory to disable depth limiting when constructing records with attributes. |
| sdk/log/logger.go | Plumbs provider depth limit settings into newly created SDK log records. |
| sdk/log/internal/gen.go | Regenerates log internal helpers under attrnorm/ instead of attrdedup/. |
| sdk/log/internal/attrnorm/dedup.go | New generated normalization implementation for log internals. |
| sdk/log/internal/attrnorm/dedup_test.go | New generated tests for log internal normalization helpers. |
| sdk/log/internal/attrdedup/dedup.go | Removes old generated log attr dedup package. |
| sdk/log/internal/attrdedup/dedup_test.go | Removes old generated log attr dedup tests. |
| sdk/internal/gen.go | Regenerates SDK internal helpers under attrnorm/ instead of attrdedup/. |
| sdk/internal/attrnorm/dedup.go | New generated normalization implementation for shared SDK internals. |
| sdk/internal/attrnorm/dedup_test.go | New generated tests for shared SDK internal normalization helpers. |
| sdk/internal/attrnorm/dedup_benchmark_test.go | Updates benchmark to target the renamed attrnorm package/API. |
| sdk/internal/attrdedup/dedup.go | Removes old generated shared SDK attr dedup package. |
| sdk/internal/attrdedup/dedup_test.go | Removes old generated shared SDK attr dedup tests. |
| internal/shared/attrdedup/dedup.go.tmpl | Updates shared generator template to produce attrnorm (dedup + depth limiting). |
| internal/shared/attrdedup/dedup_test.go.tmpl | Updates shared generator test template to match the new attrnorm API surface. |
| CHANGELOG.md | Documents new depth-limit configuration options and default depth limiting behavior. |
pellared
changed the title
pellared
marked this pull request as ready for review
July 2, 2026 10:32
pellared
requested review from
MrAlias,
XSAM,
dashpole,
dmathieu and
flc1125
as code owners
July 2, 2026 10:32
pellared
changed the title
pellared
marked this pull request as draft
July 2, 2026 10:45
pellared
added a commit
that referenced
this pull request
Jul 23, 2026
Fixes #8499 ### Motivation - Attribute normalization was split between module-local Trace and Logs truncation implementations and generated `attrdedup` packages used by Trace, Logs, and Metrics. This change establishes module-local `internal/attrnorm` packages generated from shared templates. - This prepares for, but does not implement, the allocation and traversal optimizations proposed in #8491 as well as the attribute value depth limit prototyped in #8534. It deliberately preserves the existing traversal, rebuilding, deduplication, truncation, concurrency, and public API behavior. ### Changes - Move the existing shared deduplication implementation and tests from `internal/shared/attrdedup` to `internal/shared/attrnorm` without changing its behavior. - Move the equivalent Trace and Logs truncation implementations and tests into shared `attrnorm` templates. - Generate deduplication and truncation for `go.opentelemetry.io/otel/sdk` and `go.opentelemetry.io/otel/sdk/log`; generate deduplication only for `go.opentelemetry.io/otel/sdk/metric`. - Preserve the two existing hot-path integration shapes: Trace truncates a complete `attribute.KeyValue`, while Logs truncates its `attribute.Value` and keeps body handling deduplication-only. - Update Trace, Logs, Metrics, and Resource consumers to import their own module-local `internal/attrnorm` package, and remove all obsolete `attrdedup` generation and consumers. - Generate the shared truncation regression tests in both consuming modules, including direct `TruncateValue` coverage for negative limits, scalar strings, truncated map values, and unchanged maps, while retaining module-level integration coverage. - Leave the changelog unchanged because this is an internal, behavior-preserving refactor. ### Benchmarks Focused before/after benchmarks were collected with `-benchmem` and 10 samples, then compared with `benchstat`. - Trace truncation and span-limit paths: allocations and bytes/op are unchanged; timings are mixed with no consistent material regression. - Logs truncation paths: allocations and bytes/op are unchanged. After retaining the original value-level call shape, affected ValueLimit paths are stable or faster (`AddAttributes`: -7.8% hit, -4.2% not-hit; `SetAttributes`: no significant change). - Deduplication: the allocation-free fast path is stable (161.5 ns to 161.3 ns, no significant change), with unchanged allocations and bytes/op for all cases. - Metric attribute paths: allocation counts are unchanged; timings are noisy and mixed despite the deduplication implementation being a rename-only move.
|
Thanks for the context in #8534! I've read through the prototype. It looks like the main blocker was that it was too large to merge as-is. Would it be helpful if I started by extracting just the attrnorm depth-limiting functions (TruncateDepth/TruncateValueDepth) from the prototype into a focused PR, without touching span/log wiring yet? That would be the smallest mergeable unit and unblock the rest of the split. |
trentm
pushed a commit
to trentm/opentelemetry-specification
that referenced
this pull request
Aug 7, 2026
…-telemetry#5186) Towards open-telemetry#4911 Per open-telemetry#4006 (comment) ### Motivation Bound (limit) the processing of nested attribute values. It could also mitigate possible recursive processing of nested attribute values if languages allow it. The default of 64 is intentionally conservative. It matches the .NET `System.Text.Json` default and is the lowest non-unlimited nesting/depth default found in the parsers checked. Exact depth-counting semantics differ between libraries, so this table is used only as rationale for the default value. | Library / parser | Ecosystem | Default nesting/depth limit | Notes | | --- | --- | ---: | --- | | `System.Text.Json` | .NET | 64 | `MaxDepth` default value of `0` means a maximum depth of 64. | | `serde_json` | Rust | 128 | `Deserializer` initializes `remaining_depth` to 128. | | Gson `JsonReader` | Java | 255 | `DEFAULT_NESTING_LIMIT = 255`; docs describe it as protection against recursive stack overflow. | | Moshi `JsonReader` | JVM / Kotlin | 256 | Internal stack grows up to 256 levels, including the top-level document. | | Jackson `StreamReadConstraints` | Java | 1000 | `DEFAULT_MAX_DEPTH = 1000`. | | Go `encoding/json` | Go | 10000 | Internal scanner cap exists to prevent stack overflow. | ### Changes - Add `AttributeValueDepthLimit` to the common attribute limit rules with a default of 64. - Define depth counting explicitly: depth starts at 1 for the top-level attribute value and increments when descending into array elements or map values. - Replace any nested array or map with an empty value to not go beyond the limit. The depth limiting algorithm matches what e.g. `System.Text.Json` would expect if they have the same limit. ### Prototype - open-telemetry/opentelemetry-java#8656 - open-telemetry/opentelemetry-go#8534 ### Declarative config - open-telemetry/opentelemetry-configuration#702 ### Notes - This PR does not add an environment variable for the new limit. - This is updating a stable part of the specification. However, I classify this as a bugfix as having unbounded processing is not desired (https://cwe.mitre.org/data/definitions/789.html). --------- Co-authored-by: Carlos Alberto Cortez <calberto.cortez@gmail.com>
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.
The intention of the PR is mostly for specification review purposes. This PR is too big and would need to be split into a few PRs (and refactored).
Changes
Benchmarks
Analysis
benchstatdid not report meaningful allocation changes. The significant movement is insec/op.The helper benchmark itself is statistically unchanged, which suggests the renamed
ValueDeduppath is equivalent to the oldattrdedup.Valuepath for the cases covered here. The resource merge regressions are small in absolute terms and have no allocation changes; they likely come from code layout or inlining changes around the expanded generated helper rather than a new allocation-heavy path.The trace regressions are expected from applying depth normalization to span, event, link, resource, and instrumentation scope attributes. The extra work is mostly CPU-only scanning: benchmarked bytes and allocation counts were stable. Some sampled span attribute benchmarks improved. Since the change adds checks rather than removing work, these wins are probably code layout or inlining effects and should not be treated as a deliberate optimization.
The largest log regressions are concentrated in
Record.AddAttributesandRecord.SetAttributes. This comes from depth limiting being applied to every attribute value. In theallowDupKeyscases, the old code skipped map deduplication entirely; the new code still needs to check slice/map depth, so scalar-only benchmarks pay a new function-call and type-switch cost.For nested values with duplicate-key handling enabled, the log path currently walks values twice: once for depth limiting and once for deduplication so it can preserve duplicate-key-drop logging behavior. That explains the larger nested regressions.
The metric hot path is mostly unchanged, which matches the intended behavior: measurement attributes are not depth-limited by this PR. The significant filtered 10-attribute rows should be treated as secondary until rerun with a longer benchmark window because most metric rows were unchanged and there were no allocation changes.
Improvement Opportunities
Combine log depth limiting and deduplication into one traversal. The log path currently separates these so it can call
logKeyValuePairDropped()only for duplicate map keys. A helper that returns separate change reasons, for exampledepthChangedanddedupChanged, would preserve that behavior while avoiding a second walk of nested values.Add a scalar fast path before depth normalization. Depth limiting only needs to inspect
attribute.SLICEandattribute.MAPvalues. Checking the value type before callingKeyValueLimitDepth,KeyValuesWithDepthLimit, orSetWithDepthLimitwould reduce overhead for the common scalar-attribute case, especially in logRecord.AddAttributesandRecord.SetAttributes.