diff --git a/sdk/common/src/jmh/java/io/opentelemetry/sdk/common/internal/AttributesMapBenchmark.java b/sdk/common/src/jmh/java/io/opentelemetry/sdk/common/internal/AttributesMapBenchmark.java new file mode 100644 index 00000000000..e0baa45e405 --- /dev/null +++ b/sdk/common/src/jmh/java/io/opentelemetry/sdk/common/internal/AttributesMapBenchmark.java @@ -0,0 +1,139 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.common.internal; + +import static io.opentelemetry.api.common.AttributeKey.booleanKey; +import static io.opentelemetry.api.common.AttributeKey.stringKey; + +import io.opentelemetry.api.common.AttributeKey; +import java.util.ArrayList; +import java.util.List; +import java.util.concurrent.TimeUnit; +import org.openjdk.jmh.annotations.Benchmark; +import org.openjdk.jmh.annotations.BenchmarkMode; +import org.openjdk.jmh.annotations.Fork; +import org.openjdk.jmh.annotations.Measurement; +import org.openjdk.jmh.annotations.Mode; +import org.openjdk.jmh.annotations.OutputTimeUnit; +import org.openjdk.jmh.annotations.Param; +import org.openjdk.jmh.annotations.Scope; +import org.openjdk.jmh.annotations.Setup; +import org.openjdk.jmh.annotations.State; +import org.openjdk.jmh.annotations.Warmup; +import org.openjdk.jmh.infra.Blackhole; + +/** + * Microbenchmark for {@link AttributesMap}. Parametrized by number of attributes. + * + *

Write scenarios: + * + *

+ * + *

Read scenarios (run on a pre-filled map of {@code numAttributes} unique string entries): + * + *

+ */ +@BenchmarkMode(Mode.AverageTime) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 5, time = 200, timeUnit = TimeUnit.MILLISECONDS) +@Measurement(iterations = 10, time = 200, timeUnit = TimeUnit.MILLISECONDS) +@Fork(2) +@State(Scope.Thread) +public class AttributesMapBenchmark { + + // Default SpanLimits attribute count limit. + private static final int CAPACITY = 128; + + @Param({"4", "16", "20", "32", "128"}) + int numAttributes; + + private List> stringKeys; + private List> boolKeys; + private List values; + + // Pre-filled map used by read benchmarks — populated once in @Setup. + private AttributesMap filledMap; + + @Setup + public void setup() { + stringKeys = new ArrayList<>(numAttributes); + boolKeys = new ArrayList<>(numAttributes); + values = new ArrayList<>(numAttributes); + for (int i = 0; i < numAttributes; i++) { + stringKeys.add(stringKey("key" + i)); + boolKeys.add(booleanKey("key" + i)); + values.add("value" + i); + } + filledMap = AttributesMap.create(CAPACITY, Integer.MAX_VALUE); + for (int i = 0; i < numAttributes; i++) { + filledMap.put(stringKeys.get(i), values.get(i)); + } + } + + /** Each key name is unique — the common production case. */ + @Benchmark + public AttributesMap uniqueKeys() { + AttributesMap map = AttributesMap.create(CAPACITY, Integer.MAX_VALUE); + for (int i = 0; i < numAttributes; i++) { + map.put(stringKeys.get(i), values.get(i)); + } + return map; + } + + // ---- Read benchmarks (operate on pre-filled map) ---- + + /** + * Lookup with the exact stored key type — always a hit. Measures the cost of a successful {@code + * get()} for each entry in the map. + */ + @Benchmark + public void getHit(Blackhole bh) { + for (int i = 0; i < numAttributes; i++) { + bh.consume(filledMap.get(stringKeys.get(i))); + } + } + + /** + * Lookup with a different type for the same key name — always returns null. + * + *

The map holds N string-typed entries; boolean keys for the same names locate each entry by + * name but fail the type check. Isolates the cost of a name-hit / type-miss lookup. + */ + @Benchmark + public void getTypeMiss(Blackhole bh) { + for (int i = 0; i < numAttributes; i++) { + bh.consume(filledMap.get(boolKeys.get(i))); + } + } + + /** Full iteration over all entries via {@code forEach}. */ + @Benchmark + public void forEachAll(Blackhole bh) { + filledMap.forEach((k, v) -> bh.consume(v)); + } + + /** + * Combined write + read cycle: fill a fresh map with N unique string keys, then iterate all + * entries once. Models the dominant production path: N puts during span building, followed by one + * forEach at export time. + */ + @Benchmark + public void putThenForEach(Blackhole bh) { + AttributesMap map = AttributesMap.create(CAPACITY, Integer.MAX_VALUE); + for (int i = 0; i < numAttributes; i++) { + map.put(stringKeys.get(i), values.get(i)); + } + map.forEach((k, v) -> bh.consume(v)); + } +} diff --git a/sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/AttributesMap.java b/sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/AttributesMap.java index 0ddf9599752..1f3283f672d 100644 --- a/sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/AttributesMap.java +++ b/sdk/common/src/main/java/io/opentelemetry/sdk/common/internal/AttributesMap.java @@ -8,6 +8,7 @@ import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; +import java.util.Arrays; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -18,28 +19,54 @@ * A map with a fixed capacity that drops attributes when the map gets full, and which truncates * string and array string attribute values to the {@link #lengthLimit}. * - *

WARNING: In order to reduce memory allocation, this class extends {@link HashMap} when it - * would be more appropriate to delegate. The problem with extending is that we don't enforce that - * all {@link HashMap} methods for reading / writing data conform to the configured attribute - * limits. Therefore, it's easy to accidentally call something like {@link Map#putAll(Map)} and - * bypass the restrictions (see #7135). Callers MUST - * take care to only call methods from {@link AttributesMap}, and not {@link HashMap}. + *

Keyed internally by attribute name, so that attributes with the same name but different types + * are treated as the same key (last-value-wins), consistent with the OpenTelemetry specification. + * + *

Backed by parallel arrays and an open-addressing {@code int[]} hash table (linear probing, + * load factor ≤ 0.5). Avoids per-entry object allocation; {@code forEach} is a tight sequential + * array loop with no pointer chasing. * *

This class is internal and is hence not for public use. Its APIs are unstable and can change * at any time. */ -public final class AttributesMap extends HashMap, Object> implements Attributes { +public final class AttributesMap implements Attributes { - private static final long serialVersionUID = -5072696312123632376L; + /** + * Sentinel value meaning "slot is empty" in the hash table. Using 0 lets {@code new int[n]} (JVM + * zero-initialization) serve as the initial fill, eliminating explicit {@code Arrays.fill} calls. + * Occupied slots store {@code entryIndex + 1} so that index 0 is distinguishable from EMPTY. + */ + private static final int EMPTY = 0; - private final long capacity; + private final int capacity; private final int lengthLimit; private int totalAddedValues = 0; + private int size = 0; + + /** + * Open-addressing hash table: {@code hashTable[slot]} = index into entry arrays, or {@link + * #EMPTY}. Length is always a power of 2 and ≥ 2× the entry array length (load factor ≤ 0.5). + */ + private int[] hashTable; + + /** Cached {@code hashTable.length - 1}; kept in sync with {@link #hashTable}. */ + private int mask; + + /** Parallel entry arrays. Index {@code i} holds the i-th inserted entry. */ + private String[] entryNames; + + private AttributeKey[] entryKeys; + private Object[] entryValues; private AttributesMap(long capacity, int lengthLimit) { - this.capacity = capacity; + this.capacity = (int) Math.min(capacity, Integer.MAX_VALUE); this.lengthLimit = lengthLimit; + int init = (int) Math.min(capacity, 16L); + entryNames = new String[init]; + entryKeys = new AttributeKey[init]; + entryValues = new Object[init]; + hashTable = new int[tableSizeFor(init)]; // JVM zero-init == EMPTY + mask = hashTable.length - 1; } /** @@ -55,18 +82,40 @@ public static AttributesMap create(long capacity, int lengthLimit) { /** * Add the attribute key value pair, applying capacity and length limits. Callers MUST ensure the * {@code value} type matches the type required by {@code key}. + * + *

If an attribute with the same string key name already exists (regardless of type), it is + * overwritten — last-value-wins, consistent with the OTel spec. */ - @Override @Nullable public Object put(AttributeKey key, @Nullable Object value) { if (value == null) { return null; } totalAddedValues++; - if (size() >= capacity && !containsKey(key)) { + String name = key.getKey(); + int slot = findSlot(name); + int stored = hashTable[slot]; // EMPTY or 1-based index + if (stored == EMPTY) { + if (size >= capacity) { + return null; + } + Object limited = AttributeUtil.applyAttributeLengthLimit(value, lengthLimit); + if (size == entryNames.length) { + grow(); + slot = findSlot(name); // hashTable was rebuilt by grow() + } + entryNames[size] = name; + entryKeys[size] = key; + entryValues[size] = limited; + hashTable[slot] = size + 1; // 1-based + size++; return null; } - return super.put(key, AttributeUtil.applyAttributeLengthLimit(value, lengthLimit)); + int idx = stored - 1; + Object old = entryValues[idx]; + entryKeys[idx] = key; + entryValues[idx] = AttributeUtil.applyAttributeLengthLimit(value, lengthLimit); + return old; } /** Generic overload of {@link #put(AttributeKey, Object)}. */ @@ -83,17 +132,34 @@ public int getTotalAddedValues() { @Override @Nullable public T get(AttributeKey key) { - return (T) super.get(key); + int stored = hashTable[findSlot(key.getKey())]; + if (stored == EMPTY) { + return null; + } + int idx = stored - 1; + if (!entryKeys[idx].getType().equals(key.getType())) { + return null; + } + return (T) entryValues[idx]; + } + + @Override + public int size() { + return size; + } + + @Override + public boolean isEmpty() { + return size == 0; } @Override public Map, Object> asMap() { - // Because Attributes is marked Immutable, IDEs may recognize this as redundant usage. However, - // this class is private and is actually mutable, so we need to wrap with unmodifiableMap - // anyways. We implement the immutable Attributes for this class to support the - // Attributes.builder().putAll usage - it is tricky but an implementation detail of this private - // class. - return Collections.unmodifiableMap(this); + Map, Object> snapshot = new HashMap<>(size); + for (int i = 0; i < size; i++) { + snapshot.put(entryKeys[i], entryValues[i]); + } + return Collections.unmodifiableMap(snapshot); } @Override @@ -103,17 +169,32 @@ public AttributesBuilder toBuilder() { @Override public void forEach(BiConsumer, ? super Object> action) { - // https://github.com/open-telemetry/opentelemetry-java/issues/4161 - // Help out android desugaring by having an explicit call to HashMap.forEach, when forEach is - // just called through Attributes.forEach desugaring is unable to correctly handle it. - super.forEach(action); + for (int i = 0; i < size; i++) { + action.accept(entryKeys[i], entryValues[i]); + } + } + + @Override + public boolean equals(@Nullable Object o) { + if (this == o) { + return true; + } + if (!(o instanceof Attributes)) { + return false; + } + return asMap().equals(((Attributes) o).asMap()); + } + + @Override + public int hashCode() { + return asMap().hashCode(); } @Override public String toString() { return "AttributesMap{" + "data=" - + super.toString() + + asMap() + ", capacity=" + capacity + ", totalAddedValues=" @@ -125,4 +206,44 @@ public String toString() { public Attributes immutableCopy() { return Attributes.builder().putAll(this).build(); } + + /** + * Returns the hash table slot that either contains the entry for {@code name} or is the first + * empty slot available for insertion. Single shared probe loop used by {@code put}, {@code get}, + * and {@code grow}. Slots store {@code entryIndex + 1}; 0 ({@link #EMPTY}) means unoccupied. + */ + private int findSlot(String name) { + int slot = name.hashCode() & mask; + int stored; + while ((stored = hashTable[slot]) != EMPTY && !entryNames[stored - 1].equals(name)) { + slot = (slot + 1) & mask; + } + return slot; + } + + private void grow() { + long maxLen = Math.min(capacity, (long) Integer.MAX_VALUE - 8); + int newLen = (int) Math.min((long) entryNames.length * 2, maxLen); + entryNames = Arrays.copyOf(entryNames, newLen); + entryKeys = Arrays.copyOf(entryKeys, newLen); + entryValues = Arrays.copyOf(entryValues, newLen); + hashTable = new int[tableSizeFor(newLen)]; // JVM zero-init == EMPTY + mask = hashTable.length - 1; + for (int i = 0; i < size; i++) { + int slot = findSlot(entryNames[i]); + hashTable[slot] = i + 1; // 1-based + } + } + + /** + * Returns the smallest power of 2 that is ≥ 2n, guaranteeing load factor ≤ 0.5. Using {@code + * (2n-1)} instead of {@code 2n} prevents doubling the result when {@code n} is itself a power of + * 2. + */ + private static int tableSizeFor(int n) { + if (n <= 2) { + return 4; + } + return Integer.highestOneBit(2 * n - 1) << 1; + } } diff --git a/sdk/common/src/test/java/io/opentelemetry/sdk/common/internal/AttributesMapTest.java b/sdk/common/src/test/java/io/opentelemetry/sdk/common/internal/AttributesMapTest.java index a7a1f8ecb2e..10cc7a82e5f 100644 --- a/sdk/common/src/test/java/io/opentelemetry/sdk/common/internal/AttributesMapTest.java +++ b/sdk/common/src/test/java/io/opentelemetry/sdk/common/internal/AttributesMapTest.java @@ -5,14 +5,157 @@ package io.opentelemetry.sdk.common.internal; +import static io.opentelemetry.api.common.AttributeKey.booleanKey; import static io.opentelemetry.api.common.AttributeKey.longKey; +import static io.opentelemetry.api.common.AttributeKey.stringKey; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.entry; +import com.google.common.testing.EqualsTester; +import io.opentelemetry.api.common.Attributes; +import java.util.ArrayList; +import java.util.List; import org.junit.jupiter.api.Test; class AttributesMapTest { + // ---- put ---- + + @Test + void put_returnsNullForNewEntry() { + AttributesMap map = AttributesMap.create(10, Integer.MAX_VALUE); + + assertThat(map.put(stringKey("k"), "v")).isNull(); + } + + @Test + void put_returnsOldValueOnOverwrite() { + AttributesMap map = AttributesMap.create(10, Integer.MAX_VALUE); + map.put(stringKey("k"), "first"); + + assertThat(map.put(stringKey("k"), "second")).isEqualTo("first"); + assertThat(map.get(stringKey("k"))).isEqualTo("second"); + } + + @Test + void put_ignoresNullValue() { + AttributesMap map = AttributesMap.create(10, Integer.MAX_VALUE); + map.put(stringKey("k"), null); + + assertThat(map.size()).isEqualTo(0); + assertThat(map.isEmpty()).isTrue(); + assertThat(map.getTotalAddedValues()).isEqualTo(0); + } + + @Test + void putSameKeyDifferentType_lastValueWins() { + AttributesMap map = AttributesMap.create(128, Integer.MAX_VALUE); + map.put(stringKey("k"), "hello"); + map.put(booleanKey("k"), false); + + assertThat(map.size()).isEqualTo(1); + assertThat(map.get(booleanKey("k"))).isEqualTo(false); + assertThat(map.get(stringKey("k"))).isNull(); + } + + @Test + void putSameKeyDifferentType_doesNotConsumeExtraCapacity() { + AttributesMap map = AttributesMap.create(2, Integer.MAX_VALUE); + map.put(stringKey("a"), "v1"); + map.put(booleanKey("a"), false); // overwrite — must not consume a new capacity slot + map.put(longKey("b"), 42L); + + assertThat(map.size()).isEqualTo(2); + assertThat(map.get(booleanKey("a"))).isEqualTo(false); + assertThat(map.get(longKey("b"))).isEqualTo(42L); + } + + @Test + void putSameKeyDifferentType_previousTypeGetReturnsNull() { + AttributesMap map = AttributesMap.create(128, Integer.MAX_VALUE); + map.put(stringKey("k"), "hello"); + map.put(booleanKey("k"), true); + + assertThat(map.get(stringKey("k"))).isNull(); + assertThat(map.get(booleanKey("k"))).isEqualTo(true); + } + + // ---- get ---- + + @Test + void get_returnsNullForAbsentKey() { + AttributesMap map = AttributesMap.create(10, Integer.MAX_VALUE); + + assertThat(map.get(stringKey("absent"))).isNull(); + } + + // ---- capacity ---- + + @Test + void capacity_dropsEntriesBeyondLimit() { + AttributesMap map = AttributesMap.create(2, Integer.MAX_VALUE); + map.put(stringKey("a"), "v1"); + map.put(stringKey("b"), "v2"); + map.put(stringKey("c"), "v3"); // dropped — capacity reached + + assertThat(map.size()).isEqualTo(2); + assertThat(map.getTotalAddedValues()).isEqualTo(3); + assertThat(map.get(stringKey("c"))).isNull(); + } + + @Test + void capacity_zeroDropsAllEntries() { + AttributesMap map = AttributesMap.create(0, Integer.MAX_VALUE); + map.put(stringKey("k"), "v"); + + assertThat(map.size()).isEqualTo(0); + assertThat(map.isEmpty()).isTrue(); + } + + // ---- grow ---- + + @Test + void grow_preservesAllEntriesWhenSizeExceedsInitialArrayLength() { + // init = min(capacity, 16) = 16; grow() is triggered when the 17th entry is inserted + int n = 20; + AttributesMap map = AttributesMap.create(n, Integer.MAX_VALUE); + for (int i = 0; i < n; i++) { + map.put(stringKey("key" + i), "val" + i); + } + + assertThat(map.size()).isEqualTo(n); + for (int i = 0; i < n; i++) { + assertThat(map.get(stringKey("key" + i))).isEqualTo("val" + i); + } + } + + // ---- lengthLimit ---- + + @Test + void lengthLimit_truncatesStringValues() { + AttributesMap map = AttributesMap.create(10, 3); + map.put(stringKey("k"), "hello"); + + assertThat(map.get(stringKey("k"))).isEqualTo("hel"); + } + + // ---- forEach ---- + + @Test + void forEach_iteratesInInsertionOrder() { + AttributesMap map = AttributesMap.create(10, Integer.MAX_VALUE); + map.put(stringKey("first"), "v1"); + map.put(stringKey("second"), "v2"); + map.put(stringKey("third"), "v3"); + + List keys = new ArrayList<>(); + map.forEach((k, v) -> keys.add(k.getKey())); + + assertThat(keys).containsExactly("first", "second", "third"); + } + + // ---- views ---- + @Test void asMap() { AttributesMap attributesMap = AttributesMap.create(2, Integer.MAX_VALUE); @@ -22,4 +165,28 @@ void asMap() { assertThat(attributesMap.asMap()) .containsOnly(entry(longKey("one"), 1L), entry(longKey("two"), 2L)); } + + @Test + void immutableCopy_containsAllEntries() { + AttributesMap map = AttributesMap.create(10, Integer.MAX_VALUE); + map.put(stringKey("a"), "v1"); + map.put(longKey("b"), 42L); + + Attributes copy = map.immutableCopy(); + + assertThat(copy.get(stringKey("a"))).isEqualTo("v1"); + assertThat(copy.get(longKey("b"))).isEqualTo(42L); + } + + @Test + void equals_andHashCode() { + AttributesMap mapV1a = AttributesMap.create(10, Integer.MAX_VALUE); + mapV1a.put(stringKey("k"), "v1"); + AttributesMap mapV1b = AttributesMap.create(10, Integer.MAX_VALUE); + mapV1b.put(stringKey("k"), "v1"); + AttributesMap mapV2 = AttributesMap.create(10, Integer.MAX_VALUE); + mapV2.put(stringKey("k"), "v2"); + + new EqualsTester().addEqualityGroup(mapV1a, mapV1b).addEqualityGroup(mapV2).testEquals(); + } } diff --git a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java index 2a69ade6702..b8f662e2791 100644 --- a/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java +++ b/sdk/trace/src/test/java/io/opentelemetry/sdk/trace/SdkSpanBuilderTest.java @@ -1172,4 +1172,25 @@ void doNotCrash() { }) .doesNotThrowAnyException(); } + + @Test + void setAttribute_sameKeyDifferentType_lastValueWins() { + // Regression test for https://github.com/open-telemetry/opentelemetry-java/issues/7897 + // Setting the same string key with different types must overwrite, not accumulate. + SdkSpan span = + (SdkSpan) + sdkTracer + .spanBuilder("test") + .setAttribute("key", "string_value") + .setAttribute("key", false) + .startSpan(); + try { + Attributes attributes = span.toSpanData().getAttributes(); + assertThat(attributes.size()).isEqualTo(1); + assertThat(attributes.get(booleanKey("key"))).isEqualTo(false); + assertThat(attributes.get(stringKey("key"))).isNull(); + } finally { + span.end(); + } + } }