Skip to content

sdk: consolidate attribute normalization in attrnorm - #8611

Merged
pellared merged 5 commits into
open-telemetry:mainfrom
pellared:agent/consolidate-attribute-normalization
Jul 23, 2026
Merged

sdk: consolidate attribute normalization in attrnorm#8611
pellared merged 5 commits into
open-telemetry:mainfrom
pellared:agent/consolidate-attribute-normalization

Conversation

@pellared

@pellared pellared commented Jul 15, 2026

Copy link
Copy Markdown
Member

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 Optimize truncation in Trace and Logs SDK #8491 as well as the attribute value depth limit prototyped in [Prototype] sdk: limit depth of nested attribute values #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.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pellared
pellared requested a review from Copilot July 15, 2026 07:01

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@pellared
pellared marked this pull request as draft July 15, 2026 10:52
@pellared pellared added the Skip Changelog PRs that do not require a CHANGELOG.md entry label Jul 15, 2026
@codecov

codecov Bot commented Jul 15, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.0%. Comparing base (0f58da7) to head (28a2813).

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff          @@
##            main   #8611   +/-   ##
=====================================
  Coverage   84.0%   84.0%           
=====================================
  Files        326     328    +2     
  Lines      25942   25979   +37     
=====================================
+ Hits       21799   21838   +39     
+ Misses      3761    3760    -1     
+ Partials     382     381    -1     
Files with missing lines Coverage Δ
sdk/internal/attrnorm/dedup.go 100.0% <ø> (ø)
sdk/internal/attrnorm/truncate.go 100.0% <100.0%> (ø)
sdk/log/internal/attrnorm/dedup.go 100.0% <ø> (ø)
sdk/log/internal/attrnorm/truncate.go 100.0% <100.0%> (ø)
sdk/log/provider.go 97.8% <100.0%> (ø)
sdk/log/record.go 100.0% <100.0%> (ø)
sdk/metric/instrument.go 98.1% <100.0%> (ø)
sdk/metric/internal/attrnorm/dedup.go 100.0% <ø> (ø)
sdk/metric/provider.go 100.0% <100.0%> (ø)
sdk/resource/resource.go 86.7% <100.0%> (ø)
... and 2 more
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@pellared pellared changed the title [sdk/internal] Consolidate attribute normalization in attrnorm sdk: consolidate attribute normalization in attrnorm Jul 15, 2026
@pellared
pellared marked this pull request as ready for review July 15, 2026 12:13
@pellared

Copy link
Copy Markdown
Member Author

@ps-mir, PTAL

@ps-mir ps-mir left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more satisfied with this change compared to initial plans. Despite the size of the PR, its a simplified refactoring, which moves truncation logic in shared internal template package and repurpose it as attrnorm.

Keeping truncation logic in a separate file is good.

There are no logical path changes apart from 1 small conditional logic as a result of unifying value truncation.

A nit on package comment, but nothing blocking. LGTM

Comment thread internal/shared/attrnorm/truncate.go.tmpl
Comment thread internal/shared/attrnorm/dedup.go.tmpl
@pellared pellared added this to the v1.45.0 milestone Jul 16, 2026
@pellared pellared moved this from Todo to In Progress in Go: Logs (GA) Jul 16, 2026
@pellared
pellared merged commit 2ff073d into open-telemetry:main Jul 23, 2026
31 checks passed
@pellared
pellared deleted the agent/consolidate-attribute-normalization branch July 23, 2026 21:17
@github-project-automation github-project-automation Bot moved this from In Progress to Done in Go: Logs (GA) Jul 23, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Skip Changelog PRs that do not require a CHANGELOG.md entry

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Move attribute normalization code to attrnorm package

5 participants