Skip to content

sdk/trace: add trace.WithAllowKeyDuplication - #8488

Open
Dipanshusinghh wants to merge 26 commits into
open-telemetry:mainfrom
Dipanshusinghh:feature/allow-key-duplication
Open

sdk/trace: add trace.WithAllowKeyDuplication#8488
Dipanshusinghh wants to merge 26 commits into
open-telemetry:mainfrom
Dipanshusinghh:feature/allow-key-duplication

Conversation

@Dipanshusinghh

@Dipanshusinghh Dipanshusinghh commented Jun 19, 2026

Copy link
Copy Markdown
Contributor

Closes #8480

Adds WithAllowKeyDuplication() to sdk/trace as a TracerProviderOption to opt out of the attribute map key deduplication introduced in #8471.

Instead of checking a boolean flag on every span operation (the approach reverted in 5fa1035), this swaps a function pointer once at TracerProvider construction — keeping the hot path completely branch-free with no per-operation overhead.

⚠️ Warning: Enabling this option allows spans to contain semantically ambiguous duplicate attribute keys. Downstream exporters and collectors may handle these differently. Use only if you have explicitly verified your pipeline supports duplicate keys.


Benchmarks

goos: windows
goarch: amd64
pkg: go.opentelemetry.io/otel/sdk/trace
cpu: AMD Ryzen 5 5500U with Radeon Graphics

1. Baseline Comparison (vs main)

benchstat comparing base commit 7128e65 to this branch — verifying no regression in the default (deduplication enabled) path.

Rows marked ~ indicate no statistically significant change (p > 0.05).
The +7.26% on SpanWithAttributes_4 appears to be noise at n=5 and does not reproduce consistently; happy to rerun at n=20 if needed.

│ sec/op vs base │ StartEndSpan/AlwaysSample-12 1018.0n ~ (p=0.841 n=5) StartEndSpan/NeverSample-12 542.9n ~ (p=0.548 n=5) SpanWithAttributes_4/AlwaysSample-12 1.876µ +7.26% (p=0.008 n=5) SpanWithAttributes_4/NeverSample-12 842.9n ~ (p=0.310 n=5) SpanWithEvents_4/AlwaysSample-12 2.154µ ~ (p=0.079 n=5) SpanWithEvents_4/NeverSample-12 571.5n ~ (p=0.310 n=5) geomean 1.608µ +5.45% ​

B/op and allocs/op: no change across all benchmarks.

The +5.45% geomean is driven entirely by the one marginal SpanWithAttributes_4 result; all other benchmarks are within noise.


2. WithAllowKeyDuplication Speedup

Benchmarks comparing deduplication enabled vs. disabled, with and without duplicate keys.

Operation Deduplication Keys ns/op B/op allocs/op
SetAttributes Enabled With Duplicates 2656 1168 5
SetAttributes Disabled With Duplicates 2391 976 3
SetAttributes Enabled Without Duplicates 2152 848 3
SetAttributes Disabled Without Duplicates 2106 848 3
AddEvent Enabled With Duplicates 3690 1736 9
AddEvent Disabled With Duplicates 2388 1096 6
AddEvent Enabled Without Duplicates 2724 968 6
AddEvent Disabled Without Duplicates 1838 968 6

With duplicate keys (opt-in benefit):
ns/op: −10% (SetAttributes), −35% (AddEvent)
B/op: −16% (SetAttributes), −37% (AddEvent)
allocs/op: −40% (SetAttributes), −33% (AddEvent)

Without duplicate keys (default path):
No change — fast path is completely unaffected.

Add TestSpanAttributeCapacity_WithAllowKeyDuplication to cover the
addOverCapAttrsNoDup code path that is exercised when WithAllowKeyDuplication
is enabled and attributes exceed AttributeCountLimit.

Test cases cover: under/at/over capacity, split calls that cross the limit,
invalid attributes, and a mix of valid-at-capacity followed by invalid attrs.
@Dipanshusinghh Dipanshusinghh changed the title feat(trace): add WithAllowKeyDuplication option sdk/trace: add trace.WithAllowKeyDuplication Jun 19, 2026
@pellared

pellared commented Jun 19, 2026

Copy link
Copy Markdown
Member
  1. Please revert the changes related to code generation (which is not related to the issue this PR is trying to solve).
  2. Please provide benchstat results in the PR description.

@Dipanshusinghh
Dipanshusinghh force-pushed the feature/allow-key-duplication branch from 03674ce to 2084182 Compare June 19, 2026 05:21
@codecov

codecov Bot commented Jun 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.1%. Comparing base (e35d473) to head (d9b6d66).
⚠️ Report is 11 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@          Coverage Diff           @@
##            main   #8488    +/-   ##
======================================
  Coverage   84.0%   84.1%            
======================================
  Files        329     331     +2     
  Lines      26068   26506   +438     
======================================
+ Hits       21919   22312   +393     
- Misses      3768    3788    +20     
- Partials     381     406    +25     
Files with missing lines Coverage Δ
sdk/trace/provider.go 94.5% <100.0%> (+0.7%) ⬆️
sdk/trace/span.go 84.2% <100.0%> (+0.2%) ⬆️

... and 11 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Comment thread sdk/trace/benchmark_test.go
@pellared

Copy link
Copy Markdown
Member

Marking as draft. Please make it "ready for review" once feedback is addressed. Please also avoid using force-push as described in CONTRIBUTING.md.

@pellared
pellared marked this pull request as draft June 19, 2026 06:26
@Dipanshusinghh
Dipanshusinghh marked this pull request as ready for review June 19, 2026 08:36
@Dipanshusinghh
Dipanshusinghh requested a review from pellared June 19, 2026 08:36

@pellared pellared left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I made a quick review.
Can you compare benchmarks results with main using benchstat for the go.opentelemetry.io/otel/sdk/trace package and add the results in the PR description?

Comment thread sdk/trace/provider.go
Comment thread sdk/trace/provider.go Outdated
Comment thread sdk/trace/provider.go Outdated
Comment thread sdk/trace/span.go Outdated
Comment thread sdk/trace/span.go Outdated
Comment thread sdk/trace/span.go Outdated
Comment thread sdk/trace/span.go Outdated
Comment thread sdk/trace/span.go Outdated
@Dipanshusinghh
Dipanshusinghh requested a review from pellared June 19, 2026 09:44
@Dipanshusinghh

Copy link
Copy Markdown
Contributor Author

I made a quick review. Can you compare benchmarks results with main using benchstat for the go.opentelemetry.io/otel/sdk/trace package and add the results in the PR description?

Done! Added benchstat results comparing base (7128e65) to this branch in the PR description — no regression on the default path, and WithAllowKeyDuplication speedups are shown as a table.

Comment thread sdk/trace/benchmark_test.go Outdated
Comment thread sdk/trace/benchmark_test.go Outdated
@Dipanshusinghh
Dipanshusinghh requested a review from pellared June 19, 2026 10:21
@pellared

Copy link
Copy Markdown
Member

Please update the PR description after the changes.

@Dipanshusinghh

Copy link
Copy Markdown
Contributor Author

Please update the PR description after the changes.

Done i have updated the PR discription.

@Dipanshusinghh
Dipanshusinghh force-pushed the feature/allow-key-duplication branch from 8ec3b0b to 75ba388 Compare June 27, 2026 03:19

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

Comment thread CHANGELOG.md Outdated
Comment thread sdk/trace/provider.go Outdated
Comment thread sdk/trace/trace_test.go Outdated
Comment thread sdk/trace/provider.go

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.

@Dipanshusinghh
Dipanshusinghh requested a review from dashpole July 9, 2026 14:06

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.

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comment thread sdk/trace/span.go Outdated
@MrAlias

MrAlias commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

@Dipanshusinghh please merge main, resolve all threads that you have addressed, restore the accidentially removed changelog entry, fix the stale comments.

@Dipanshusinghh

Dipanshusinghh commented Aug 11, 2026

Copy link
Copy Markdown
Contributor Author

@Dipanshusinghh please merge main, resolve all threads that you have addressed, restore the accidentially removed changelog entry, fix the stale comments.

will do this by today
thnx !

@Dipanshusinghh

Copy link
Copy Markdown
Contributor Author

@MrAlias PTAL !

@MrAlias

MrAlias commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

resolve all threads that you have addressed

@Dipanshusinghh are the unresolved threads still works in progress? Do you have permissions to resolve the threads?

@Dipanshusinghh

Copy link
Copy Markdown
Contributor Author

resolve all threads that you have addressed

@Dipanshusinghh are the unresolved threads still works in progress? Do you have permissions to resolve the threads?

sorry just forgot to click the resolve button! done now thnx

Comment thread CHANGELOG.md Outdated
Co-authored-by: Tyler Yahn <MrAlias@users.noreply.github.com>
@Dipanshusinghh
Dipanshusinghh requested a review from MrAlias August 13, 2026 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add trace.WithAllowKeyDuplication

5 participants