From fa15e8b2d087afda82d4f5a165a4b4b87a17d58f Mon Sep 17 00:00:00 2001 From: Anuraag Agrawal Date: Thu, 18 Feb 2021 02:57:56 +0900 Subject: [PATCH] Remove BaggageBuilder.setParent / setNoParent (#2838) --- .../api/baggage/BaggageBenchmark.java | 33 ------- .../api/baggage/BaggageBuilder.java | 31 ------- .../api/baggage/ImmutableBaggage.java | 42 +-------- .../api/baggage/BaggageTest.java | 5 +- .../api/baggage/ImmutableBaggageTest.java | 90 ++++--------------- .../api/baggage/ScopedBaggageTest.java | 24 +---- .../opentracingshim/SpanContextShim.java | 5 +- 7 files changed, 22 insertions(+), 208 deletions(-) diff --git a/api/all/src/jmh/java/io/opentelemetry/api/baggage/BaggageBenchmark.java b/api/all/src/jmh/java/io/opentelemetry/api/baggage/BaggageBenchmark.java index 5884e7d912..0377c3d171 100644 --- a/api/all/src/jmh/java/io/opentelemetry/api/baggage/BaggageBenchmark.java +++ b/api/all/src/jmh/java/io/opentelemetry/api/baggage/BaggageBenchmark.java @@ -5,7 +5,6 @@ package io.opentelemetry.api.baggage; -import io.opentelemetry.context.Context; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; @@ -65,36 +64,4 @@ public class BaggageBenchmark { } return baggage; } - - @Benchmark - @BenchmarkMode({Mode.AverageTime}) - @Fork(1) - @Measurement(iterations = 15, time = 1) - @OutputTimeUnit(TimeUnit.NANOSECONDS) - @Warmup(iterations = 5, time = 1) - public Baggage baggageParentBenchmark() { - Baggage baggage = Baggage.empty(); - Context context = Context.root().with(baggage); - for (int i = 0; i < itemsToAdd; i++) { - baggage = Baggage.builder().put(keys.get(i), values.get(i)).setParent(context).build(); - context = context.with(baggage); - } - return baggage; - } - - @Benchmark - @BenchmarkMode({Mode.AverageTime}) - @Fork(1) - @Measurement(iterations = 15, time = 1) - @OutputTimeUnit(TimeUnit.NANOSECONDS) - @Warmup(iterations = 5, time = 1) - public Baggage baggageParentBenchmark_noContent() { - Baggage baggage = Baggage.empty(); - Context context = Context.root().with(baggage); - for (int i = 0; i < itemsToAdd; i++) { - baggage = Baggage.builder().setParent(context).build(); - context = context.with(baggage); - } - return baggage; - } } diff --git a/api/all/src/main/java/io/opentelemetry/api/baggage/BaggageBuilder.java b/api/all/src/main/java/io/opentelemetry/api/baggage/BaggageBuilder.java index 65e22b2891..5cf6c7891b 100644 --- a/api/all/src/main/java/io/opentelemetry/api/baggage/BaggageBuilder.java +++ b/api/all/src/main/java/io/opentelemetry/api/baggage/BaggageBuilder.java @@ -5,8 +5,6 @@ package io.opentelemetry.api.baggage; -import io.opentelemetry.context.Context; - /** * A builder of {@link Baggage}. * @@ -14,35 +12,6 @@ import io.opentelemetry.context.Context; */ public interface BaggageBuilder { - /** - * Sets the parent {@link Baggage} to use from the specified {@code Context}. If no parent {@link - * Baggage} is provided, the value of {@link Baggage#current()} at {@link #build()} time will be - * used as parent, unless {@link #setNoParent()} was called. - * - *

If no parent {@link Baggage} is available in the specified {@code Context}, the resulting - * {@link Baggage} will become a root instance, as if {@link #setNoParent()} had been called. - * - *

This must be used to create a {@link Baggage} when manual Context propagation is - * used. - * - *

If called multiple times, only the last specified value will be used. - * - * @param context the {@code Context}. - * @return this. - * @throws NullPointerException if {@code context} is {@code null}. - * @see #setNoParent() - */ - BaggageBuilder setParent(Context context); - - /** - * Sets the option to become a root {@link Baggage} with no parent. If not called, the - * value provided using {@link #setParent(Context)} or otherwise {@link Baggage#current()} at - * {@link #build()} time will be used as parent. - * - * @return this. - */ - BaggageBuilder setNoParent(); - /** * Adds the key/value pair and metadata regardless of whether the key is present. * diff --git a/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java b/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java index 39ca99ef0f..81c70aa9ce 100644 --- a/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java +++ b/api/all/src/main/java/io/opentelemetry/api/baggage/ImmutableBaggage.java @@ -5,11 +5,8 @@ package io.opentelemetry.api.baggage; -import static java.util.Objects.requireNonNull; - import io.opentelemetry.api.internal.ImmutableKeyValuePairs; import io.opentelemetry.api.internal.StringUtils; -import io.opentelemetry.context.Context; import java.util.ArrayList; import java.util.List; import javax.annotation.Nullable; @@ -42,9 +39,7 @@ final class ImmutableBaggage extends ImmutableKeyValuePairs(data())); - builder.noImplicitParent = true; - return builder; + return new Builder(new ArrayList<>(data())); } private static Baggage sortAndFilterToBaggage(Object[] data) { @@ -55,8 +50,6 @@ final class ImmutableBaggage extends ImmutableKeyValuePairs data; Builder() { @@ -67,20 +60,6 @@ final class ImmutableBaggage extends ImmutableKeyValuePairs data = this.data; - if (parent != null && !parent.isEmpty()) { - List merged = new ArrayList<>(parent.size() * 2 + data.size()); - if (parent instanceof ImmutableBaggage) { - merged.addAll(((ImmutableBaggage) parent).data()); - } else { - parent.forEach( - (key, entry) -> { - merged.add(key); - merged.add(entry); - }); - } - merged.addAll(data); - data = merged; - } return sortAndFilterToBaggage(data.toArray()); } } diff --git a/api/all/src/test/java/io/opentelemetry/api/baggage/BaggageTest.java b/api/all/src/test/java/io/opentelemetry/api/baggage/BaggageTest.java index 9e7ed3f12e..4c141276e1 100644 --- a/api/all/src/test/java/io/opentelemetry/api/baggage/BaggageTest.java +++ b/api/all/src/test/java/io/opentelemetry/api/baggage/BaggageTest.java @@ -22,12 +22,9 @@ class BaggageTest { @Test void current() { try (Scope scope = - Context.root() - .with(Baggage.builder().put("foo", "bar").setNoParent().build()) - .makeCurrent()) { + Context.root().with(Baggage.builder().put("foo", "bar").build()).makeCurrent()) { Baggage result = Baggage.current(); assertThat(result.getEntryValue("foo")).isEqualTo("bar"); - assertThat(result).isEqualTo(Baggage.builder().setNoParent().put("foo", "bar").build()); } } } diff --git a/api/all/src/test/java/io/opentelemetry/api/baggage/ImmutableBaggageTest.java b/api/all/src/test/java/io/opentelemetry/api/baggage/ImmutableBaggageTest.java index a7b962d6f1..ca78124fb4 100644 --- a/api/all/src/test/java/io/opentelemetry/api/baggage/ImmutableBaggageTest.java +++ b/api/all/src/test/java/io/opentelemetry/api/baggage/ImmutableBaggageTest.java @@ -6,12 +6,9 @@ package io.opentelemetry.api.baggage; 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.testing.EqualsTester; -import io.opentelemetry.context.Context; -import io.opentelemetry.context.Scope; import org.junit.jupiter.api.Test; /** @@ -61,8 +58,7 @@ class ImmutableBaggageTest { @Test void getEntries_chain() { - Context parentContext = Context.root().with(TWO_ENTRIES); - Baggage baggage = Baggage.builder().setParent(parentContext).put(K1, V2, TMD).build(); + Baggage baggage = TWO_ENTRIES.toBuilder().put(K1, V2, TMD).build(); assertThat(baggage.asMap()) .containsOnly( entry(K1, ImmutableEntry.create(V2, TMD)), entry(K2, ImmutableEntry.create(V2, TMD))); @@ -70,23 +66,20 @@ class ImmutableBaggageTest { @Test void put_newKey() { - Context parentContext = Context.root().with(ONE_ENTRY); - assertThat(Baggage.builder().setParent(parentContext).put(K2, V2, TMD).build().asMap()) + assertThat(ONE_ENTRY.toBuilder().put(K2, V2, TMD).build().asMap()) .containsOnly( entry(K1, ImmutableEntry.create(V1, TMD)), entry(K2, ImmutableEntry.create(V2, TMD))); } @Test void put_existingKey() { - Context parentContext = Context.root().with(ONE_ENTRY); - assertThat(Baggage.builder().setParent(parentContext).put(K1, V2, TMD).build().asMap()) + assertThat(ONE_ENTRY.toBuilder().put(K1, V2, TMD).build().asMap()) .containsOnly(entry(K1, ImmutableEntry.create(V2, TMD))); } @Test void put_nullKey() { - Context parentContext = Context.root().with(ONE_ENTRY); - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = ONE_ENTRY.toBuilder(); Baggage built = builder.build(); builder.put(null, V2, TMD); assertThat(builder.build()).isEqualTo(built); @@ -94,8 +87,7 @@ class ImmutableBaggageTest { @Test void put_nullValue() { - Context parentContext = Context.root().with(ONE_ENTRY); - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = ONE_ENTRY.toBuilder(); Baggage built = builder.build(); builder.put(K2, null, TMD); assertThat(builder.build()).isEqualTo(built); @@ -103,8 +95,7 @@ class ImmutableBaggageTest { @Test void put_nullMetadata() { - Context parentContext = Context.root().with(ONE_ENTRY); - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = ONE_ENTRY.toBuilder(); Baggage built = builder.build(); builder.put(K2, V2, null); assertThat(builder.build()).isEqualTo(built); @@ -112,8 +103,7 @@ class ImmutableBaggageTest { @Test void put_keyUnprintableChars() { - Context parentContext = Context.root().with(ONE_ENTRY); - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = ONE_ENTRY.toBuilder(); Baggage built = builder.build(); builder.put("\2ab\3cd", "value"); assertThat(builder.build()).isEqualTo(built); @@ -121,8 +111,7 @@ class ImmutableBaggageTest { @Test void put_keyEmpty() { - Context parentContext = Context.root().with(ONE_ENTRY); - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = ONE_ENTRY.toBuilder(); Baggage built = builder.build(); builder.put("", "value"); assertThat(builder.build()).isEqualTo(built); @@ -130,44 +119,12 @@ class ImmutableBaggageTest { @Test void put_valueUnprintableChars() { - Context parentContext = Context.root().with(ONE_ENTRY); - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = ONE_ENTRY.toBuilder(); Baggage built = builder.build(); builder.put(K2, "\2ab\3cd"); assertThat(builder.build()).isEqualTo(built); } - @Test - void setParent_nullContext() { - assertThatThrownBy( - () -> Baggage.builder().setParent(null).put("cat", "meow").build().getEntryValue("cat")) - .isInstanceOf(NullPointerException.class); - } - - @Test - void setParent_fromContext() { - Baggage baggage = Baggage.builder().put(K2, V2, TMD).build(); - Context context = Context.root().with(baggage); - baggage = Baggage.builder().setParent(context).build(); - assertThat(baggage.asMap()).containsOnly(entry(K2, ImmutableEntry.create(V2, TMD))); - } - - @Test - void setParent_fromEmptyContext() { - Context emptyContext = Context.root(); - try (Scope ignored = ONE_ENTRY.makeCurrent()) { - Baggage baggage = Baggage.builder().setParent(emptyContext).build(); - assertThat(baggage.isEmpty()).isTrue(); - } - } - - @Test - void setParent_setNoParent() { - Context parentContext = Context.root().with(ONE_ENTRY); - Baggage baggage = Baggage.builder().setParent(parentContext).setNoParent().build(); - assertThat(baggage.isEmpty()).isTrue(); - } - @Test void remove_existingKey() { BaggageBuilder builder = Baggage.builder(); @@ -190,8 +147,7 @@ class ImmutableBaggageTest { @Test void remove_keyFromParent() { - Context parentContext = Context.root().with(TWO_ENTRIES); - assertThat(Baggage.builder().setParent(parentContext).remove(K1).build().asMap()) + assertThat(TWO_ENTRIES.toBuilder().remove(K1).build().asMap()) .containsOnly(entry(K2, ImmutableEntry.create(V2, TMD))); } @@ -210,42 +166,28 @@ class ImmutableBaggageTest { Baggage originalBaggage = Baggage.builder().put("key", "value").build(); assertThat(originalBaggage.toBuilder().build()).isEqualTo(originalBaggage); - - Baggage parentedBaggage = - Baggage.builder().setParent(Context.root().with(originalBaggage)).build(); - assertThat(parentedBaggage.toBuilder().build()).isEqualTo(parentedBaggage); } @Test void toBuilder_allowChanges() { - Baggage singleItemNoParent = Baggage.builder().put("key1", "value1").setNoParent().build(); - Baggage singleItemWithParent = - Baggage.builder() - .setParent(Context.root().with(Baggage.empty())) - .put("key1", "value1") - .build(); + Baggage singleItemNoParent = Baggage.builder().put("key1", "value1").build(); + Baggage singleItemWithParent = Baggage.builder().put("key1", "value1").build(); assertThat(Baggage.empty().toBuilder().put("key1", "value1").build()) .isEqualTo(singleItemNoParent); assertThat(singleItemNoParent.toBuilder().put("key2", "value2").build()) - .isEqualTo( - Baggage.builder().put("key1", "value1").put("key2", "value2").setNoParent().build()); + .isEqualTo(Baggage.builder().put("key1", "value1").put("key2", "value2").build()); assertThat(singleItemNoParent.toBuilder().put("key1", "value2").build()) - .isEqualTo(Baggage.builder().put("key1", "value2").setNoParent().build()); + .isEqualTo(Baggage.builder().put("key1", "value2").build()); assertThat(singleItemWithParent.toBuilder().put("key1", "value2").build()) - .isEqualTo( - Baggage.builder() - .put("key1", "value2") - .setParent(Context.root().with(Baggage.empty())) - .build()); + .isEqualTo(Baggage.builder().put("key1", "value2").build()); } @Test void testEquals() { Baggage baggage1 = Baggage.builder().put(K1, V1).build(); - Baggage baggage2 = - Baggage.builder().setParent(Context.current().with(baggage1)).put(K1, V2).build(); + Baggage baggage2 = baggage1.toBuilder().put(K1, V2).build(); Baggage baggage3 = Baggage.builder().put(K1, V2).build(); new EqualsTester() .addEqualityGroup( diff --git a/api/all/src/test/java/io/opentelemetry/api/baggage/ScopedBaggageTest.java b/api/all/src/test/java/io/opentelemetry/api/baggage/ScopedBaggageTest.java index 62b3a8899e..2a018b5fa2 100644 --- a/api/all/src/test/java/io/opentelemetry/api/baggage/ScopedBaggageTest.java +++ b/api/all/src/test/java/io/opentelemetry/api/baggage/ScopedBaggageTest.java @@ -51,7 +51,7 @@ class ScopedBaggageTest { Baggage.builder().put(KEY_1, VALUE_1, METADATA_UNLIMITED_PROPAGATION).build(); try (Scope scope = scopedBaggage.makeCurrent()) { Baggage newEntries = - Baggage.builder().put(KEY_2, VALUE_2, METADATA_UNLIMITED_PROPAGATION).build(); + Baggage.current().toBuilder().put(KEY_2, VALUE_2, METADATA_UNLIMITED_PROPAGATION).build(); assertThat(newEntries.asMap()) .containsOnly( entry(KEY_1, ImmutableEntry.create(VALUE_1, METADATA_UNLIMITED_PROPAGATION)), @@ -80,7 +80,7 @@ class ScopedBaggageTest { Baggage.builder().put(KEY_1, VALUE_1, METADATA_UNLIMITED_PROPAGATION).build(); try (Scope scope1 = scopedBaggage.makeCurrent()) { Baggage innerBaggage = - Baggage.builder().put(KEY_2, VALUE_2, METADATA_UNLIMITED_PROPAGATION).build(); + Baggage.current().toBuilder().put(KEY_2, VALUE_2, METADATA_UNLIMITED_PROPAGATION).build(); try (Scope scope2 = innerBaggage.makeCurrent()) { assertThat(Baggage.current().asMap()) .containsOnly( @@ -101,7 +101,7 @@ class ScopedBaggageTest { .build(); try (Scope scope1 = scopedBaggage.makeCurrent()) { Baggage innerBaggage = - Baggage.builder() + Baggage.current().toBuilder() .put(KEY_3, VALUE_3, METADATA_NO_PROPAGATION) .put(KEY_2, VALUE_4, METADATA_NO_PROPAGATION) .build(); @@ -116,22 +116,4 @@ class ScopedBaggageTest { assertThat(Baggage.current()).isSameAs(scopedBaggage); } } - - @Test - void setNoParent_doesNotInheritContext() { - assertThat(Baggage.current().isEmpty()).isTrue(); - Baggage scopedBaggage = - Baggage.builder().put(KEY_1, VALUE_1, METADATA_UNLIMITED_PROPAGATION).build(); - try (Scope scope = scopedBaggage.makeCurrent()) { - Baggage innerBaggage = - Baggage.builder() - .setNoParent() - .put(KEY_2, VALUE_2, METADATA_UNLIMITED_PROPAGATION) - .build(); - assertThat(innerBaggage.asMap()) - .containsOnly( - entry(KEY_2, ImmutableEntry.create(VALUE_2, METADATA_UNLIMITED_PROPAGATION))); - } - assertThat(Baggage.current().isEmpty()).isTrue(); - } } diff --git a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanContextShim.java b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanContextShim.java index 53db4c3868..62d31fe45a 100644 --- a/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanContextShim.java +++ b/opentracing-shim/src/main/java/io/opentelemetry/opentracingshim/SpanContextShim.java @@ -8,7 +8,6 @@ package io.opentelemetry.opentracingshim; import io.opentelemetry.api.baggage.Baggage; import io.opentelemetry.api.baggage.BaggageBuilder; import io.opentelemetry.api.baggage.BaggageEntryMetadata; -import io.opentelemetry.context.Context; import io.opentracing.SpanContext; import java.util.AbstractMap; import java.util.ArrayList; @@ -42,9 +41,7 @@ final class SpanContextShim extends BaseShimObject implements SpanContext { } SpanContextShim newWithKeyValue(String key, String value) { - Context parentContext = Context.current().with(baggage); - - BaggageBuilder builder = Baggage.builder().setParent(parentContext); + BaggageBuilder builder = baggage.toBuilder(); builder.put(key, value, BaggageEntryMetadata.empty()); return new SpanContextShim(telemetryInfo(), context, builder.build());