open-telemetry
/
opentelemetry-java
mirror of https://github.com/open-telemetry/opentelemetry-java.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Fix tracestate builder reuse and Optimize. (#4325)

This commit is contained in:
Anuraag Agrawal 2022-04-01 11:47:35 +09:00 committed by GitHub
parent 282ae40a3f
commit bc9dafca03
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
4 changed files with 185 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

56
api/all/src/jmh/java/io/opentelemetry/api/trace/TraceStateBenchmark.java Normal file
View File

@ -0,0 +1,56 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.api.trace;
import java.util.concurrent.TimeUnit;
import org.openjdk.jmh.annotations.Benchmark;
import org.openjdk.jmh.annotations.BenchmarkMode;
import org.openjdk.jmh.annotations.Fork;
import org.openjdk.jmh.annotations.Measurement;
import org.openjdk.jmh.annotations.Mode;
import org.openjdk.jmh.annotations.OutputTimeUnit;
import org.openjdk.jmh.annotations.Warmup;
@BenchmarkMode(Mode.AverageTime)
@Fork(1)
@Measurement(iterations = 15, time = 1)
@OutputTimeUnit(TimeUnit.NANOSECONDS)
@Warmup(iterations = 5, time = 1)
public class TraceStateBenchmark {
@Benchmark
public TraceState oneItem() {
TraceStateBuilder builder = TraceState.builder();
builder.put("key1", "val");
return builder.build();
}
@Benchmark
public TraceState fiveItems() {
TraceStateBuilder builder = TraceState.builder();
builder.put("key1", "val");
builder.put("key2", "val");
builder.put("key3", "val");
builder.put("key4", "val");
builder.put("key5", "val");
return builder.build();
}
@Benchmark
public TraceState fiveItemsWithRemoval() {
TraceStateBuilder builder = TraceState.builder();
builder.put("key1", "val");
builder.put("key2", "val");
builder.put("key3", "val");
builder.remove("key2");
builder.remove("key3");
builder.put("key2", "val");
builder.put("key3", "val");
builder.put("key4", "val");
builder.put("key5", "val");
return builder.build();
}
}

3
api/all/src/main/java/io/opentelemetry/api/trace/ArrayBasedTraceState.java
View File

@ -7,7 +7,6 @@ package io.opentelemetry.api.trace;
import com.google.auto.value.AutoValue;
import io.opentelemetry.api.internal.ReadOnlyArrayMap;
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.function.BiConsumer;
@ -68,7 +67,7 @@ abstract class ArrayBasedTraceState implements TraceState {
}
static ArrayBasedTraceState create(List<String> entries) {
return new AutoValue_ArrayBasedTraceState(Collections.unmodifiableList(entries));
return new AutoValue_ArrayBasedTraceState(entries);
}
ArrayBasedTraceState() {}

83
api/all/src/main/java/io/opentelemetry/api/trace/ArrayBasedTraceStateBuilder.java
View File

@ -7,6 +7,7 @@ package io.opentelemetry.api.trace;
import io.opentelemetry.api.internal.StringUtils;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
import javax.annotation.Nullable;
@ -20,23 +21,37 @@ final class ArrayBasedTraceStateBuilder implements TraceStateBuilder {
private static final ArrayBasedTraceState EMPTY =
ArrayBasedTraceState.create(Collections.emptyList());
private static final int MAX_KEY_VALUE_PAIRS = 32;
private static final int MAX_ENTRIES = 32;
private static final int KEY_MAX_SIZE = 256;
private static final int VALUE_MAX_SIZE = 256;
private static final int MAX_TENANT_ID_SIZE = 240;
private final List<String> entries;
// Later calls to put must be at the front of trace state. We append to the list and then reverse
// when finished.
private final List<String> reversedEntries;
// We record removed entries with null values, this is the number of entries in the builder not
// including removed ones.
int numEntries;
static TraceState empty() {
return EMPTY;
}
ArrayBasedTraceStateBuilder() {
entries = new ArrayList<>();
reversedEntries = new ArrayList<>();
numEntries = 0;
}
ArrayBasedTraceStateBuilder(ArrayBasedTraceState parent) {
entries = new ArrayList<>(parent.getEntries());
List<String> entries = parent.getEntries();
int size = entries.size();
reversedEntries = new ArrayList<>(size);
for (int i = size - 2; i >= 0; i -= 2) {
reversedEntries.add(entries.get(i));
reversedEntries.add(entries.get(i + 1));
}
numEntries = size / 2;
}
/**
@ -51,14 +66,22 @@ final class ArrayBasedTraceStateBuilder implements TraceStateBuilder {
*/
@Override
public TraceStateBuilder put(String key, String value) {
if (!isKeyValid(key) || !isValueValid(value) || entries.size() >= MAX_KEY_VALUE_PAIRS) {
if (!isKeyValid(key) || !isValueValid(value) || numEntries >= MAX_ENTRIES) {
return this;
}
removeEntry(key);
// Inserts the element at the front of this list. (note: probably pretty inefficient with an
// ArrayList as the underlying implementation!)
entries.add(0, key);
entries.add(1, value);
for (int i = 0; i < reversedEntries.size(); i += 2) {
if (reversedEntries.get(i).equals(key)) {
String currentValue = reversedEntries.get(i + 1);
reversedEntries.set(i + 1, value);
if (currentValue == null) {
numEntries++;
}
return this;
}
}
reversedEntries.add(key);
reversedEntries.add(value);
numEntries++;
return this;
}
@ -67,26 +90,38 @@ final class ArrayBasedTraceStateBuilder implements TraceStateBuilder {
if (key == null) {
return this;
}
removeEntry(key);
return this;
}
private void removeEntry(String key) {
int currentSize = entries.size();
for (int i = 0; i < currentSize; i += 2) {
if (entries.get(i).equals(key)) {
// remove twice at i to get the key & the value (yes, this is pretty ugly).
entries.remove(i);
entries.remove(i);
// Exit now because the entries list cannot contain duplicates.
break;
for (int i = 0; i < reversedEntries.size(); i += 2) {
if (reversedEntries.get(i).equals(key)) {
reversedEntries.set(i + 1, null);
numEntries--;
return this;
}
}
return this;
}
@Override
public TraceState build() {
return ArrayBasedTraceState.create(entries);
if (numEntries == 0) {
return empty();
}
if (reversedEntries.size() == 2) {
return ArrayBasedTraceState.create(new ArrayList<>(reversedEntries));
}
String[] entries = new String[numEntries * 2];
int pos = 0;
for (int i = reversedEntries.size() - 2; i >= 0; i -= 2) {
String key = reversedEntries.get(i);
String value = reversedEntries.get(i + 1);
if (value != null) {
entries[pos++] = key;
entries[pos++] = value;
}
}
// TODO(anuraaga): We may consider removing AutoValue which prevents us from storing the array
// directly as it would be a bit more performant, though not hugely.
return ArrayBasedTraceState.create(Arrays.asList(entries));
}
/**

80
api/all/src/test/java/io/opentelemetry/api/trace/TraceStateTest.java
View File

@ -8,7 +8,6 @@ package io.opentelemetry.api.trace;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;
import static org.assertj.core.api.Assertions.entry;
import static org.assertj.core.api.InstanceOfAssertFactories.type;
import com.google.common.testing.EqualsTester;
import java.util.Arrays;
@ -242,11 +241,9 @@ class TraceStateTest {
firstTraceState.toBuilder()
.put(FIRST_KEY, SECOND_VALUE) // update the existing entry
.put(SECOND_KEY, FIRST_VALUE) // add a new entry
.build())
.asInstanceOf(type(ArrayBasedTraceState.class))
.extracting(ArrayBasedTraceState::getEntries)
.asList()
.containsExactly(SECOND_KEY, FIRST_VALUE, FIRST_KEY, SECOND_VALUE);
.build()
.asMap())
.containsExactly(entry(SECOND_KEY, FIRST_VALUE), entry(FIRST_KEY, SECOND_VALUE));
}
@Test
@ -255,11 +252,26 @@ class TraceStateTest {
TraceState.builder()
.put(FIRST_KEY, SECOND_VALUE) // update the existing entry
.put(FIRST_KEY, FIRST_VALUE) // add a new entry
.build())
.asInstanceOf(type(ArrayBasedTraceState.class))
.extracting(ArrayBasedTraceState::getEntries)
.asList()
.containsExactly(FIRST_KEY, FIRST_VALUE);
.build()
.asMap())
.containsExactly(entry(FIRST_KEY, FIRST_VALUE));
}
@Test
void tooManyEntries() {
TraceStateBuilder stateBuilder = TraceState.builder();
for (int i = 0; i < 32; i++) {
stateBuilder.put("key" + i, "val");
// Make sure removal does actually allow adding more keys up to the limit.
stateBuilder.remove("key" + i);
stateBuilder.put("key" + i, "val");
}
stateBuilder.put("key32", "val");
TraceState state = stateBuilder.build();
assertThat(state.size()).isEqualTo(32);
for (int i = 0; i < 32; i++) {
assertThat(state.get("key" + i)).isEqualTo("val");
}
}
@Test
@ -268,6 +280,12 @@ class TraceStateTest {
.isEqualTo(firstTraceState);
}
@Test
void removeNotPresent() {
assertThat(multiValueTraceState.toBuilder().remove("unknown").build())
.isEqualTo(multiValueTraceState);
}
@Test
void addAndRemoveEntry() {
assertThat(
@ -278,6 +296,18 @@ class TraceStateTest {
.isEqualTo(TraceState.getDefault());
}
@Test
void addRemoveAndAddAgainEntry() {
assertThat(
TraceState.builder()
.put(FIRST_KEY, SECOND_VALUE) // update the existing entry
.remove(FIRST_KEY) // add a new entry
.put(FIRST_KEY, FIRST_VALUE)
.build()
.asMap())
.containsExactly(entry(FIRST_KEY, FIRST_VALUE));
}
@Test
void remove_NullNotAllowed() {
assertThat(multiValueTraceState.toBuilder().remove(null).build())
@ -309,4 +339,32 @@ class TraceStateTest {
assertThat(TraceState.getDefault().get(null)).isNull();
assertThatCode(() -> TraceState.getDefault().forEach(null)).doesNotThrowAnyException();
}
@Test
void reuseBuilder() {
TraceStateBuilder builder = TraceState.builder();
builder.put("animal", "bear");
TraceState state1 = builder.build();
builder.put("food", "pizza");
TraceState state2 = builder.build();
assertThat(state1.asMap()).containsExactly(entry("animal", "bear"));
assertThat(state2.asMap()).containsExactly(entry("food", "pizza"), entry("animal", "bear"));
}
// Not strictly a necessary test for behavior but it's still good to verify optimizations hold in
// case things get refactored.
@Test
void emptyIsSingleton() {
assertThat(TraceState.builder().build()).isSameAs(TraceState.getDefault());
assertThat(TraceState.builder().put("animal", "bear").remove("animal").build())
.isSameAs(TraceState.getDefault());
assertThat(
TraceState.builder()
.put("animal", "bear")
.put("food", "pizza")
.remove("animal")
.remove("food")
.build())
.isSameAs(TraceState.getDefault());
}
}