From 0da6fab400193bb6835210e7df6528242030f7c6 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Thu, 8 Aug 2024 15:10:27 -0500 Subject: [PATCH 01/10] Optimize advice with FilteredAttributes --- .../api/internal/ImmutableKeyValuePairs.java | 5 + .../sdk/metrics/MetricAdviceBenchmark.java | 151 +++++++++++ .../view/AdviceAttributesProcessor.java | 10 +- .../internal/view/FilteredAttributes.java | 239 +++++++++++++++++ .../metrics/MetricAdviceBenchmarkTest.java | 62 +++++ .../internal/view/FilteredAttributesTest.java | 246 ++++++++++++++++++ 6 files changed, 704 insertions(+), 9 deletions(-) create mode 100644 sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java create mode 100644 sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java create mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java create mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java b/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java index a2590633b83..0b0399f500c 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java @@ -274,4 +274,9 @@ public String toString() { sb.append("}"); return sb.toString(); } + + @SuppressWarnings("AvoidObjectArrays") + public Object[] getData() { + return data; + } } diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java new file mode 100644 index 00000000000..971da6354ac --- /dev/null +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java @@ -0,0 +1,151 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.incubator.metrics.ExtendedLongCounterBuilder; +import io.opentelemetry.api.metrics.LongCounter; +import io.opentelemetry.api.metrics.Meter; +import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader; +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.Level; +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.TearDown; +import org.openjdk.jmh.annotations.Threads; +import org.openjdk.jmh.annotations.Warmup; + +@BenchmarkMode({Mode.AverageTime}) +@OutputTimeUnit(TimeUnit.NANOSECONDS) +@Warmup(iterations = 5, time = 1) +@Measurement(iterations = 10, time = 1) +@Fork(1) +public class MetricAdviceBenchmark { + + private static final Attributes ALL_ATTRIBUTES; + private static final Attributes SOME_ATTRIBUTES; + private static final List> SOME_ATTRIBUTE_KEYS; + + static { + SOME_ATTRIBUTES = + Attributes.builder() + .put("http.request.method", "GET") + .put("http.route", "/v1/users/{id}") + .put("http.response.status_code", 200) + .build(); + ALL_ATTRIBUTES = + SOME_ATTRIBUTES.toBuilder().put("http.url", "http://localhost:8080/v1/users/123").build(); + SOME_ATTRIBUTE_KEYS = new ArrayList<>(SOME_ATTRIBUTES.asMap().keySet()); + } + + @State(Scope.Benchmark) + public static class ThreadState { + + @Param InstrumentParam instrumentParam; + + SdkMeterProvider meterProvider; + + @Setup(Level.Iteration) + public void setup() { + meterProvider = + SdkMeterProvider.builder() + .registerMetricReader(InMemoryMetricReader.createDelta()) + .build(); + Meter meter = meterProvider.get("meter"); + instrumentParam.instrument().setup(meter); + } + + @TearDown + public void tearDown() { + meterProvider.shutdown().join(10, TimeUnit.SECONDS); + } + } + + @Benchmark + @Threads(1) + public void record(ThreadState threadState) { + threadState.instrumentParam.instrument().record(1); + } + + @SuppressWarnings("ImmutableEnumChecker") + public enum InstrumentParam { + NO_ADVICE_RECORD_ALL( + new Instrument() { + private LongCounter counter; + + @Override + void setup(Meter meter) { + counter = ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")).build(); + } + + @Override + void record(long value) { + counter.add(value, ALL_ATTRIBUTES); + } + }), + ADVICE_RECORD_ALL( + new Instrument() { + private LongCounter counter; + + @Override + void setup(Meter meter) { + counter = + ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) + .setAttributesAdvice(SOME_ATTRIBUTE_KEYS) + .build(); + } + + @Override + void record(long value) { + counter.add(value, ALL_ATTRIBUTES); + } + }), + ADVICE_RECORD_SOME( + new Instrument() { + private LongCounter counter; + + @Override + void setup(Meter meter) { + counter = + ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) + .setAttributesAdvice(SOME_ATTRIBUTE_KEYS) + .build(); + } + + @Override + void record(long value) { + counter.add(value, SOME_ATTRIBUTES); + } + }); + + private final Instrument instrument; + + InstrumentParam(Instrument instrument) { + this.instrument = instrument; + } + + Instrument instrument() { + return instrument; + } + } + + private abstract static class Instrument { + abstract void setup(Meter meter); + + abstract void record(long value); + } +} diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java index b0373f25765..f87253f18f3 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java @@ -7,7 +7,6 @@ import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; -import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.context.Context; import java.util.HashSet; import java.util.List; @@ -23,14 +22,7 @@ final class AdviceAttributesProcessor extends AttributesProcessor { @Override public Attributes process(Attributes incoming, Context context) { - // Exit early to avoid allocations if the incoming attributes do not have extra keys to be - // filtered - if (!hasExtraKeys(incoming)) { - return incoming; - } - AttributesBuilder builder = incoming.toBuilder(); - builder.removeIf(key -> !attributeKeys.contains(key)); - return builder.build(); + return FilteredAttributes.create(incoming, attributeKeys); } /** Returns true if {@code attributes} has keys not contained in {@link #attributeKeys}. */ diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java new file mode 100644 index 00000000000..c2819e9d8b8 --- /dev/null +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -0,0 +1,239 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.internal.view; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.api.internal.ImmutableKeyValuePairs; +import java.util.ArrayList; +import java.util.Collections; +import java.util.HashMap; +import java.util.List; +import java.util.Map; +import java.util.Objects; +import java.util.Set; +import java.util.StringJoiner; +import java.util.function.BiConsumer; +import javax.annotation.Nullable; + +@SuppressWarnings("unchecked") +final class FilteredAttributes implements Attributes { + + private final Object[] sourceData; + private final List filteredIndices; + private final Set> keys; + private int hashcode; + + private FilteredAttributes( + ImmutableKeyValuePairs, Object> source, Set> keys) { + this.keys = keys; + this.sourceData = source.getData(); + filteredIndices = new ArrayList<>(source.size() - keys.size()); + for (int i = 0; i < sourceData.length; i += 2) { + if (!keys.contains(sourceData[i])) { + filteredIndices.add(i); + } + } + } + + static Attributes create(Attributes source, Set> keys) { + if (keys.isEmpty()) { + return source; + } + if (!hasExtraKeys(source, keys)) { + return source; + } + if (source instanceof ImmutableKeyValuePairs) { + return new FilteredAttributes((ImmutableKeyValuePairs, Object>) source, keys); + } + AttributesBuilder builder = source.toBuilder(); + builder.removeIf(key -> !keys.contains(key)); + return builder.build(); + } + + /** Returns true if {@code attributes} has keys not contained in {@code keys}. */ + private static boolean hasExtraKeys(Attributes attributes, Set> keys) { + if (attributes.size() > keys.size()) { + return true; + } + boolean[] result = {false}; + attributes.forEach( + (key, value) -> { + if (!result[0] && !keys.contains(key)) { + result[0] = true; + } + }); + return result[0]; + } + + @Nullable + @Override + public T get(AttributeKey key) { + if (key == null) { + return null; + } + for (int i = 0; i < sourceData.length; i += 2) { + if (key.equals(sourceData[i]) && keys.contains(sourceData[i])) { + return (T) sourceData[i + 1]; + } + } + return null; + } + + @Override + public void forEach(BiConsumer, ? super Object> consumer) { + for (int i = 0; i < sourceData.length; i += 2) { + if (keys.contains(sourceData[i])) { + consumer.accept((AttributeKey) sourceData[i], sourceData[i + 1]); + } + } + } + + @Override + public int size() { + return sourceData.length / 2 - filteredIndices.size(); + } + + @Override + public boolean isEmpty() { + return size() == 0; + } + + @Override + public Map, Object> asMap() { + int size = size(); + if (size == 0) { + return Collections.emptyMap(); + } + Map, Object> result = new HashMap<>(size); + for (int i = 0; i < sourceData.length; i += 2) { + if (keys.contains(sourceData[i])) { + result.put((AttributeKey) sourceData[i], sourceData[i + 1]); + } + } + return Collections.unmodifiableMap(result); + } + + @Override + public AttributesBuilder toBuilder() { + AttributesBuilder builder = Attributes.builder(); + int size = size(); + if (size == 0) { + return builder; + } + for (int i = 0; i < sourceData.length; i += 2) { + if (keys.contains(sourceData[i])) { + putInBuilder(builder, (AttributeKey) sourceData[i], sourceData[i + 1]); + } + } + return builder; + } + + private static void putInBuilder(AttributesBuilder builder, AttributeKey key, T value) { + builder.put(key, value); + } + + @Override + public boolean equals(Object object) { + if (this == object) { + return true; + } + if (object == null || getClass() != object.getClass()) { + return false; + } + + // TODO: valid equals implementation when object isn't FilteredAttributes, or ensure no short + // circuiting to other types in FilteredAttributes.create + FilteredAttributes that = (FilteredAttributes) object; + int size = size(); + if (size != that.size()) { + return false; + } else if (size == 0) { + return true; + } + + // compare each non-filtered key / value pair from this to that. + // depends on the entries from the backing ImmutableKeyValuePairs being sorted. + // sacrifice readability for performance because this is on the hotpath + int thisIndex = 0; + int thatIndex = 0; + int thisFilterIndex = 0; + int thisFilterIndexValue = nextFilteredIndex(thisFilterIndex, this); + int thatFilterIndex = 0; + int thatFilterIndexValue = nextFilteredIndex(thatFilterIndex, that); + boolean thisDone; + boolean thatDone; + do { + thisDone = thisIndex >= this.sourceData.length; + thatDone = thatIndex >= that.sourceData.length; + // advance to next unfiltered key value pair for this and that + if (!thisDone && thisFilterIndexValue != -1 && thisIndex == thisFilterIndexValue) { + thisFilterIndexValue = nextFilteredIndex(thisFilterIndex++, this); + thisIndex += 2; + continue; + } + if (!thatDone && thatFilterIndexValue != -1 && thatIndex == thatFilterIndexValue) { + thisFilterIndexValue = nextFilteredIndex(thatFilterIndex++, this); + thatIndex += 2; + continue; + } + // if we're done iterating both this and that, we exit and return true since these are equal + if (thisDone && thatDone) { + break; + } + // if either this or that is done iterating, but not both, these are not equal + if (thisDone != thatDone) { + return false; + } + // if we make it here, both thisIndex and thatIndex are unfiltered and are within the bounds + // of their respective sourceData. the current key and value and this and that must be equal + // for this and that to be equal. + if (!Objects.equals(this.sourceData[thisIndex], that.sourceData[thatIndex]) + || !Objects.equals(this.sourceData[thisIndex + 1], that.sourceData[thatIndex + 1])) { + return false; + } + thisIndex += 2; + thatIndex += 2; + } while (true); + + return true; + } + + private static int nextFilteredIndex(int index, FilteredAttributes filteredAttributes) { + return filteredAttributes.filteredIndices.size() > index + ? filteredAttributes.filteredIndices.get(index) + : -1; + } + + @Override + public int hashCode() { + // memoize the hashcode to avoid comparatively expensive recompute + int result = hashcode; + if (result == 0) { + result = 1; + for (int i = 0; i < sourceData.length; i += 2) { + if (keys.contains(sourceData[i])) { + result = 31 * result + sourceData[i].hashCode(); + result = 31 * result + sourceData[i + 1].hashCode(); + } + } + hashcode = result; + } + return result; + } + + @Override + public String toString() { + StringJoiner joiner = new StringJoiner(",", "FilteredAttributes{", "}"); + for (int i = 0; i < sourceData.length; i += 2) { + if (keys.contains(sourceData[i])) { + joiner.add(((AttributeKey) sourceData[i]).getKey() + "=" + sourceData[i + 1]); + } + } + return joiner.toString(); + } +} diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java new file mode 100644 index 00000000000..a919556325c --- /dev/null +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java @@ -0,0 +1,62 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import io.opentelemetry.api.incubator.metrics.ExtendedLongCounterBuilder; +import io.opentelemetry.api.metrics.LongCounter; +import io.opentelemetry.api.metrics.Meter; +import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader; +import java.util.ArrayList; +import java.util.List; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; + +public class MetricAdviceBenchmarkTest { + + private static final Attributes ALL_ATTRIBUTES; + private static final Attributes SOME_ATTRIBUTES; + private static final List> SOME_ATTRIBUTE_KEYS; + + static { + SOME_ATTRIBUTES = + Attributes.builder() + .put("http.request.method", "GET") + .put("http.route", "/v1/users/{id}") + .put("http.response.status_code", 200) + .build(); + ALL_ATTRIBUTES = + SOME_ATTRIBUTES.toBuilder().put("http.url", "http://localhost:8080/v1/users/123").build(); + SOME_ATTRIBUTE_KEYS = new ArrayList<>(SOME_ATTRIBUTES.asMap().keySet()); + } + + private SdkMeterProvider meterProvider; + private Meter meter; + + @BeforeEach + void setup() { + meterProvider = + SdkMeterProvider.builder().registerMetricReader(InMemoryMetricReader.createDelta()).build(); + meter = meterProvider.get("meter"); + } + + @AfterEach + void tearDown() {} + + @Test + void adviceRecordAll() { + LongCounter counter = + ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) + .setAttributesAdvice(SOME_ATTRIBUTE_KEYS) + .build(); + + for (int i = 0; i < 1_000_000; i++) { + counter.add(1, ALL_ATTRIBUTES); + } + } +} diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java new file mode 100644 index 00000000000..56432cd76ff --- /dev/null +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java @@ -0,0 +1,246 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.sdk.metrics.internal.view; + +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.assertThatThrownBy; +import static org.assertj.core.api.Assertions.entry; + +import com.google.common.collect.ImmutableSet; +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.Attributes; +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.NoSuchElementException; +import org.junit.jupiter.api.Test; + +/** Unit tests for {@link FilteredAttributes}s. */ +@SuppressWarnings("rawtypes") +class FilteredAttributesTest { + + @Test + void forEach() { + Map entriesSeen = new LinkedHashMap<>(); + + Attributes attributes = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), longKey("key2"))); + + attributes.forEach(entriesSeen::put); + + assertThat(entriesSeen) + .containsExactly(entry(stringKey("key1"), "value1"), entry(longKey("key2"), 333L)); + } + + @Test + void forEach_singleAttribute() { + Map entriesSeen = new HashMap<>(); + + Attributes attributes = + FilteredAttributes.create( + Attributes.of(stringKey("key"), "value"), ImmutableSet.of(stringKey("key"))); + attributes.forEach(entriesSeen::put); + assertThat(entriesSeen).containsExactly(entry(stringKey("key"), "value")); + } + + @SuppressWarnings("CollectionIncompatibleType") + @Test + void asMap() { + Attributes attributes = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), longKey("key2"))); + + Map, Object> map = attributes.asMap(); + assertThat(map) + .containsExactly(entry(stringKey("key1"), "value1"), entry(longKey("key2"), 333L)); + + assertThat(map.get(stringKey("key1"))).isEqualTo("value1"); + assertThat(map.get(longKey("key2"))).isEqualTo(333L); + // Map of AttributeKey, not String + assertThat(map.get("key1")).isNull(); + assertThat(map.get(null)).isNull(); + assertThat(map.keySet()).containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); + assertThat(map.values()).containsExactlyInAnyOrder("value1", 333L); + assertThat(map.entrySet()) + .containsExactlyInAnyOrder( + entry(stringKey("key1"), "value1"), entry(longKey("key2"), 333L)); + assertThat(map.entrySet().contains(entry(stringKey("key1"), "value1"))).isTrue(); + assertThat(map.entrySet().contains(entry(stringKey("key1"), "value2"))).isFalse(); + assertThat(map.isEmpty()).isFalse(); + assertThat(map.containsKey(stringKey("key1"))).isTrue(); + assertThat(map.containsKey(longKey("key2"))).isTrue(); + assertThat(map.containsKey(stringKey("key3"))).isFalse(); + assertThat(map.containsKey(null)).isFalse(); + assertThat(map.containsValue("value1")).isTrue(); + assertThat(map.containsValue(333L)).isTrue(); + assertThat(map.containsValue("cat")).isFalse(); + assertThatThrownBy(() -> map.put(stringKey("animal"), "cat")) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.remove(stringKey("key1"))) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.putAll(Collections.emptyMap())) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(map::clear).isInstanceOf(UnsupportedOperationException.class); + + assertThat(map.containsKey(stringKey("key1"))).isTrue(); + assertThat(map.containsKey(stringKey("key3"))).isFalse(); + assertThat(map.keySet().containsAll(Arrays.asList(stringKey("key1"), longKey("key2")))) + .isTrue(); + assertThat(map.keySet().containsAll(Arrays.asList(stringKey("key1"), longKey("key3")))) + .isFalse(); + assertThat(map.keySet().containsAll(Collections.emptyList())).isTrue(); + assertThat(map.keySet().size()).isEqualTo(2); + assertThat(map.keySet().toArray()) + .containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); + AttributeKey[] keys = new AttributeKey[2]; + map.keySet().toArray(keys); + assertThat(keys).containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); + keys = new AttributeKey[0]; + assertThat(map.keySet().toArray(keys)) + .containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); + assertThat(keys).isEmpty(); // Didn't use input array. + assertThatThrownBy(() -> map.keySet().iterator().remove()) + .isInstanceOf(UnsupportedOperationException.class); + assertThat(map.containsKey(stringKey("key1"))).isTrue(); + assertThat(map.keySet().containsAll(Arrays.asList(stringKey("key1"), stringKey("key3")))) + .isFalse(); + assertThat(map.keySet().isEmpty()).isFalse(); + assertThatThrownBy(() -> map.keySet().add(stringKey("key3"))) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.keySet().remove(stringKey("key1"))) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.keySet().add(stringKey("key3"))) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.keySet().retainAll(Collections.singletonList(stringKey("key3")))) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.keySet().removeAll(Collections.singletonList(stringKey("key3")))) + .isInstanceOf(UnsupportedOperationException.class); + assertThatThrownBy(() -> map.keySet().clear()) + .isInstanceOf(UnsupportedOperationException.class); + + assertThat(map.containsValue("value1")).isTrue(); + assertThat(map.containsValue("value3")).isFalse(); + + assertThat(map.toString()).isEqualTo("{key1=value1, key2=333}"); + + Map, Object> emptyMap = Attributes.builder().build().asMap(); + assertThat(emptyMap.isEmpty()).isTrue(); + assertThatThrownBy(() -> emptyMap.entrySet().iterator().next()) + .isInstanceOf(NoSuchElementException.class); + } + + @Test + void equalsAndHashCode() { + Attributes one = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", + stringKey("key2"), "value2", + stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), stringKey("key2"))); + Attributes two = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", + stringKey("key2"), "value2", + stringKey("key3"), "other"), + ImmutableSet.of(stringKey("key1"), stringKey("key2"))); + Attributes three = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", + stringKey("key2"), "value2", + stringKey("key4"), "value4"), + ImmutableSet.of(stringKey("key1"), stringKey("key2"))); + + assertThat(one).isEqualTo(two); + assertThat(one).isEqualTo(three); + assertThat(two).isEqualTo(three); + assertThat(one.hashCode()).isEqualTo(two.hashCode()); + assertThat(two.hashCode()).isEqualTo(three.hashCode()); + + Attributes four = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", + stringKey("key2"), "other"), + ImmutableSet.of(stringKey("key1"), stringKey("key2"))); + Attributes five = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", + stringKey("key2"), "value2", + stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), stringKey("key2"), stringKey("key3"))); + assertThat(one).isNotEqualTo(four); + assertThat(one).isNotEqualTo(five); + assertThat(one.hashCode()).isNotEqualTo(four.hashCode()); + assertThat(one.hashCode()).isNotEqualTo(five.hashCode()); + + assertThat(two).isNotEqualTo(four); + assertThat(two).isNotEqualTo(five); + assertThat(two.hashCode()).isNotEqualTo(four.hashCode()); + assertThat(two.hashCode()).isNotEqualTo(five.hashCode()); + + assertThat(three).isNotEqualTo(four); + assertThat(three).isNotEqualTo(five); + assertThat(three.hashCode()).isNotEqualTo(four.hashCode()); + assertThat(three.hashCode()).isNotEqualTo(five.hashCode()); + } + + @Test + void get_Null() { + Attributes attributes = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), longKey("key2"))); + assertThat(attributes.get(stringKey("foo"))).isNull(); + assertThat(attributes.get(stringKey("key3"))).isNull(); + assertThat(attributes.get(stringKey("key3"))).isNull(); + } + + @Test + void get() { + Attributes attributes = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), longKey("key2"))); + assertThat(attributes.get(stringKey("key1"))).isEqualTo("value1"); + assertThat(attributes.get(longKey("key2"))).isEqualTo(333L); + } + + @Test + void toBuilder() { + Attributes attributes = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), longKey("key2"))); + assertThat(attributes.toBuilder().build()) + .isEqualTo(Attributes.of(stringKey("key1"), "value1", longKey("key2"), 333L)); + } + + @Test + void stringRepresentation() { + Attributes attributes = + FilteredAttributes.create( + Attributes.of( + stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), + ImmutableSet.of(stringKey("key1"), longKey("key2"))); + assertThat(attributes.toString()).isEqualTo("FilteredAttributes{key1=value1,key2=333}"); + } +} From 65fb907502bc139df077bee3d9f9a5453ba97dd0 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Fri, 9 Aug 2024 09:55:18 -0500 Subject: [PATCH 02/10] Use bitset to track filtered indices --- .../internal/view/FilteredAttributes.java | 48 +++++++------------ 1 file changed, 17 insertions(+), 31 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index c2819e9d8b8..7e58fca7d6a 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -9,10 +9,9 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.internal.ImmutableKeyValuePairs; -import java.util.ArrayList; +import java.util.BitSet; import java.util.Collections; -import java.util.HashMap; -import java.util.List; +import java.util.LinkedHashMap; import java.util.Map; import java.util.Objects; import java.util.Set; @@ -24,20 +23,20 @@ final class FilteredAttributes implements Attributes { private final Object[] sourceData; - private final List filteredIndices; - private final Set> keys; + private final BitSet filteredIndices; + private final int size; private int hashcode; private FilteredAttributes( ImmutableKeyValuePairs, Object> source, Set> keys) { - this.keys = keys; this.sourceData = source.getData(); - filteredIndices = new ArrayList<>(source.size() - keys.size()); + this.filteredIndices = new BitSet(source.size()); for (int i = 0; i < sourceData.length; i += 2) { if (!keys.contains(sourceData[i])) { - filteredIndices.add(i); + filteredIndices.set(i / 2); } } + this.size = sourceData.length / 2 - filteredIndices.cardinality(); } static Attributes create(Attributes source, Set> keys) { @@ -77,7 +76,7 @@ public T get(AttributeKey key) { return null; } for (int i = 0; i < sourceData.length; i += 2) { - if (key.equals(sourceData[i]) && keys.contains(sourceData[i])) { + if (key.equals(sourceData[i]) && !filteredIndices.get(i / 2)) { return (T) sourceData[i + 1]; } } @@ -87,7 +86,7 @@ public T get(AttributeKey key) { @Override public void forEach(BiConsumer, ? super Object> consumer) { for (int i = 0; i < sourceData.length; i += 2) { - if (keys.contains(sourceData[i])) { + if (!filteredIndices.get(i / 2)) { consumer.accept((AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -95,7 +94,7 @@ public void forEach(BiConsumer, ? super Object> consumer @Override public int size() { - return sourceData.length / 2 - filteredIndices.size(); + return size; } @Override @@ -109,9 +108,9 @@ public Map, Object> asMap() { if (size == 0) { return Collections.emptyMap(); } - Map, Object> result = new HashMap<>(size); + Map, Object> result = new LinkedHashMap<>(size); for (int i = 0; i < sourceData.length; i += 2) { - if (keys.contains(sourceData[i])) { + if (!filteredIndices.get(i / 2)) { result.put((AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -126,7 +125,7 @@ public AttributesBuilder toBuilder() { return builder; } for (int i = 0; i < sourceData.length; i += 2) { - if (keys.contains(sourceData[i])) { + if (!filteredIndices.get(i / 2)) { putInBuilder(builder, (AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -158,26 +157,19 @@ public boolean equals(Object object) { // compare each non-filtered key / value pair from this to that. // depends on the entries from the backing ImmutableKeyValuePairs being sorted. - // sacrifice readability for performance because this is on the hotpath int thisIndex = 0; int thatIndex = 0; - int thisFilterIndex = 0; - int thisFilterIndexValue = nextFilteredIndex(thisFilterIndex, this); - int thatFilterIndex = 0; - int thatFilterIndexValue = nextFilteredIndex(thatFilterIndex, that); boolean thisDone; boolean thatDone; do { thisDone = thisIndex >= this.sourceData.length; thatDone = thatIndex >= that.sourceData.length; // advance to next unfiltered key value pair for this and that - if (!thisDone && thisFilterIndexValue != -1 && thisIndex == thisFilterIndexValue) { - thisFilterIndexValue = nextFilteredIndex(thisFilterIndex++, this); + if (!thisDone && this.filteredIndices.get(thisIndex / 2)) { thisIndex += 2; continue; } - if (!thatDone && thatFilterIndexValue != -1 && thatIndex == thatFilterIndexValue) { - thisFilterIndexValue = nextFilteredIndex(thatFilterIndex++, this); + if (!thatDone && that.filteredIndices.get(thatIndex / 2)) { thatIndex += 2; continue; } @@ -203,12 +195,6 @@ public boolean equals(Object object) { return true; } - private static int nextFilteredIndex(int index, FilteredAttributes filteredAttributes) { - return filteredAttributes.filteredIndices.size() > index - ? filteredAttributes.filteredIndices.get(index) - : -1; - } - @Override public int hashCode() { // memoize the hashcode to avoid comparatively expensive recompute @@ -216,7 +202,7 @@ public int hashCode() { if (result == 0) { result = 1; for (int i = 0; i < sourceData.length; i += 2) { - if (keys.contains(sourceData[i])) { + if (!filteredIndices.get(i / 2)) { result = 31 * result + sourceData[i].hashCode(); result = 31 * result + sourceData[i + 1].hashCode(); } @@ -230,7 +216,7 @@ public int hashCode() { public String toString() { StringJoiner joiner = new StringJoiner(",", "FilteredAttributes{", "}"); for (int i = 0; i < sourceData.length; i += 2) { - if (keys.contains(sourceData[i])) { + if (!filteredIndices.get(i / 2)) { joiner.add(((AttributeKey) sourceData[i]).getKey() + "=" + sourceData[i + 1]); } } From e227bd2a972913d684b021bffaf7aeced5e03d53 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Fri, 9 Aug 2024 10:03:52 -0500 Subject: [PATCH 03/10] Compute hashcode up front --- .../internal/view/FilteredAttributes.java | 23 ++++++++----------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index 7e58fca7d6a..b0529927fc5 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -25,17 +25,24 @@ final class FilteredAttributes implements Attributes { private final Object[] sourceData; private final BitSet filteredIndices; private final int size; - private int hashcode; + private final int hashcode; private FilteredAttributes( ImmutableKeyValuePairs, Object> source, Set> keys) { this.sourceData = source.getData(); this.filteredIndices = new BitSet(source.size()); + // Record the indices to filter. + // Compute hashcode inline to avoid iteration later + int hashcode = 1; for (int i = 0; i < sourceData.length; i += 2) { if (!keys.contains(sourceData[i])) { filteredIndices.set(i / 2); + } else { + hashcode = 31 * hashcode + sourceData[i].hashCode(); + hashcode = 31 * hashcode + sourceData[i + 1].hashCode(); } } + this.hashcode = hashcode; this.size = sourceData.length / 2 - filteredIndices.cardinality(); } @@ -197,19 +204,7 @@ public boolean equals(Object object) { @Override public int hashCode() { - // memoize the hashcode to avoid comparatively expensive recompute - int result = hashcode; - if (result == 0) { - result = 1; - for (int i = 0; i < sourceData.length; i += 2) { - if (!filteredIndices.get(i / 2)) { - result = 31 * result + sourceData[i].hashCode(); - result = 31 * result + sourceData[i + 1].hashCode(); - } - } - hashcode = result; - } - return result; + return hashcode; } @Override From 6ddf43a2ffa190e30fc0ccfa2210adfbed801aa0 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Fri, 9 Aug 2024 10:49:09 -0500 Subject: [PATCH 04/10] Custom BitSet implementation --- .../internal/view/FilteredAttributes.java | 39 ++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index b0529927fc5..6b70a4b0077 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -9,7 +9,6 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.internal.ImmutableKeyValuePairs; -import java.util.BitSet; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; @@ -217,4 +216,42 @@ public String toString() { } return joiner.toString(); } + + private static class BitSet { + private static final int BITS_PER_WORD = 8; + + private final byte[] words; + + private BitSet(int numBits) { + words = new byte[wordIndex(numBits - 1) + 1]; + } + + private static int wordIndex(int bitIndex) { + return bitIndex / BITS_PER_WORD; + } + + private void set(int bitIndex) { + // TODO check range + int wordIndex = wordIndex(bitIndex); + bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; + int word = words[wordIndex]; + words[wordIndex] = (byte) (word | (1 << bitIndex)); + } + + private boolean get(int bitIndex) { + // TODO check range + int wordIndex = wordIndex(bitIndex); + bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; + int word = words[wordIndex]; + return (word & (1 << bitIndex)) != 0; + } + + private int cardinality() { + int sum = 0; + for (byte word : words) { + sum += Integer.bitCount(word); + } + return sum; + } + } } From 2a2f1f3caca475726d5f8531ad585e3bf7443ed9 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Fri, 9 Aug 2024 17:16:55 -0500 Subject: [PATCH 05/10] Merge bitset into FilteredAttributes to reduce memory, rework benchmarks --- .../sdk/metrics/MetricAdviceBenchmark.java | 108 +++++++++++---- .../internal/view/FilteredAttributes.java | 130 +++++++----------- .../metrics/MetricAdviceBenchmarkTest.java | 82 ++++++++--- 3 files changed, 202 insertions(+), 118 deletions(-) diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java index 971da6354ac..f9610a5fb7f 100644 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java @@ -11,7 +11,7 @@ import io.opentelemetry.api.metrics.LongCounter; import io.opentelemetry.api.metrics.Meter; import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader; -import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import java.util.concurrent.TimeUnit; import org.openjdk.jmh.annotations.Benchmark; @@ -36,20 +36,68 @@ @Fork(1) public class MetricAdviceBenchmark { - private static final Attributes ALL_ATTRIBUTES; - private static final Attributes SOME_ATTRIBUTES; - private static final List> SOME_ATTRIBUTE_KEYS; - - static { - SOME_ATTRIBUTES = - Attributes.builder() - .put("http.request.method", "GET") - .put("http.route", "/v1/users/{id}") - .put("http.response.status_code", 200) - .build(); - ALL_ATTRIBUTES = - SOME_ATTRIBUTES.toBuilder().put("http.url", "http://localhost:8080/v1/users/123").build(); - SOME_ATTRIBUTE_KEYS = new ArrayList<>(SOME_ATTRIBUTES.asMap().keySet()); + static final AttributeKey HTTP_REQUEST_METHOD = + AttributeKey.stringKey("http.request.method"); + static final AttributeKey URL_PATH = AttributeKey.stringKey("url.path"); + static final AttributeKey URL_SCHEME = AttributeKey.stringKey("url.scheme"); + static final AttributeKey HTTP_RESPONSE_STATUS_CODE = + AttributeKey.longKey("http.response.status_code"); + static final AttributeKey HTTP_ROUTE = AttributeKey.stringKey("http.route"); + static final AttributeKey NETWORK_PROTOCOL_NAME = + AttributeKey.stringKey("network.protocol.name"); + static final AttributeKey SERVER_PORT = AttributeKey.longKey("server.port"); + static final AttributeKey URL_QUERY = AttributeKey.stringKey("url.query"); + static final AttributeKey CLIENT_ADDRESS = AttributeKey.stringKey("client.address"); + static final AttributeKey NETWORK_PEER_ADDRESS = + AttributeKey.stringKey("network.peer.address"); + static final AttributeKey NETWORK_PEER_PORT = AttributeKey.longKey("network.peer.port"); + static final AttributeKey NETWORK_PROTOCOL_VERSION = + AttributeKey.stringKey("network.protocol.version"); + static final AttributeKey SERVER_ADDRESS = AttributeKey.stringKey("server.address"); + static final AttributeKey USER_AGENT_ORIGINAL = + AttributeKey.stringKey("user_agent.original"); + + static final List> httpServerMetricAttributeKeys = + Arrays.asList( + HTTP_REQUEST_METHOD, + URL_SCHEME, + HTTP_RESPONSE_STATUS_CODE, + HTTP_ROUTE, + NETWORK_PROTOCOL_NAME, + SERVER_PORT, + NETWORK_PROTOCOL_VERSION, + SERVER_ADDRESS); + + static Attributes httpServerMetricAttributes() { + return Attributes.builder() + .put(HTTP_REQUEST_METHOD, "GET") + .put(URL_SCHEME, "http") + .put(HTTP_RESPONSE_STATUS_CODE, 200) + .put(HTTP_ROUTE, "/v1/users/{id}") + .put(NETWORK_PROTOCOL_NAME, "http") + .put(SERVER_PORT, 8080) + .put(NETWORK_PROTOCOL_VERSION, "1.1") + .put(SERVER_ADDRESS, "localhost") + .build(); + } + + static Attributes httpServerSpanAttributes() { + return Attributes.builder() + .put(HTTP_REQUEST_METHOD, "GET") + .put(URL_PATH, "/v1/users/123") + .put(URL_SCHEME, "http") + .put(HTTP_RESPONSE_STATUS_CODE, 200) + .put(HTTP_ROUTE, "/v1/users/{id}") + .put(NETWORK_PROTOCOL_NAME, "http") + .put(SERVER_PORT, 8080) + .put(URL_QUERY, "with=email") + .put(CLIENT_ADDRESS, "192.168.0.17") + .put(NETWORK_PEER_ADDRESS, "192.168.0.17") + .put(NETWORK_PEER_PORT, 11265) + .put(NETWORK_PROTOCOL_VERSION, "1.1") + .put(SERVER_ADDRESS, "localhost") + .put(USER_AGENT_ORIGINAL, "okhttp/1.27.2") + .build(); } @State(Scope.Benchmark) @@ -83,7 +131,21 @@ public void record(ThreadState threadState) { @SuppressWarnings("ImmutableEnumChecker") public enum InstrumentParam { - NO_ADVICE_RECORD_ALL( + NO_ADVICE_ALL_ATTRIBUTES( + new Instrument() { + private LongCounter counter; + + @Override + void setup(Meter meter) { + counter = ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")).build(); + } + + @Override + void record(long value) { + counter.add(value, httpServerSpanAttributes()); + } + }), + NO_ADVICE_FILTERED_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -94,10 +156,10 @@ void setup(Meter meter) { @Override void record(long value) { - counter.add(value, ALL_ATTRIBUTES); + counter.add(value, httpServerMetricAttributes()); } }), - ADVICE_RECORD_ALL( + ADVICE_ALL_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -105,16 +167,16 @@ void record(long value) { void setup(Meter meter) { counter = ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) - .setAttributesAdvice(SOME_ATTRIBUTE_KEYS) + .setAttributesAdvice(httpServerMetricAttributeKeys) .build(); } @Override void record(long value) { - counter.add(value, ALL_ATTRIBUTES); + counter.add(value, httpServerSpanAttributes()); } }), - ADVICE_RECORD_SOME( + ADVICE_FILTERED_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -122,13 +184,13 @@ void record(long value) { void setup(Meter meter) { counter = ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) - .setAttributesAdvice(SOME_ATTRIBUTE_KEYS) + .setAttributesAdvice(httpServerMetricAttributeKeys) .build(); } @Override void record(long value) { - counter.add(value, SOME_ATTRIBUTES); + counter.add(value, httpServerMetricAttributes()); } }); diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index 6b70a4b0077..30b4ade6993 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -21,58 +21,48 @@ @SuppressWarnings("unchecked") final class FilteredAttributes implements Attributes { + private static final int BITS_PER_WORD = 8; + + private final byte[] words; private final Object[] sourceData; - private final BitSet filteredIndices; - private final int size; private final int hashcode; private FilteredAttributes( ImmutableKeyValuePairs, Object> source, Set> keys) { this.sourceData = source.getData(); - this.filteredIndices = new BitSet(source.size()); + this.words = new byte[wordIndex(source.size() - 1) + 1]; // Record the indices to filter. // Compute hashcode inline to avoid iteration later int hashcode = 1; for (int i = 0; i < sourceData.length; i += 2) { if (!keys.contains(sourceData[i])) { - filteredIndices.set(i / 2); + set(i / 2); } else { hashcode = 31 * hashcode + sourceData[i].hashCode(); hashcode = 31 * hashcode + sourceData[i + 1].hashCode(); } } this.hashcode = hashcode; - this.size = sourceData.length / 2 - filteredIndices.cardinality(); } static Attributes create(Attributes source, Set> keys) { if (keys.isEmpty()) { return source; } - if (!hasExtraKeys(source, keys)) { - return source; - } - if (source instanceof ImmutableKeyValuePairs) { - return new FilteredAttributes((ImmutableKeyValuePairs, Object>) source, keys); + if (!(source instanceof ImmutableKeyValuePairs)) { + // convert alternative implementations of attributes to standard implementation and wrap with + // FilteredAttributes + // this is required for proper funcitoning of equals and hashcode + AttributesBuilder builder = Attributes.builder(); + source.forEach( + (key, value) -> putInBuilder(builder, (AttributeKey) key, value)); + source = builder.build(); } - AttributesBuilder builder = source.toBuilder(); - builder.removeIf(key -> !keys.contains(key)); - return builder.build(); - } - - /** Returns true if {@code attributes} has keys not contained in {@code keys}. */ - private static boolean hasExtraKeys(Attributes attributes, Set> keys) { - if (attributes.size() > keys.size()) { - return true; + if (!(source instanceof ImmutableKeyValuePairs)) { + throw new IllegalStateException( + "Expected ImmutableKeyValuePairs based implementation of Attributes. This is a programming error."); } - boolean[] result = {false}; - attributes.forEach( - (key, value) -> { - if (!result[0] && !keys.contains(key)) { - result[0] = true; - } - }); - return result[0]; + return new FilteredAttributes((ImmutableKeyValuePairs, Object>) source, keys); } @Nullable @@ -82,7 +72,7 @@ public T get(AttributeKey key) { return null; } for (int i = 0; i < sourceData.length; i += 2) { - if (key.equals(sourceData[i]) && !filteredIndices.get(i / 2)) { + if (key.equals(sourceData[i]) && !get(i / 2)) { return (T) sourceData[i + 1]; } } @@ -92,7 +82,7 @@ public T get(AttributeKey key) { @Override public void forEach(BiConsumer, ? super Object> consumer) { for (int i = 0; i < sourceData.length; i += 2) { - if (!filteredIndices.get(i / 2)) { + if (!get(i / 2)) { consumer.accept((AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -100,7 +90,7 @@ public void forEach(BiConsumer, ? super Object> consumer @Override public int size() { - return size; + return sourceData.length / 2 - cardinality(); } @Override @@ -116,7 +106,7 @@ public Map, Object> asMap() { } Map, Object> result = new LinkedHashMap<>(size); for (int i = 0; i < sourceData.length; i += 2) { - if (!filteredIndices.get(i / 2)) { + if (!get(i / 2)) { result.put((AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -131,7 +121,7 @@ public AttributesBuilder toBuilder() { return builder; } for (int i = 0; i < sourceData.length; i += 2) { - if (!filteredIndices.get(i / 2)) { + if (!get(i / 2)) { putInBuilder(builder, (AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -147,22 +137,17 @@ public boolean equals(Object object) { if (this == object) { return true; } + // We require other object to also be instance of FilteredAttributes. In other words, where one + // FilteredAttributes is used for a key in a map, it must be used for all the keys. Note, this + // same requirement exists for the default Attributes implementation - you can not mix + // implementations. if (object == null || getClass() != object.getClass()) { return false; } - // TODO: valid equals implementation when object isn't FilteredAttributes, or ensure no short - // circuiting to other types in FilteredAttributes.create FilteredAttributes that = (FilteredAttributes) object; - int size = size(); - if (size != that.size()) { - return false; - } else if (size == 0) { - return true; - } - - // compare each non-filtered key / value pair from this to that. - // depends on the entries from the backing ImmutableKeyValuePairs being sorted. + // Compare each non-filtered key / value pair from this to that. + // Depends on the entries from the backing ImmutableKeyValuePairs being sorted. int thisIndex = 0; int thatIndex = 0; boolean thisDone; @@ -171,11 +156,11 @@ public boolean equals(Object object) { thisDone = thisIndex >= this.sourceData.length; thatDone = thatIndex >= that.sourceData.length; // advance to next unfiltered key value pair for this and that - if (!thisDone && this.filteredIndices.get(thisIndex / 2)) { + if (!thisDone && this.get(thisIndex / 2)) { thisIndex += 2; continue; } - if (!thatDone && that.filteredIndices.get(thatIndex / 2)) { + if (!thatDone && that.get(thatIndex / 2)) { thatIndex += 2; continue; } @@ -210,48 +195,37 @@ public int hashCode() { public String toString() { StringJoiner joiner = new StringJoiner(",", "FilteredAttributes{", "}"); for (int i = 0; i < sourceData.length; i += 2) { - if (!filteredIndices.get(i / 2)) { + if (!get(i / 2)) { joiner.add(((AttributeKey) sourceData[i]).getKey() + "=" + sourceData[i + 1]); } } return joiner.toString(); } - private static class BitSet { - private static final int BITS_PER_WORD = 8; - - private final byte[] words; - - private BitSet(int numBits) { - words = new byte[wordIndex(numBits - 1) + 1]; - } - - private static int wordIndex(int bitIndex) { - return bitIndex / BITS_PER_WORD; - } + private static int wordIndex(int bitIndex) { + return bitIndex >> BITS_PER_WORD; + } - private void set(int bitIndex) { - // TODO check range - int wordIndex = wordIndex(bitIndex); - bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; - int word = words[wordIndex]; - words[wordIndex] = (byte) (word | (1 << bitIndex)); - } + private void set(int bitIndex) { + // TODO check range + int wordIndex = wordIndex(bitIndex); + bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; + int word = words[wordIndex]; + words[wordIndex] = (byte) (word | (1 << bitIndex)); + } - private boolean get(int bitIndex) { - // TODO check range - int wordIndex = wordIndex(bitIndex); - bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; - int word = words[wordIndex]; - return (word & (1 << bitIndex)) != 0; - } + private boolean get(int bitIndex) { + int wordIndex = wordIndex(bitIndex); + bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; + int word = words[wordIndex]; + return (word & (1 << bitIndex)) != 0; + } - private int cardinality() { - int sum = 0; - for (byte word : words) { - sum += Integer.bitCount(word); - } - return sum; + private int cardinality() { + int sum = 0; + for (byte word : words) { + sum += Integer.bitCount(word); } + return sum; } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java index a919556325c..922c9bb4a5e 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java @@ -11,7 +11,7 @@ import io.opentelemetry.api.metrics.LongCounter; import io.opentelemetry.api.metrics.Meter; import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader; -import java.util.ArrayList; +import java.util.Arrays; import java.util.List; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; @@ -19,20 +19,68 @@ public class MetricAdviceBenchmarkTest { - private static final Attributes ALL_ATTRIBUTES; - private static final Attributes SOME_ATTRIBUTES; - private static final List> SOME_ATTRIBUTE_KEYS; + static final AttributeKey HTTP_REQUEST_METHOD = + AttributeKey.stringKey("http.request.method"); + static final AttributeKey URL_PATH = AttributeKey.stringKey("url.path"); + static final AttributeKey URL_SCHEME = AttributeKey.stringKey("url.scheme"); + static final AttributeKey HTTP_RESPONSE_STATUS_CODE = + AttributeKey.longKey("http.response.status_code"); + static final AttributeKey HTTP_ROUTE = AttributeKey.stringKey("http.route"); + static final AttributeKey NETWORK_PROTOCOL_NAME = + AttributeKey.stringKey("network.protocol.name"); + static final AttributeKey SERVER_PORT = AttributeKey.longKey("server.port"); + static final AttributeKey URL_QUERY = AttributeKey.stringKey("url.query"); + static final AttributeKey CLIENT_ADDRESS = AttributeKey.stringKey("client.address"); + static final AttributeKey NETWORK_PEER_ADDRESS = + AttributeKey.stringKey("network.peer.address"); + static final AttributeKey NETWORK_PEER_PORT = AttributeKey.longKey("network.peer.port"); + static final AttributeKey NETWORK_PROTOCOL_VERSION = + AttributeKey.stringKey("network.protocol.version"); + static final AttributeKey SERVER_ADDRESS = AttributeKey.stringKey("server.address"); + static final AttributeKey USER_AGENT_ORIGINAL = + AttributeKey.stringKey("user_agent.original"); - static { - SOME_ATTRIBUTES = - Attributes.builder() - .put("http.request.method", "GET") - .put("http.route", "/v1/users/{id}") - .put("http.response.status_code", 200) - .build(); - ALL_ATTRIBUTES = - SOME_ATTRIBUTES.toBuilder().put("http.url", "http://localhost:8080/v1/users/123").build(); - SOME_ATTRIBUTE_KEYS = new ArrayList<>(SOME_ATTRIBUTES.asMap().keySet()); + static final List> httpServerMetricAttributeKeys = + Arrays.asList( + HTTP_REQUEST_METHOD, + URL_SCHEME, + HTTP_RESPONSE_STATUS_CODE, + HTTP_ROUTE, + NETWORK_PROTOCOL_NAME, + SERVER_PORT, + NETWORK_PROTOCOL_VERSION, + SERVER_ADDRESS); + + static Attributes httpServerMetricAttributes() { + return Attributes.builder() + .put(HTTP_REQUEST_METHOD, "GET") + .put(URL_SCHEME, "http") + .put(HTTP_RESPONSE_STATUS_CODE, 200) + .put(HTTP_ROUTE, "/v1/users/{id}") + .put(NETWORK_PROTOCOL_NAME, "http") + .put(SERVER_PORT, 8080) + .put(NETWORK_PROTOCOL_VERSION, "1.1") + .put(SERVER_ADDRESS, "localhost") + .build(); + } + + static Attributes httpServerSpanAttributes() { + return Attributes.builder() + .put(HTTP_REQUEST_METHOD, "GET") + .put(URL_PATH, "/v1/users/123") + .put(URL_SCHEME, "http") + .put(HTTP_RESPONSE_STATUS_CODE, 200) + .put(HTTP_ROUTE, "/v1/users/{id}") + .put(NETWORK_PROTOCOL_NAME, "http") + .put(SERVER_PORT, 8080) + .put(URL_QUERY, "with=email") + .put(CLIENT_ADDRESS, "192.168.0.17") + .put(NETWORK_PEER_ADDRESS, "192.168.0.17") + .put(NETWORK_PEER_PORT, 11265) + .put(NETWORK_PROTOCOL_VERSION, "1.1") + .put(SERVER_ADDRESS, "localhost") + .put(USER_AGENT_ORIGINAL, "okhttp/1.27.2") + .build(); } private SdkMeterProvider meterProvider; @@ -49,14 +97,14 @@ void setup() { void tearDown() {} @Test - void adviceRecordAll() { + void adviceAllAttributes() { LongCounter counter = ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) - .setAttributesAdvice(SOME_ATTRIBUTE_KEYS) + .setAttributesAdvice(httpServerMetricAttributeKeys) .build(); for (int i = 0; i < 1_000_000; i++) { - counter.add(1, ALL_ATTRIBUTES); + counter.add(1, httpServerSpanAttributes()); } } } From 56a677858fe29f6844f87c340644011c546744ef Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Sun, 11 Aug 2024 11:39:12 -0500 Subject: [PATCH 06/10] Encode filtered attributes into int --- .../sdk/metrics/MetricAdviceBenchmark.java | 33 +++++++++++++ .../internal/view/FilteredAttributes.java | 47 +++++++++---------- .../metrics/MetricAdviceBenchmarkTest.java | 13 +++++ 3 files changed, 68 insertions(+), 25 deletions(-) diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java index f9610a5fb7f..eafbcc56131 100644 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java @@ -100,6 +100,8 @@ static Attributes httpServerSpanAttributes() { .build(); } + static final Attributes CACHED_HTTP_SERVER_SPAN_ATTRIBUTES = httpServerSpanAttributes(); + @State(Scope.Benchmark) public static class ThreadState { @@ -159,6 +161,20 @@ void record(long value) { counter.add(value, httpServerMetricAttributes()); } }), + NO_ADVICE_ALL_ATTRIBUTES_CACHED( + new Instrument() { + private LongCounter counter; + + @Override + void setup(Meter meter) { + counter = ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")).build(); + } + + @Override + void record(long value) { + counter.add(value, CACHED_HTTP_SERVER_SPAN_ATTRIBUTES); + } + }), ADVICE_ALL_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -192,6 +208,23 @@ void setup(Meter meter) { void record(long value) { counter.add(value, httpServerMetricAttributes()); } + }), + ADVICE_ALL_ATTRIBUTES_CACHED( + new Instrument() { + private LongCounter counter; + + @Override + void setup(Meter meter) { + counter = + ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) + .setAttributesAdvice(httpServerMetricAttributeKeys) + .build(); + } + + @Override + void record(long value) { + counter.add(value, CACHED_HTTP_SERVER_SPAN_ATTRIBUTES); + } }); private final Instrument instrument; diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index 30b4ade6993..dc300154931 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -9,6 +9,7 @@ import io.opentelemetry.api.common.Attributes; import io.opentelemetry.api.common.AttributesBuilder; import io.opentelemetry.api.internal.ImmutableKeyValuePairs; +import java.util.BitSet; import java.util.Collections; import java.util.LinkedHashMap; import java.util.Map; @@ -21,27 +22,37 @@ @SuppressWarnings("unchecked") final class FilteredAttributes implements Attributes { - private static final int BITS_PER_WORD = 8; + private static final int BITS_PER_INTEGER = 32; - private final byte[] words; + private final int filteredIndices; + @Nullable private final BitSet overflowFilteredIndices; private final Object[] sourceData; private final int hashcode; + @SuppressWarnings("NullAway") private FilteredAttributes( ImmutableKeyValuePairs, Object> source, Set> keys) { this.sourceData = source.getData(); - this.words = new byte[wordIndex(source.size() - 1) + 1]; + int filteredIndices = 0; + this.overflowFilteredIndices = + source.size() > BITS_PER_INTEGER ? new BitSet(source.size() - BITS_PER_INTEGER) : null; // Record the indices to filter. // Compute hashcode inline to avoid iteration later int hashcode = 1; for (int i = 0; i < sourceData.length; i += 2) { + int filterIndex = i / 2; if (!keys.contains(sourceData[i])) { - set(i / 2); + if (filterIndex < BITS_PER_INTEGER) { + filteredIndices = filteredIndices | (1 << filterIndex); + } else { + overflowFilteredIndices.set(filterIndex - BITS_PER_INTEGER); + } } else { hashcode = 31 * hashcode + sourceData[i].hashCode(); hashcode = 31 * hashcode + sourceData[i + 1].hashCode(); } } + this.filteredIndices = filteredIndices; this.hashcode = hashcode; } @@ -202,30 +213,16 @@ public String toString() { return joiner.toString(); } - private static int wordIndex(int bitIndex) { - return bitIndex >> BITS_PER_WORD; - } - - private void set(int bitIndex) { - // TODO check range - int wordIndex = wordIndex(bitIndex); - bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; - int word = words[wordIndex]; - words[wordIndex] = (byte) (word | (1 << bitIndex)); - } - + @SuppressWarnings("NullAway") private boolean get(int bitIndex) { - int wordIndex = wordIndex(bitIndex); - bitIndex = wordIndex == 0 ? bitIndex : bitIndex % wordIndex; - int word = words[wordIndex]; - return (word & (1 << bitIndex)) != 0; + if (bitIndex < BITS_PER_INTEGER) { + return (filteredIndices & (1 << bitIndex)) != 0; + } + return overflowFilteredIndices.get(bitIndex - BITS_PER_INTEGER); } private int cardinality() { - int sum = 0; - for (byte word : words) { - sum += Integer.bitCount(word); - } - return sum; + return Integer.bitCount(filteredIndices) + + (overflowFilteredIndices == null ? 0 : overflowFilteredIndices.cardinality()); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java index 922c9bb4a5e..397e5bbcbc5 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java @@ -107,4 +107,17 @@ void adviceAllAttributes() { counter.add(1, httpServerSpanAttributes()); } } + + @Test + void adviceAllAttributesCached() { + Attributes attributes = httpServerSpanAttributes(); + LongCounter counter = + ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) + .setAttributesAdvice(httpServerMetricAttributeKeys) + .build(); + + for (int i = 0; i < 1_000_000; i++) { + counter.add(1, attributes); + } + } } From 2a9ad4b24db5071e9c8c909ee3871f5f7bf82db2 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Mon, 12 Aug 2024 12:56:30 -0500 Subject: [PATCH 07/10] clean up --- sdk/metrics/build.gradle.kts | 1 + .../view/AdviceAttributesProcessor.java | 12 - .../internal/view/FilteredAttributes.java | 155 +++++--- .../internal/view/FilteredAttributesTest.java | 352 ++++++++---------- 4 files changed, 246 insertions(+), 274 deletions(-) diff --git a/sdk/metrics/build.gradle.kts b/sdk/metrics/build.gradle.kts index ef2dd8244a5..0fffd03457e 100644 --- a/sdk/metrics/build.gradle.kts +++ b/sdk/metrics/build.gradle.kts @@ -25,6 +25,7 @@ dependencies { testImplementation(project(":sdk:testing")) testImplementation("com.google.guava:guava") + testImplementation("com.google.guava:guava-testlib") jmh(project(":sdk:trace")) jmh(project(":sdk:testing")) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java index f87253f18f3..8b77b48e631 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/AdviceAttributesProcessor.java @@ -25,18 +25,6 @@ public Attributes process(Attributes incoming, Context context) { return FilteredAttributes.create(incoming, attributeKeys); } - /** Returns true if {@code attributes} has keys not contained in {@link #attributeKeys}. */ - private boolean hasExtraKeys(Attributes attributes) { - boolean[] result = {false}; - attributes.forEach( - (key, value) -> { - if (!result[0] && !attributeKeys.contains(key)) { - result[0] = true; - } - }); - return result[0]; - } - @Override public boolean usesContext() { return false; diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index dc300154931..0392a90ab23 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -19,61 +19,104 @@ import java.util.function.BiConsumer; import javax.annotation.Nullable; +/** + * Filtered attributes is a filtered view of a {@link ImmutableKeyValuePairs} backed {@link + * Attributes} instance. Rather than creating an entirely new attributes instance, it keeps track of + * which source attributes are excluded while implementing the {@link Attributes} interface. + * + *

Notably, the {@link FilteredAttributes#equals(Object)} and {@link + * FilteredAttributes#hashCode()} depend on comparison against other {@link FilteredAttributes} + * instances. This means that where {@link FilteredAttributes} is used for things like map keys, it + * must be used for all keys in that map. You cannot mix {@link Attributes} implementations. This is + * also true for the default attributes implementation. + */ @SuppressWarnings("unchecked") final class FilteredAttributes implements Attributes { private static final int BITS_PER_INTEGER = 32; + // Backing source data from ImmutableKeyValuePairs.data. This array MUST NOT be mutated. + private final Object[] sourceData; + // The bits of filteredIndices track whether the first 32 key-values of sourceData should be + // excluded in the output. A bit equal to 1 indicates the sourceData key at i*2 should be + // excluded. overflowFilteredIndices is used when more than 32 key-value pairs are present in + // sourceData. private final int filteredIndices; @Nullable private final BitSet overflowFilteredIndices; - private final Object[] sourceData; private final int hashcode; + private final int size; - @SuppressWarnings("NullAway") private FilteredAttributes( - ImmutableKeyValuePairs, Object> source, Set> keys) { - this.sourceData = source.getData(); + Object[] sourceData, + int filteredIndices, + @Nullable BitSet overflowFilteredIndices, + int hashcode, + int size) { + this.sourceData = sourceData; + this.filteredIndices = filteredIndices; + this.overflowFilteredIndices = overflowFilteredIndices; + this.hashcode = hashcode; + this.size = size; + } + + /** + * Create a {@link FilteredAttributes} instance. + * + * @param source the source attributes, which SHOULD be based on the standard {@link + * ImmutableKeyValuePairs}. If not, the source will first be converted to the standard + * implementation. + * @param includedKeys the set of attribute keys to include in the output. + */ + @SuppressWarnings("NullAway") + static Attributes create(Attributes source, Set> includedKeys) { + // Convert alternative implementations of Attributes to standard implementation. + // This is required for proper functioning of equals and hashcode. + if (!(source instanceof ImmutableKeyValuePairs)) { + source = convertToStandardImplementation(source); + } + if (!(source instanceof ImmutableKeyValuePairs)) { + throw new IllegalStateException( + "Expected ImmutableKeyValuePairs based implementation of Attributes. This is a programming error."); + } + // Compute filteredIndices (and overflowIndices if needed) during initialization. Compute + // hashcode at the same time to avoid iteration later. + Object[] sourceData = ((ImmutableKeyValuePairs) source).getData(); int filteredIndices = 0; - this.overflowFilteredIndices = + BitSet overflowFilteredIndices = source.size() > BITS_PER_INTEGER ? new BitSet(source.size() - BITS_PER_INTEGER) : null; - // Record the indices to filter. - // Compute hashcode inline to avoid iteration later int hashcode = 1; + int size = 0; for (int i = 0; i < sourceData.length; i += 2) { int filterIndex = i / 2; - if (!keys.contains(sourceData[i])) { + // If the sourceData key isn't present in includedKeys, record the exclusion in + // filteredIndices (or + // overflowFilteredIndices based on the index). + if (!includedKeys.contains(sourceData[i])) { + // Record if (filterIndex < BITS_PER_INTEGER) { filteredIndices = filteredIndices | (1 << filterIndex); } else { overflowFilteredIndices.set(filterIndex - BITS_PER_INTEGER); } - } else { + } else { // The key-value is included in the output, record in the hashcode and size. hashcode = 31 * hashcode + sourceData[i].hashCode(); hashcode = 31 * hashcode + sourceData[i + 1].hashCode(); + size++; } } - this.filteredIndices = filteredIndices; - this.hashcode = hashcode; + // If size is 0, short circuit and return Attributes.empty() + if (size == 0) { + return Attributes.empty(); + } + return new FilteredAttributes( + sourceData, filteredIndices, overflowFilteredIndices, hashcode, size); } - static Attributes create(Attributes source, Set> keys) { - if (keys.isEmpty()) { - return source; - } - if (!(source instanceof ImmutableKeyValuePairs)) { - // convert alternative implementations of attributes to standard implementation and wrap with - // FilteredAttributes - // this is required for proper funcitoning of equals and hashcode - AttributesBuilder builder = Attributes.builder(); - source.forEach( - (key, value) -> putInBuilder(builder, (AttributeKey) key, value)); - source = builder.build(); - } - if (!(source instanceof ImmutableKeyValuePairs)) { - throw new IllegalStateException( - "Expected ImmutableKeyValuePairs based implementation of Attributes. This is a programming error."); - } - return new FilteredAttributes((ImmutableKeyValuePairs, Object>) source, keys); + private static Attributes convertToStandardImplementation(Attributes source) { + AttributesBuilder builder = Attributes.builder(); + source.forEach( + (key, value) -> putInBuilder(builder, (AttributeKey) key, value)); + return builder.build(); } @Nullable @@ -83,7 +126,7 @@ public T get(AttributeKey key) { return null; } for (int i = 0; i < sourceData.length; i += 2) { - if (key.equals(sourceData[i]) && !get(i / 2)) { + if (key.equals(sourceData[i]) && includeIndexInOutput(i)) { return (T) sourceData[i + 1]; } } @@ -93,7 +136,7 @@ public T get(AttributeKey key) { @Override public void forEach(BiConsumer, ? super Object> consumer) { for (int i = 0; i < sourceData.length; i += 2) { - if (!get(i / 2)) { + if (includeIndexInOutput(i)) { consumer.accept((AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -101,23 +144,21 @@ public void forEach(BiConsumer, ? super Object> consumer @Override public int size() { - return sourceData.length / 2 - cardinality(); + return size; } @Override public boolean isEmpty() { - return size() == 0; + // #create short circuits and returns Attributes.empty() if empty, so FilteredAttributes is + // never empty + return false; } @Override public Map, Object> asMap() { - int size = size(); - if (size == 0) { - return Collections.emptyMap(); - } Map, Object> result = new LinkedHashMap<>(size); for (int i = 0; i < sourceData.length; i += 2) { - if (!get(i / 2)) { + if (includeIndexInOutput(i)) { result.put((AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -127,12 +168,8 @@ public Map, Object> asMap() { @Override public AttributesBuilder toBuilder() { AttributesBuilder builder = Attributes.builder(); - int size = size(); - if (size == 0) { - return builder; - } for (int i = 0; i < sourceData.length; i += 2) { - if (!get(i / 2)) { + if (includeIndexInOutput(i)) { putInBuilder(builder, (AttributeKey) sourceData[i], sourceData[i + 1]); } } @@ -148,7 +185,7 @@ public boolean equals(Object object) { if (this == object) { return true; } - // We require other object to also be instance of FilteredAttributes. In other words, where one + // We require other object to also be instances of FilteredAttributes. In other words, where one // FilteredAttributes is used for a key in a map, it must be used for all the keys. Note, this // same requirement exists for the default Attributes implementation - you can not mix // implementations. @@ -157,6 +194,10 @@ public boolean equals(Object object) { } FilteredAttributes that = (FilteredAttributes) object; + // exit early if sizes are not equal + if (size() != that.size()) { + return false; + } // Compare each non-filtered key / value pair from this to that. // Depends on the entries from the backing ImmutableKeyValuePairs being sorted. int thisIndex = 0; @@ -167,11 +208,11 @@ public boolean equals(Object object) { thisDone = thisIndex >= this.sourceData.length; thatDone = thatIndex >= that.sourceData.length; // advance to next unfiltered key value pair for this and that - if (!thisDone && this.get(thisIndex / 2)) { + if (!thisDone && !this.includeIndexInOutput(thisIndex)) { thisIndex += 2; continue; } - if (!thatDone && that.get(thatIndex / 2)) { + if (!thatDone && !that.includeIndexInOutput(thatIndex)) { thatIndex += 2; continue; } @@ -183,9 +224,9 @@ public boolean equals(Object object) { if (thisDone != thatDone) { return false; } - // if we make it here, both thisIndex and thatIndex are unfiltered and are within the bounds - // of their respective sourceData. the current key and value and this and that must be equal - // for this and that to be equal. + // if we make it here, both thisIndex and thatIndex within bounds and are included in the + // output. the current + // key and value and this and that must be equal for this and that to be equal. if (!Objects.equals(this.sourceData[thisIndex], that.sourceData[thatIndex]) || !Objects.equals(this.sourceData[thisIndex + 1], that.sourceData[thatIndex + 1])) { return false; @@ -193,7 +234,7 @@ public boolean equals(Object object) { thisIndex += 2; thatIndex += 2; } while (true); - + // if we make it here without exiting early, all elements of this and that are equal return true; } @@ -206,7 +247,7 @@ public int hashCode() { public String toString() { StringJoiner joiner = new StringJoiner(",", "FilteredAttributes{", "}"); for (int i = 0; i < sourceData.length; i += 2) { - if (!get(i / 2)) { + if (includeIndexInOutput(i)) { joiner.add(((AttributeKey) sourceData[i]).getKey() + "=" + sourceData[i + 1]); } } @@ -214,15 +255,11 @@ public String toString() { } @SuppressWarnings("NullAway") - private boolean get(int bitIndex) { + private boolean includeIndexInOutput(int sourceIndex) { + int bitIndex = sourceIndex / 2; if (bitIndex < BITS_PER_INTEGER) { - return (filteredIndices & (1 << bitIndex)) != 0; + return (filteredIndices & (1 << bitIndex)) == 0; } - return overflowFilteredIndices.get(bitIndex - BITS_PER_INTEGER); - } - - private int cardinality() { - return Integer.bitCount(filteredIndices) - + (overflowFilteredIndices == null ? 0 : overflowFilteredIndices.cardinality()); + return !overflowFilteredIndices.get(bitIndex - BITS_PER_INTEGER); } } diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java index 56432cd76ff..b8bc0d10932 100644 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java +++ b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributesTest.java @@ -8,239 +8,185 @@ 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.assertThatThrownBy; -import static org.assertj.core.api.Assertions.entry; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.ImmutableSet; +import com.google.common.testing.EqualsTester; import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.Attributes; -import java.util.Arrays; +import io.opentelemetry.api.common.AttributesBuilder; import java.util.Collections; import java.util.HashMap; -import java.util.LinkedHashMap; +import java.util.HashSet; import java.util.Map; -import java.util.NoSuchElementException; +import java.util.Set; +import java.util.stream.Collectors; +import java.util.stream.IntStream; +import java.util.stream.Stream; +import org.junit.jupiter.api.RepeatedTest; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; /** Unit tests for {@link FilteredAttributes}s. */ @SuppressWarnings("rawtypes") class FilteredAttributesTest { - @Test - void forEach() { - Map entriesSeen = new LinkedHashMap<>(); - - Attributes attributes = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), longKey("key2"))); - - attributes.forEach(entriesSeen::put); + private static final AttributeKey KEY1 = stringKey("key1"); + private static final AttributeKey KEY2 = stringKey("key2"); + private static final AttributeKey KEY3 = stringKey("key3"); + private static final AttributeKey KEY4 = stringKey("key4"); + private static final AttributeKey KEY2_LONG = longKey("key2"); + private static final Set> ALL_KEYS = + ImmutableSet.of(KEY1, KEY2, KEY3, KEY4, KEY2_LONG); + private static final Attributes ALL_ATTRIBUTES = + Attributes.of(KEY1, "value1", KEY2, "value2", KEY2_LONG, 222L, KEY3, "value3"); + private static final Attributes FILTERED_ATTRIBUTES_ONE = + FilteredAttributes.create(ALL_ATTRIBUTES, ImmutableSet.of(KEY1)); + private static final Attributes FILTERED_ATTRIBUTES_TWO = + FilteredAttributes.create(ALL_ATTRIBUTES, ImmutableSet.of(KEY1, KEY2_LONG)); + private static final Attributes FILTERED_ATTRIBUTES_THREE = + FilteredAttributes.create(ALL_ATTRIBUTES, ImmutableSet.of(KEY1, KEY2_LONG, KEY3)); + private static final Attributes FILTERED_ATTRIBUTES_FOUR = + FilteredAttributes.create(ALL_ATTRIBUTES, ImmutableSet.of(KEY1, KEY2_LONG, KEY3, KEY4)); + private static final Attributes FILTERED_ATTRIBUTES_EMPTY_SOURCE = + FilteredAttributes.create(Attributes.empty(), ImmutableSet.of(KEY1)); + private static final Attributes FILTERED_ATTRIBUTES_EMPTY = + FilteredAttributes.create(ALL_ATTRIBUTES, Collections.emptySet()); + + @ParameterizedTest + @MethodSource("mapArgs") + void forEach(Attributes filteredAttributes, Map, Object> expectedMapEntries) { + Map entriesSeen = new HashMap<>(); + filteredAttributes.forEach(entriesSeen::put); + assertThat(entriesSeen).isEqualTo(expectedMapEntries); + } - assertThat(entriesSeen) - .containsExactly(entry(stringKey("key1"), "value1"), entry(longKey("key2"), 333L)); + @ParameterizedTest + @MethodSource("mapArgs") + void asMap(Attributes filteredAttributes, Map, Object> expectedMapEntries) { + assertThat(filteredAttributes.asMap()).isEqualTo(expectedMapEntries); } - @Test - void forEach_singleAttribute() { - Map entriesSeen = new HashMap<>(); + @ParameterizedTest + @MethodSource("mapArgs") + void size(Attributes filteredAttributes, Map, Object> expectedMapEntries) { + assertThat(filteredAttributes.size()).isEqualTo(expectedMapEntries.size()); + } - Attributes attributes = - FilteredAttributes.create( - Attributes.of(stringKey("key"), "value"), ImmutableSet.of(stringKey("key"))); - attributes.forEach(entriesSeen::put); - assertThat(entriesSeen).containsExactly(entry(stringKey("key"), "value")); + @ParameterizedTest + @MethodSource("mapArgs") + void isEmpty(Attributes filteredAttributes, Map, Object> expectedMapEntries) { + assertThat(filteredAttributes.isEmpty()).isEqualTo(expectedMapEntries.isEmpty()); } - @SuppressWarnings("CollectionIncompatibleType") - @Test - void asMap() { - Attributes attributes = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), longKey("key2"))); - - Map, Object> map = attributes.asMap(); - assertThat(map) - .containsExactly(entry(stringKey("key1"), "value1"), entry(longKey("key2"), 333L)); - - assertThat(map.get(stringKey("key1"))).isEqualTo("value1"); - assertThat(map.get(longKey("key2"))).isEqualTo(333L); - // Map of AttributeKey, not String - assertThat(map.get("key1")).isNull(); - assertThat(map.get(null)).isNull(); - assertThat(map.keySet()).containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); - assertThat(map.values()).containsExactlyInAnyOrder("value1", 333L); - assertThat(map.entrySet()) - .containsExactlyInAnyOrder( - entry(stringKey("key1"), "value1"), entry(longKey("key2"), 333L)); - assertThat(map.entrySet().contains(entry(stringKey("key1"), "value1"))).isTrue(); - assertThat(map.entrySet().contains(entry(stringKey("key1"), "value2"))).isFalse(); - assertThat(map.isEmpty()).isFalse(); - assertThat(map.containsKey(stringKey("key1"))).isTrue(); - assertThat(map.containsKey(longKey("key2"))).isTrue(); - assertThat(map.containsKey(stringKey("key3"))).isFalse(); - assertThat(map.containsKey(null)).isFalse(); - assertThat(map.containsValue("value1")).isTrue(); - assertThat(map.containsValue(333L)).isTrue(); - assertThat(map.containsValue("cat")).isFalse(); - assertThatThrownBy(() -> map.put(stringKey("animal"), "cat")) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.remove(stringKey("key1"))) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.putAll(Collections.emptyMap())) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(map::clear).isInstanceOf(UnsupportedOperationException.class); - - assertThat(map.containsKey(stringKey("key1"))).isTrue(); - assertThat(map.containsKey(stringKey("key3"))).isFalse(); - assertThat(map.keySet().containsAll(Arrays.asList(stringKey("key1"), longKey("key2")))) - .isTrue(); - assertThat(map.keySet().containsAll(Arrays.asList(stringKey("key1"), longKey("key3")))) - .isFalse(); - assertThat(map.keySet().containsAll(Collections.emptyList())).isTrue(); - assertThat(map.keySet().size()).isEqualTo(2); - assertThat(map.keySet().toArray()) - .containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); - AttributeKey[] keys = new AttributeKey[2]; - map.keySet().toArray(keys); - assertThat(keys).containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); - keys = new AttributeKey[0]; - assertThat(map.keySet().toArray(keys)) - .containsExactlyInAnyOrder(stringKey("key1"), longKey("key2")); - assertThat(keys).isEmpty(); // Didn't use input array. - assertThatThrownBy(() -> map.keySet().iterator().remove()) - .isInstanceOf(UnsupportedOperationException.class); - assertThat(map.containsKey(stringKey("key1"))).isTrue(); - assertThat(map.keySet().containsAll(Arrays.asList(stringKey("key1"), stringKey("key3")))) - .isFalse(); - assertThat(map.keySet().isEmpty()).isFalse(); - assertThatThrownBy(() -> map.keySet().add(stringKey("key3"))) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.keySet().remove(stringKey("key1"))) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.keySet().add(stringKey("key3"))) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.keySet().retainAll(Collections.singletonList(stringKey("key3")))) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.keySet().removeAll(Collections.singletonList(stringKey("key3")))) - .isInstanceOf(UnsupportedOperationException.class); - assertThatThrownBy(() -> map.keySet().clear()) - .isInstanceOf(UnsupportedOperationException.class); - - assertThat(map.containsValue("value1")).isTrue(); - assertThat(map.containsValue("value3")).isFalse(); - - assertThat(map.toString()).isEqualTo("{key1=value1, key2=333}"); - - Map, Object> emptyMap = Attributes.builder().build().asMap(); - assertThat(emptyMap.isEmpty()).isTrue(); - assertThatThrownBy(() -> emptyMap.entrySet().iterator().next()) - .isInstanceOf(NoSuchElementException.class); + @ParameterizedTest + @MethodSource("mapArgs") + void get(Attributes filteredAttributes, Map, Object> expectedMapEntries) { + for (AttributeKey key : ALL_KEYS) { + Object expectedValue = expectedMapEntries.get(key); + assertThat(filteredAttributes.get(key)).isEqualTo(expectedValue); + } } - @Test - void equalsAndHashCode() { - Attributes one = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", - stringKey("key2"), "value2", - stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), stringKey("key2"))); - Attributes two = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", - stringKey("key2"), "value2", - stringKey("key3"), "other"), - ImmutableSet.of(stringKey("key1"), stringKey("key2"))); - Attributes three = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", - stringKey("key2"), "value2", - stringKey("key4"), "value4"), - ImmutableSet.of(stringKey("key1"), stringKey("key2"))); - - assertThat(one).isEqualTo(two); - assertThat(one).isEqualTo(three); - assertThat(two).isEqualTo(three); - assertThat(one.hashCode()).isEqualTo(two.hashCode()); - assertThat(two.hashCode()).isEqualTo(three.hashCode()); - - Attributes four = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", - stringKey("key2"), "other"), - ImmutableSet.of(stringKey("key1"), stringKey("key2"))); - Attributes five = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", - stringKey("key2"), "value2", - stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), stringKey("key2"), stringKey("key3"))); - assertThat(one).isNotEqualTo(four); - assertThat(one).isNotEqualTo(five); - assertThat(one.hashCode()).isNotEqualTo(four.hashCode()); - assertThat(one.hashCode()).isNotEqualTo(five.hashCode()); - - assertThat(two).isNotEqualTo(four); - assertThat(two).isNotEqualTo(five); - assertThat(two.hashCode()).isNotEqualTo(four.hashCode()); - assertThat(two.hashCode()).isNotEqualTo(five.hashCode()); - - assertThat(three).isNotEqualTo(four); - assertThat(three).isNotEqualTo(five); - assertThat(three.hashCode()).isNotEqualTo(four.hashCode()); - assertThat(three.hashCode()).isNotEqualTo(five.hashCode()); + @ParameterizedTest + @MethodSource("mapArgs") + void toBuilder(Attributes filteredAttributes, Map, Object> expectedMapEntries) { + Attributes attributes = filteredAttributes.toBuilder().build(); + assertThat(attributes.asMap()).isEqualTo(expectedMapEntries); } - @Test - void get_Null() { - Attributes attributes = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), longKey("key2"))); - assertThat(attributes.get(stringKey("foo"))).isNull(); - assertThat(attributes.get(stringKey("key3"))).isNull(); - assertThat(attributes.get(stringKey("key3"))).isNull(); + private static Stream mapArgs() { + return Stream.of( + Arguments.of(FILTERED_ATTRIBUTES_ONE, ImmutableMap.of(KEY1, "value1")), + Arguments.of(FILTERED_ATTRIBUTES_TWO, ImmutableMap.of(KEY1, "value1", KEY2_LONG, 222L)), + Arguments.of( + FILTERED_ATTRIBUTES_THREE, + ImmutableMap.of(KEY1, "value1", KEY2_LONG, 222L, KEY3, "value3")), + Arguments.of( + FILTERED_ATTRIBUTES_FOUR, + ImmutableMap.of(KEY1, "value1", KEY2_LONG, 222L, KEY3, "value3")), + Arguments.of(FILTERED_ATTRIBUTES_EMPTY_SOURCE, Collections.emptyMap()), + Arguments.of(FILTERED_ATTRIBUTES_EMPTY, Collections.emptyMap())); } @Test - void get() { - Attributes attributes = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), longKey("key2"))); - assertThat(attributes.get(stringKey("key1"))).isEqualTo("value1"); - assertThat(attributes.get(longKey("key2"))).isEqualTo(333L); + void stringRepresentation() { + assertThat(FILTERED_ATTRIBUTES_ONE.toString()).isEqualTo("FilteredAttributes{key1=value1}"); + assertThat(FILTERED_ATTRIBUTES_TWO.toString()) + .isEqualTo("FilteredAttributes{key1=value1,key2=222}"); + assertThat(FILTERED_ATTRIBUTES_THREE.toString()) + .isEqualTo("FilteredAttributes{key1=value1,key2=222,key3=value3}"); + assertThat(FILTERED_ATTRIBUTES_FOUR.toString()) + .isEqualTo("FilteredAttributes{key1=value1,key2=222,key3=value3}"); + assertThat(FILTERED_ATTRIBUTES_EMPTY_SOURCE.toString()).isEqualTo("{}"); + assertThat(FILTERED_ATTRIBUTES_EMPTY.toString()).isEqualTo("{}"); } - @Test - void toBuilder() { - Attributes attributes = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), longKey("key2"))); - assertThat(attributes.toBuilder().build()) - .isEqualTo(Attributes.of(stringKey("key1"), "value1", longKey("key2"), 333L)); + /** + * Test behavior of attributes with more than the 32 limit of FilteredAttributes.filteredIndices. + */ + @RepeatedTest(10) + void largeAttributes() { + Set> allKeys = new HashSet<>(); + AttributesBuilder allAttributesBuilder = Attributes.builder(); + IntStream.range(0, 100) + .forEach( + i -> { + AttributeKey key = stringKey("key" + i); + allKeys.add(key); + allAttributesBuilder.put(key, "value" + i); + }); + Attributes allAttributes = allAttributesBuilder.build(); + + Attributes empty = FilteredAttributes.create(allAttributes, Collections.emptySet()); + assertThat(empty.size()).isEqualTo(0); + assertThat(empty.isEmpty()).isTrue(); + + Set> oneKey = allKeys.stream().limit(1).collect(Collectors.toSet()); + Attributes one = FilteredAttributes.create(allAttributes, oneKey); + assertThat(one.size()).isEqualTo(1); + assertThat(one.isEmpty()).isFalse(); + allKeys.stream() + .forEach( + key -> { + if (oneKey.contains(key)) { + assertThat(one.get(key)).isNotNull(); + } else { + assertThat(one.get(key)).isNull(); + } + }); + + Set> tenKeys = allKeys.stream().limit(10).collect(Collectors.toSet()); + Attributes ten = FilteredAttributes.create(allAttributes, tenKeys); + assertThat(ten.size()).isEqualTo(10); + assertThat(ten.isEmpty()).isFalse(); + allKeys.stream() + .forEach( + key -> { + if (tenKeys.contains(key)) { + assertThat(ten.get(key)).isNotNull(); + } else { + assertThat(ten.get(key)).isNull(); + } + }); } @Test - void stringRepresentation() { - Attributes attributes = - FilteredAttributes.create( - Attributes.of( - stringKey("key1"), "value1", longKey("key2"), 333L, stringKey("key3"), "value3"), - ImmutableSet.of(stringKey("key1"), longKey("key2"))); - assertThat(attributes.toString()).isEqualTo("FilteredAttributes{key1=value1,key2=333}"); + void equalsAndHashCode() { + new EqualsTester() + .addEqualityGroup( + FILTERED_ATTRIBUTES_ONE, + FilteredAttributes.create(Attributes.of(KEY1, "value1"), Collections.singleton(KEY1)), + FilteredAttributes.create(Attributes.of(KEY1, "value1"), ImmutableSet.of(KEY1, KEY2)), + FilteredAttributes.create( + Attributes.of(KEY1, "value1", KEY2, "value2"), Collections.singleton(KEY1)), + FilteredAttributes.create( + Attributes.of(KEY1, "value1", KEY2_LONG, 222L), Collections.singleton(KEY1))) + .addEqualityGroup(FILTERED_ATTRIBUTES_TWO) + .addEqualityGroup(FILTERED_ATTRIBUTES_THREE, FILTERED_ATTRIBUTES_FOUR) + .addEqualityGroup(FILTERED_ATTRIBUTES_EMPTY, FILTERED_ATTRIBUTES_EMPTY_SOURCE) + .testEquals(); } } From 69f3c30c933bfbd478a4431f2978e2e372af3e64 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Mon, 12 Aug 2024 13:02:22 -0500 Subject: [PATCH 08/10] Remove metric advice benchmark test --- .../sdk/metrics/MetricAdviceBenchmark.java | 27 ++++ .../metrics/MetricAdviceBenchmarkTest.java | 123 ------------------ 2 files changed, 27 insertions(+), 123 deletions(-) delete mode 100644 sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java diff --git a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java index eafbcc56131..66c1a830607 100644 --- a/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java +++ b/sdk/metrics/src/jmh/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmark.java @@ -133,6 +133,10 @@ public void record(ThreadState threadState) { @SuppressWarnings("ImmutableEnumChecker") public enum InstrumentParam { + /** + * Record HTTP span attributes without advice. This baseline shows the CPU and memory allocation + * independent of advice. + */ NO_ADVICE_ALL_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -147,6 +151,13 @@ void record(long value) { counter.add(value, httpServerSpanAttributes()); } }), + /** + * Record HTTP metric attributes without advice. This baseline shows the lower bound if + * attribute filtering was done in instrumentation instead of the metrics SDK with advice. It's + * not quite fair though because instrumentation would have to separately allocate attributes + * for spans and metrics, whereas with advice, we can manage to only allocate span attributes + * and a lightweight metrics attributes view derived from span attributes. + */ NO_ADVICE_FILTERED_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -161,6 +172,10 @@ void record(long value) { counter.add(value, httpServerMetricAttributes()); } }), + /** + * Record cached HTTP span attributes without advice. This baseline helps isolate the CPU and + * memory allocation for recording vs. creating attributes. + */ NO_ADVICE_ALL_ATTRIBUTES_CACHED( new Instrument() { private LongCounter counter; @@ -175,6 +190,10 @@ void record(long value) { counter.add(value, CACHED_HTTP_SERVER_SPAN_ATTRIBUTES); } }), + /** + * Record HTTP span attributes with advice filtering to HTTP metric attributes. This is meant to + * realistically demonstrate a typical HTTP server instrumentation scenario. + */ ADVICE_ALL_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -192,6 +211,10 @@ void record(long value) { counter.add(value, httpServerSpanAttributes()); } }), + /** + * Record HTTP metric attributes with advice filtering to HTTP metric attributes. This + * demonstrates the overhead of advice when no attributes are filtered. + */ ADVICE_FILTERED_ATTRIBUTES( new Instrument() { private LongCounter counter; @@ -209,6 +232,10 @@ void record(long value) { counter.add(value, httpServerMetricAttributes()); } }), + /** + * Record cached HTTP span attributes with advice filtering to HTTP metric attributes. This + * isolates the CPU and memory allocation for applying advice vs. creating attributes. + */ ADVICE_ALL_ATTRIBUTES_CACHED( new Instrument() { private LongCounter counter; diff --git a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java b/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java deleted file mode 100644 index 397e5bbcbc5..00000000000 --- a/sdk/metrics/src/test/java/io/opentelemetry/sdk/metrics/MetricAdviceBenchmarkTest.java +++ /dev/null @@ -1,123 +0,0 @@ -/* - * Copyright The OpenTelemetry Authors - * SPDX-License-Identifier: Apache-2.0 - */ - -package io.opentelemetry.sdk.metrics; - -import io.opentelemetry.api.common.AttributeKey; -import io.opentelemetry.api.common.Attributes; -import io.opentelemetry.api.incubator.metrics.ExtendedLongCounterBuilder; -import io.opentelemetry.api.metrics.LongCounter; -import io.opentelemetry.api.metrics.Meter; -import io.opentelemetry.sdk.testing.exporter.InMemoryMetricReader; -import java.util.Arrays; -import java.util.List; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Test; - -public class MetricAdviceBenchmarkTest { - - static final AttributeKey HTTP_REQUEST_METHOD = - AttributeKey.stringKey("http.request.method"); - static final AttributeKey URL_PATH = AttributeKey.stringKey("url.path"); - static final AttributeKey URL_SCHEME = AttributeKey.stringKey("url.scheme"); - static final AttributeKey HTTP_RESPONSE_STATUS_CODE = - AttributeKey.longKey("http.response.status_code"); - static final AttributeKey HTTP_ROUTE = AttributeKey.stringKey("http.route"); - static final AttributeKey NETWORK_PROTOCOL_NAME = - AttributeKey.stringKey("network.protocol.name"); - static final AttributeKey SERVER_PORT = AttributeKey.longKey("server.port"); - static final AttributeKey URL_QUERY = AttributeKey.stringKey("url.query"); - static final AttributeKey CLIENT_ADDRESS = AttributeKey.stringKey("client.address"); - static final AttributeKey NETWORK_PEER_ADDRESS = - AttributeKey.stringKey("network.peer.address"); - static final AttributeKey NETWORK_PEER_PORT = AttributeKey.longKey("network.peer.port"); - static final AttributeKey NETWORK_PROTOCOL_VERSION = - AttributeKey.stringKey("network.protocol.version"); - static final AttributeKey SERVER_ADDRESS = AttributeKey.stringKey("server.address"); - static final AttributeKey USER_AGENT_ORIGINAL = - AttributeKey.stringKey("user_agent.original"); - - static final List> httpServerMetricAttributeKeys = - Arrays.asList( - HTTP_REQUEST_METHOD, - URL_SCHEME, - HTTP_RESPONSE_STATUS_CODE, - HTTP_ROUTE, - NETWORK_PROTOCOL_NAME, - SERVER_PORT, - NETWORK_PROTOCOL_VERSION, - SERVER_ADDRESS); - - static Attributes httpServerMetricAttributes() { - return Attributes.builder() - .put(HTTP_REQUEST_METHOD, "GET") - .put(URL_SCHEME, "http") - .put(HTTP_RESPONSE_STATUS_CODE, 200) - .put(HTTP_ROUTE, "/v1/users/{id}") - .put(NETWORK_PROTOCOL_NAME, "http") - .put(SERVER_PORT, 8080) - .put(NETWORK_PROTOCOL_VERSION, "1.1") - .put(SERVER_ADDRESS, "localhost") - .build(); - } - - static Attributes httpServerSpanAttributes() { - return Attributes.builder() - .put(HTTP_REQUEST_METHOD, "GET") - .put(URL_PATH, "/v1/users/123") - .put(URL_SCHEME, "http") - .put(HTTP_RESPONSE_STATUS_CODE, 200) - .put(HTTP_ROUTE, "/v1/users/{id}") - .put(NETWORK_PROTOCOL_NAME, "http") - .put(SERVER_PORT, 8080) - .put(URL_QUERY, "with=email") - .put(CLIENT_ADDRESS, "192.168.0.17") - .put(NETWORK_PEER_ADDRESS, "192.168.0.17") - .put(NETWORK_PEER_PORT, 11265) - .put(NETWORK_PROTOCOL_VERSION, "1.1") - .put(SERVER_ADDRESS, "localhost") - .put(USER_AGENT_ORIGINAL, "okhttp/1.27.2") - .build(); - } - - private SdkMeterProvider meterProvider; - private Meter meter; - - @BeforeEach - void setup() { - meterProvider = - SdkMeterProvider.builder().registerMetricReader(InMemoryMetricReader.createDelta()).build(); - meter = meterProvider.get("meter"); - } - - @AfterEach - void tearDown() {} - - @Test - void adviceAllAttributes() { - LongCounter counter = - ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) - .setAttributesAdvice(httpServerMetricAttributeKeys) - .build(); - - for (int i = 0; i < 1_000_000; i++) { - counter.add(1, httpServerSpanAttributes()); - } - } - - @Test - void adviceAllAttributesCached() { - Attributes attributes = httpServerSpanAttributes(); - LongCounter counter = - ((ExtendedLongCounterBuilder) meter.counterBuilder("counter")) - .setAttributesAdvice(httpServerMetricAttributeKeys) - .build(); - - for (int i = 0; i < 1_000_000; i++) { - counter.add(1, attributes); - } - } -} From ba582aeec552b9293c3cbf987017ec4ff95ba0ac Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Mon, 12 Aug 2024 13:28:55 -0500 Subject: [PATCH 09/10] Add javadoc to ImmutableKeyValuePairs --- .../io/opentelemetry/api/internal/ImmutableKeyValuePairs.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java b/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java index 0b0399f500c..4e6b19cee36 100644 --- a/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java +++ b/api/all/src/main/java/io/opentelemetry/api/internal/ImmutableKeyValuePairs.java @@ -275,6 +275,10 @@ public String toString() { return sb.toString(); } + /** + * Return the backing data array for these attributes. This is only exposed for internal use by + * opentelemetry authors. The contents of the array MUST NOT be modified. + */ @SuppressWarnings("AvoidObjectArrays") public Object[] getData() { return data; From 7e86ba96f93e4535d73f0a243bbfd26f0d6fec87 Mon Sep 17 00:00:00 2001 From: Jack Berg Date: Wed, 18 Sep 2024 11:43:55 -0500 Subject: [PATCH 10/10] PR feedback --- .../internal/view/FilteredAttributes.java | 91 +++++++++++-------- 1 file changed, 55 insertions(+), 36 deletions(-) diff --git a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java index 0392a90ab23..d71616c20e8 100644 --- a/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java +++ b/sdk/metrics/src/main/java/io/opentelemetry/sdk/metrics/internal/view/FilteredAttributes.java @@ -31,30 +31,15 @@ * also true for the default attributes implementation. */ @SuppressWarnings("unchecked") -final class FilteredAttributes implements Attributes { - - private static final int BITS_PER_INTEGER = 32; +abstract class FilteredAttributes implements Attributes { // Backing source data from ImmutableKeyValuePairs.data. This array MUST NOT be mutated. private final Object[] sourceData; - // The bits of filteredIndices track whether the first 32 key-values of sourceData should be - // excluded in the output. A bit equal to 1 indicates the sourceData key at i*2 should be - // excluded. overflowFilteredIndices is used when more than 32 key-value pairs are present in - // sourceData. - private final int filteredIndices; - @Nullable private final BitSet overflowFilteredIndices; private final int hashcode; private final int size; - private FilteredAttributes( - Object[] sourceData, - int filteredIndices, - @Nullable BitSet overflowFilteredIndices, - int hashcode, - int size) { + private FilteredAttributes(Object[] sourceData, int hashcode, int size) { this.sourceData = sourceData; - this.filteredIndices = filteredIndices; - this.overflowFilteredIndices = overflowFilteredIndices; this.hashcode = hashcode; this.size = size; } @@ -78,25 +63,24 @@ static Attributes create(Attributes source, Set> includedKeys) { throw new IllegalStateException( "Expected ImmutableKeyValuePairs based implementation of Attributes. This is a programming error."); } - // Compute filteredIndices (and overflowIndices if needed) during initialization. Compute + // Compute filteredIndices (and filteredIndicesBitSet if needed) during initialization. Compute // hashcode at the same time to avoid iteration later. Object[] sourceData = ((ImmutableKeyValuePairs) source).getData(); int filteredIndices = 0; - BitSet overflowFilteredIndices = - source.size() > BITS_PER_INTEGER ? new BitSet(source.size() - BITS_PER_INTEGER) : null; + BitSet filteredIndicesBitSet = + source.size() > SmallFilteredAttributes.BITS_PER_INTEGER ? new BitSet(source.size()) : null; int hashcode = 1; int size = 0; for (int i = 0; i < sourceData.length; i += 2) { int filterIndex = i / 2; // If the sourceData key isn't present in includedKeys, record the exclusion in - // filteredIndices (or - // overflowFilteredIndices based on the index). + // filteredIndices or filteredIndicesBitSet (depending on size) if (!includedKeys.contains(sourceData[i])) { // Record - if (filterIndex < BITS_PER_INTEGER) { - filteredIndices = filteredIndices | (1 << filterIndex); + if (filteredIndicesBitSet != null) { + filteredIndicesBitSet.set(filterIndex); } else { - overflowFilteredIndices.set(filterIndex - BITS_PER_INTEGER); + filteredIndices = filteredIndices | (1 << filterIndex); } } else { // The key-value is included in the output, record in the hashcode and size. hashcode = 31 * hashcode + sourceData[i].hashCode(); @@ -108,8 +92,50 @@ static Attributes create(Attributes source, Set> includedKeys) { if (size == 0) { return Attributes.empty(); } - return new FilteredAttributes( - sourceData, filteredIndices, overflowFilteredIndices, hashcode, size); + return filteredIndicesBitSet != null + ? new RegularFilteredAttributes(sourceData, hashcode, size, filteredIndicesBitSet) + : new SmallFilteredAttributes(sourceData, hashcode, size, filteredIndices); + } + + /** + * Implementation that relies on the source having less than {@link #BITS_PER_INTEGER} attributes, + * and storing entry filter status in the bits of an integer. + */ + private static class SmallFilteredAttributes extends FilteredAttributes { + + private static final int BITS_PER_INTEGER = 32; + + private final int filteredIndices; + + private SmallFilteredAttributes( + Object[] sourceData, int hashcode, int size, int filteredIndices) { + super(sourceData, hashcode, size); + this.filteredIndices = filteredIndices; + } + + @Override + boolean includeIndexInOutput(int sourceIndex) { + return (filteredIndices & (1 << (sourceIndex / 2))) == 0; + } + } + + /** + * Implementation that can handle attributes of arbitrary size by storing filter status in a + * {@link BitSet}. + */ + private static class RegularFilteredAttributes extends FilteredAttributes { + + private final BitSet bitSet; + + private RegularFilteredAttributes(Object[] sourceData, int hashcode, int size, BitSet bitSet) { + super(sourceData, hashcode, size); + this.bitSet = bitSet; + } + + @Override + boolean includeIndexInOutput(int sourceIndex) { + return !bitSet.get(sourceIndex / 2); + } } private static Attributes convertToStandardImplementation(Attributes source) { @@ -189,7 +215,7 @@ public boolean equals(Object object) { // FilteredAttributes is used for a key in a map, it must be used for all the keys. Note, this // same requirement exists for the default Attributes implementation - you can not mix // implementations. - if (object == null || getClass() != object.getClass()) { + if (object == null || !(object instanceof FilteredAttributes)) { return false; } @@ -254,12 +280,5 @@ public String toString() { return joiner.toString(); } - @SuppressWarnings("NullAway") - private boolean includeIndexInOutput(int sourceIndex) { - int bitIndex = sourceIndex / 2; - if (bitIndex < BITS_PER_INTEGER) { - return (filteredIndices & (1 << bitIndex)) == 0; - } - return !overflowFilteredIndices.get(bitIndex - BITS_PER_INTEGER); - } + abstract boolean includeIndexInOutput(int sourceIndex); }