Enforce last-value-wins semantics in AttributesMap without performance regression

[EvgeniiR]

Fixes the Issue #7897

TLDR; Current AttributesMap does not fully compy with that part of spec:

“Setting an attribute with the same key as an existing attribute SHOULD overwrite the existing attribute’s value.”
https://opentelemetry.io/docs/specs/otel/trace/api/#set-attributes

Problem

AttributesMap extended HashMap<AttributeKey<?>, Object>, using AttributeKey as the map
key. Because AttributeKey.equals() includes the attribute type, two attributes with the same
name but different types (e.g. stringKey("http.method") and longKey("http.method")) were
stored as separate entries.
This violated the OpenTelemetry specification, which requires that
attribute name alone determines identity — last write wins regardless of type.

Solution

Correctness fix

Replace the HashMap<AttributeKey<?>, Object> backing store with a string-keyed map so that
put("http.method", String) followed by put("http.method", Long) results in exactly one entry
(the Long value), consuming only one capacity slot.

The first implementations were based on HashMap/LinkedHashMap (2nd performed better once forEach was included in the benchmarks), but they introduced another issue — the fixed AttributesMap performed ~40-80% worse than baseline.

putThenForEach — LinkedHashMap fix vs baseline

n	baseline	LHM fix	LHM vs baseline
4	39.3 ± 1.9	50.1 ± 0.3	+27%
16	156.0 ± 2.4	222.8 ± 9.0	+43%
20	179.7 ± 3.9	261.7 ± 7.2	+46%
32	346.4 ± 20.8	462.6 ± 4.7	+34%
128	1465.5 ± 83.6	2085.2 ± 106.3	+42%

Therefore, I started looking for a better solution that would preserve the required last-value-wins semantics without introducing a performance regression.

LinkedHashMap implementation can be observed in the previous commit — https://github.com/EvgeniiR/opentelemetry-java/blob/d7df58af76e693aa1fe897d2757e2bdb50ab9798/sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/AttributesMap.java

Final solution is described below.

Performance optimization

Instead of LinkedHashMap<String, AttributeEntry>, use parallel arrays with an open-addressing
int[] hash table (linear probing, load factor ≤ 0.5):

int[]           hashTable   — slot → entryIndex+1 (0 = empty, JVM zero-init)
String[]        entryNames  — for hash lookup
AttributeKey[]  entryKeys   — for get() type check and forEach()
Object[]        entryValues — attribute values

forEach becomes a tight sequential array loop with no pointer chasing, directly benefiting the export.

Benchmark results (avgt ns/op, lower is better)

putThenForEach — N unique puts + 1 forEach (dominant production path)

n	baseline	PA	PA vs base
4	39.3 ± 1.9	39.9 ± 0.6	+2%
16	156.0 ± 2.4	135.6 ± 7.7	−13%
20	179.7 ± 3.9	241.3 ± 11.0	+34%
32	346.4 ± 20.8	346.4 ± 14.4	0%
128	1465.5 ± 83.6	2047.5 ± 52.3	+40%

The small-span case is effectively unchanged, and 16 attributes improve. The 20- and
128-attribute cases regress: the parallel-array implementation starts with arrays
for 16 entries and grows through 32 and 64 before reaching 128.

AttributesMapBenchmark — putThenForEach memory allocation (gc.alloc.rate.norm, B/op, lower is better)

n	baseline	PA	delta
4	272	384	+41%
16	800	384	−52%
20	928	1136	+22%
32	1584	1136	−28%
128	6224	5104	−18%

A smaller initial array would improve n=4 case, but would move the
resize cost to spans with 16–20 attributes.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.51%. Comparing base (f8bd413) to head (44f6b9c).

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #8548      +/-   ##
============================================
+ Coverage     91.48%   91.51%   +0.02%     
- Complexity    10467    10487      +20     
============================================
  Files          1021     1021              
  Lines         27694    27755      +61     
  Branches       3247     3255       +8     
============================================
+ Hits          25337    25401      +64     
+ Misses         1615     1614       -1     
+ Partials        742      740       -2     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[EvgeniiR]

Benchmark results ... putThenForEach — N unique puts + 1 forEach

This PR also includes benchmarks for individual read/write operations (uniqueKeys, getHit, getTypeMiss, forEachAll). The project already has SpanRecordBenchmark and FillSpanBenchmark that cover the write path at the span level. I will publish some additional benchmarks under cut, if for some reason you want to check them out too.

More benchmarks

AttributesMapBenchmark — write (avg ns/op, lower is better)

Benchmark	n	baseline	LHM fix	PA	PA vs base	PA vs LHM
uniqueKeys	4	41.8	48.9	38.2	−3.6 (−9%)	−10.7 (−22%)
uniqueKeys	16	134.9	231.0	134.3	−0.6 (0%)	−96.7 (−42%)
uniqueKeys	128	1309.7	2199.7	1950.2	+640.5 (+49%)	−249.5 (−11%)
sameKeySameType	4	46.1	33.7	35.0	−11.1 (−24%)	+1.3 (+4%)
sameKeySameType	16	164.5	105.4	124.6	−39.9 (−24%)	+19.2 (+18%)
sameKeySameType	128	1320.7	659.1	862.5	−458.2 (−35%)	+203.4 (+31%)
sameKeyDifferentType	4	32.1	42.9	42.9	+10.8 (+34%)	0.0 (0%)
sameKeyDifferentType	16	181.6	146.5	156.6	−25.0 (−14%)	+10.1 (+7%)
sameKeyDifferentType	128	1407.2	1073.0	1183.7	−223.5 (−16%)	+110.7 (+10%)
mixedUniqueAndOverwrite	4	32.1	43.2	44.4	+12.3 (+38%)	+1.2 (+3%)
mixedUniqueAndOverwrite	16	134.0	155.7	154.5	+20.5 (+15%)	−1.2 (−1%)
mixedUniqueAndOverwrite	128	1400.7	1531.9	1721.5	+320.8 (+23%)	+189.6 (+12%)

AttributesMapBenchmark — read (avg ns/op, lower is better)

Benchmark	n	baseline	LHM fix	PA	PA vs base	PA vs LHM
getHit	4	10.9	15.8	18.1	+7.2 (+66%)	+2.3 (+15%)
getHit	16	41.7	58.3	73.7	+32.0 (+77%)	+15.4 (+26%)
getHit	128	350.6	476.6	916.9	+566.3 (+161%)	+440.3 (+92%)
getTypeMiss	4	8.7	23.9	24.7	+16.0 (+184%)	+0.8 (+3%)
getTypeMiss	16	35.0	96.9	97.3	+62.3 (+178%)	+0.4 (+0%)
getTypeMiss	128	283.3	856.9	1136.7	+853.4 (+301%)	+279.8 (+33%)
forEachAll	4	11.1	5.8	2.9	−8.2 (−74%)	−2.9 (−50%)
forEachAll	16	26.0	25.3	8.1	−17.9 (−69%)	−17.2 (−68%)
forEachAll	128	185.5	229.9	46.8	−138.7 (−75%)	−183.1 (−80%)

FillSpanBenchmark (ops/ms, higher is better)

Benchmark	baseline	LHM fix	PA	PA vs base	PA vs LHM
setFourAttributes	6160.0	5589.7	5197.7	−962.3 (−16%)	−392.0 (−7%)

SpanRecordBenchmark (ops/s, higher is better)

threads	SpanSize	baseline	LHM fix	PA	PA vs base	PA vs LHM
1	SMALL	6 043 137	6 079 646	5 334 831	−708 306 (−12%)	−744 815 (−12%)
1	MEDIUM	658 387	650 851	574 200	−84 187 (−13%)	−76 651 (−12%)
1	LARGE	65 240	62 313	64 207	−1 033 (−2%)	+1 894 (+3%)
4	SMALL	8 785 156	9 009 770	8 454 487	−330 669 (−4%)	−555 283 (−6%)
4	MEDIUM	1 535 077	1 697 175	1 463 172	−71 905 (−5%)	−234 003 (−14%)
4	LARGE	165 137	175 858	165 824	+687 (+0.4%)	−10 034 (−6%)

[jack-berg]

This violated the OpenTelemetry specification, which requires that
attribute name alone determines identity — last write wins regardless of type.

Can you provide a link the portion of the spec you're referring to? Thanks.

[EvgeniiR]

@jack-berg Hi! I'm referring to this part of spec:

https://opentelemetry.io/docs/specs/otel/trace/api/#set-attributes

“Setting an attribute with the same key as an existing attribute SHOULD overwrite the existing attribute’s value.”

I'm not sure if you're asking because the PR description doesn't provide enough context, or if you think I might be misinterpreting the spec and solving a problem that doesn't actually exist. Happy to clarify either way.

I originally found this through the linked issue. After seeing unfinished implementation attempt and corresponding discussion, I decided to try and finish it.

Happy to hear your thoughts, thanks!

[jack-berg]

I'm not sure if you're asking because the PR description doesn't provide enough context, or if you think I might be misinterpreting the spec and solving a problem that doesn't actually exist. Happy to clarify either way.

The spec is big and occasionally contradictory. Always good to have a reference!

I originally found this through the linked issue.

Missing the linked issue

[EvgeniiR]

Missing the linked issue

That one (from the PR description):

Fixes #7897

Also previous implementation attempt from another developer — #8420

[jack-berg]

Hey so I've been thinking about this. AttributesMap exists because the default implementation of Attributes / AttributesBuilder is not limits aware. I think that the fact that its backed by HashMap is a function of convenience: we need a simple implementation that we can apply limits too as its being built up. I think performance is important, and a Map based implementation also has better put performance than the default array based implementations ImmutableKeyValuePairs / ArrayBackedAttributesBuilder. But I think this is coincidence, since I've never heard us telling people "prefer using Span.setAttribute because it uses a more performant map based implementation".

I've never loved the fact that AttributesMap exists. I'd rather have one implementation. And I think your changes to AttributesMap reinforces this because while it still tries to do some map things for performance, other parts of it are starting to look more like ImmutableKeyValuePairs / ArrayBackedAttributesBuilder. And so its got me thinking about whether we can evolve ImmutableKeyValuePairs / ArrayBackedAttributesBuilder to meet the limits requirements and rip out AttributesMap altogether.

I've got two prototypes on how this could work, besides yours which I'll call "Approach 1":

• Approach 2: Make the internal classes ImmutableKeyValuePairs / ArrayBackedAttributesBuilder limits aware, and use gradle build tooling to make a copy of them into sdk/common. Public API surface area stays the same, and we have one source of truth for an Attributes implementation, but we publish two copies of it.
• Approach 3: Extend opentelemetry-api with capabilities to enforce limits directly in AttributesBuilder: Attributes.builder(AttributeLimits). Delete AttributesMap and leverage new api enforced attribute limits in its place.

Relative strengths / weaknesses of the different approaches:

Dimension	PR #8548	Approach 2 (source copy)	Approach 3 (public API)
Public API added	None (patch)	None (patch)	AttributeLimits + AttributeLimitsBuilder + one method on Attributes
Code shape	Bespoke ~200-line hash table in sdk/common/internal. Self-contained.	One canonical class in api.common implementing both AttributesBuilder and Attributes, plus SDK-only helper methods. Duplicated onto classpath via Gradle Copy task.	One class in api.common implementing only AttributesBuilder. AttributesMap deleted. No build magic.
Typical spans (SMALL/MEDIUM, 0-10 attrs)	As submitted: −12 to −13% on SpanRecord (likely fixable)	Within noise (~0 to −6%)	Within noise (~0 to −6%)
Large spans (100 attrs)	−2% (O(1) put wins)	−36% (O(n^2) linear-scan dedup, or more memory for additional tracking that avoids the O(n^2))	−37% (O(n^2) linear-scan dedup, or more memory for additional tracking that avoids the O(n^2))
forEach throughput	Big win: −50% to −80% latency (tight array loop)	~baseline	~baseline
User benefit / extensibility	None (SDK-internal)	None (SDK-internal). Future limit fields require touching the copy source + filter awareness.	Users can create bounded builders themselves. Future limit fields added non-breakingly via AutoValue pattern.

Currently I lean towards Approach 3. I recognize that it probably seems somewhat orthogonal to the task you set out to solve. Like, isn't extending Attributes / AttributesBuilder a different scope / goal than making the existing SdkSpan, SdkLogRecordBuilder follow the spec W.R.T. "last write wins"? And maybe we do end up treating them separately. But the appeal of maintaining a separate Attributes implementation in AttributesMap goes away when its no longer a thin wrapper over HashMap, but rather a dedicated data structure with various low level nuances.

[jack-berg]

@open-telemetry/java-approvers PTAL at my message above and let me know if you have thoughts.

[EvgeniiR]

Hi, thanks for the feedback. I definitely hadn't considered the idea of abandoning AttributesMap completely.

Also I re-checked my benchmarks and there was a mistake. The AttributesMapBenchmark created the map with capacity = numAttributes, but the SDK creates it with the configured limit (128 by default), and PA allocates its initial arrays based on capacity. With capacity=128 the memory numbers at n=4 flip: PA is actually +41% allocations / +10% time vs baseline, not −47% as in the description. It is fixable for n=4 with smaller initial arrays, but then spans with 16–20 attributes will spend more time on resize instead.
I updated the benchmark code, but beware that the numbers I provided in the PR description are stale now.
upd: updated numbers in PR description

Approach 3 looks good to me, so I will leave the decision to the maintainers.

[jkwatson]

I would love to get us to a single implementation as well. The de-duping logic in the array based approach has always been the scary bit to my avoiding use using it instead of AttributesMap. If we're ok eating that performance hit, option 3 seems fine to me (as does 2, tbh). it's all the usual tradeoff of memory vs. speed, slightly coupled with maintenance complexity, I suppose (it's more maintenance to have 2 implementations vs. just 1).

I wonder if there's a clever solution to the deduping, using some sort of lightweight sketch-based approach, alongside the array to save the linear scan in most cases. Could be an interesting research project for someone with time on their hands. ;)

[opentelemetry-pr-dashboard]

Pull request dashboard status

Waiting on reviewers · refreshed 2026-08-02 21:22 UTC

Review the latest changes.

Status above doesn't look right?

• Just replied or pushed? Anything around or after the refresh time above may not be picked up yet — give it a few minutes.
• Anything look wrong? Report it with what you expected; it helps us improve the dashboard.

[jack-berg]

After exploring various versions of trying to reuse ArrayBackedAttributes / ArrayBackedAttributesBuilder in both API and SDK, I think its a dead end. See #8681 description for details. I've closed #8681 and #8681 accordingly, and think we should go with some version of the strategy proposed in this PR.

Basically, I think for SDK attribute requirements (iterative building with enforcement of limits), we do need a separate hash map style implementation to maintain performance. It does mean maintaining two implementations, but so be it.

I still need to think through / review this PR carefully.

[EvgeniiR]

I rebased onto the latest main and updated the PR description with the correct benchmark results. The previous results were stale and incorrect due to a benchmark code issue I mentioned and fixed earlier: #8548 (comment) . No code changes were made as part of this update,