Don't override preconfigured propagators. (#2004)

* don't override preconfigured propagators.

* add tests around duplicates, and deduplicate

* spotless life for clean living.

* move a few reused vars to fields.

* simplify list creation.

javaagent-tooling/javaagent-tooling.gradle

@@ -36,6 +36,8 @@ dependencies {
   implementation deps.slf4j
 
   testImplementation project(':testing-common')
+  testImplementation group: 'org.assertj', name: 'assertj-core', version: '3.18.1'
+  testImplementation group: 'org.mockito', name: 'mockito-core', version: '3.6.0'
 
   instrumentationMuzzle sourceSets.main.output
   instrumentationMuzzle configurations.implementation

javaagent-tooling/src/main/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializer.java

@@ -5,11 +5,11 @@
 
 package io.opentelemetry.javaagent.tooling;
 
+import static java.util.stream.Collectors.toSet;
+
 import com.google.common.collect.ImmutableMap;
-import io.opentelemetry.api.OpenTelemetry;
+import io.opentelemetry.api.GlobalOpenTelemetry;
 import io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator;
-import io.opentelemetry.api.trace.Span;
-import io.opentelemetry.api.trace.TracerProvider;
 import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.propagation.ContextPropagators;
@@ -18,13 +18,13 @@ import io.opentelemetry.extension.trace.propagation.AwsXRayPropagator;
 import io.opentelemetry.extension.trace.propagation.B3Propagator;
 import io.opentelemetry.extension.trace.propagation.JaegerPropagator;
 import io.opentelemetry.extension.trace.propagation.OtTracerPropagator;
-import java.lang.reflect.Method;
 import java.util.ArrayList;
+import java.util.Arrays;
 import java.util.Collection;
-import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
-import java.util.Set;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -58,97 +58,58 @@ public class PropagatorsInitializer {
    * </ul>
    */
   public static void initializePropagators(List<String> propagatorIds) {
-    /* Only override the default propagators *if* the user specified any. */
+    initializePropagators(
+        propagatorIds,
+        () -> GlobalOpenTelemetry.get().getPropagators().getTextMapPropagator(),
+        p -> GlobalOpenTelemetry.get().setPropagators(p));
+  }
+
+  // exists for testing
+  static void initializePropagators(
+      List<String> propagatorIds,
+      Supplier<TextMapPropagator> preconfiguredPropagator,
+      Consumer<ContextPropagators> globalSetter) {
+    ContextPropagators propagators =
+        createPropagators(propagatorIds, preconfiguredPropagator.get());
+    // Register it in the global propagators:
+    globalSetter.accept(propagators);
+  }
+
+  private static ContextPropagators createPropagators(
+      List<String> propagatorIds, TextMapPropagator preconfiguredPropagator) {
+    /* Only override the default propagators *if* the caller specified any. */
     if (propagatorIds.size() == 0) {
       // TODO this is probably temporary until default propagators are supplied by SDK
       //  https://github.com/open-telemetry/opentelemetry-java/issues/1742
-      OpenTelemetry.setGlobalPropagators(
+      return createPropagatorsRemovingNoops(
-          ContextPropagators.create(
+          Arrays.asList(
-              TextMapPropagator.composite(
+              preconfiguredPropagator,
-                  W3CTraceContextPropagator.getInstance(), W3CBaggagePropagator.getInstance())));
+              W3CTraceContextPropagator.getInstance(),
-      return;
+              W3CBaggagePropagator.getInstance()));
     }
 
-    List<Propagator> propagators = new ArrayList<>(propagatorIds.size());
     List<TextMapPropagator> textMapPropagators = new ArrayList<>();
+    textMapPropagators.add(preconfiguredPropagator);
 
     for (String propagatorId : propagatorIds) {
       Propagator propagator = TEXTMAP_PROPAGATORS.get(propagatorId);
       if (propagator != null) {
-        propagators.add(propagator);
+        textMapPropagators.add(propagator);
         log.info("Added " + propagatorId + " propagator");
       } else {
         log.warn("No matching propagator for " + propagatorId);
       }
     }
-    if (propagators.size() > 1) {
+    return createPropagatorsRemovingNoops(textMapPropagators);
-      textMapPropagators.add(new MultiPropagator(propagators));
-    } else if (propagators.size() == 1) {
-      textMapPropagators.add(propagators.get(0));
-    }
-    // Register it in the global propagators:
-    OpenTelemetry.setGlobalPropagators(
-        ContextPropagators.create(TextMapPropagator.composite(textMapPropagators)));
   }
 
-  private static TracerProvider unobfuscate(TracerProvider tracerProvider) {
+  private static ContextPropagators createPropagatorsRemovingNoops(
-    if (tracerProvider.getClass().getName().endsWith("TracerSdkProvider")) {
+      List<TextMapPropagator> textMapPropagators) {
-      return tracerProvider;
+    return ContextPropagators.create(
-    }
+        TextMapPropagator.composite(
-    try {
+            textMapPropagators.stream()
-      Method unobfuscate = tracerProvider.getClass().getDeclaredMethod("unobfuscate");
+                .filter(propagator -> propagator != TextMapPropagator.noop())
-      unobfuscate.setAccessible(true);
+                .collect(toSet())));
-      return (TracerProvider) unobfuscate.invoke(tracerProvider);
-    } catch (Throwable t) {
-      return tracerProvider;
-    }
-  }
-
-  static class MultiPropagator implements TextMapPropagator {
-    private final List<Propagator> propagators;
-    private final List<String> fields;
-
-    private MultiPropagator(List<Propagator> propagators) {
-      this.propagators = propagators;
-
-      Set<String> fields = new LinkedHashSet<>();
-      for (Propagator propagator : propagators) {
-        fields.addAll(propagator.delegate.fields());
-      }
-      this.fields = new ArrayList<>(fields);
-    }
-
-    @Override
-    public List<String> fields() {
-      return fields;
-    }
-
-    @Override
-    public <C> void inject(Context context, C carrier, Setter<C> setter) {
-      for (Propagator propagator : propagators) {
-        propagator.inject(context, carrier, setter);
-      }
-    }
-
-    @Override
-    public <C> Context extract(Context context, C carrier, Getter<C> getter) {
-      boolean spanContextExtracted = false;
-      for (int i = propagators.size() - 1; i >= 0; i--) {
-        Propagator propagator = propagators.get(i);
-        if (!propagator.alwaysRun() && spanContextExtracted) {
-          continue;
-        }
-        context = propagator.extract(context, carrier, getter);
-        if (!spanContextExtracted) {
-          spanContextExtracted = isSpanContextExtracted(context);
-        }
-      }
-      return context;
-    }
-
-    private static boolean isSpanContextExtracted(Context context) {
-      return Span.fromContextOrNull(context) != null;
-    }
   }
 
   enum Propagator implements TextMapPropagator {

javaagent-tooling/src/test/java/io/opentelemetry/javaagent/tooling/PropagatorsInitializerTest.java

@@ -0,0 +1,158 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.tooling;
+
+import static java.util.Collections.emptyList;
+import static java.util.Collections.singletonList;
+import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
+
+import io.opentelemetry.api.baggage.propagation.W3CBaggagePropagator;
+import io.opentelemetry.api.trace.propagation.W3CTraceContextPropagator;
+import io.opentelemetry.context.propagation.ContextPropagators;
+import io.opentelemetry.context.propagation.TextMapPropagator;
+import java.util.Arrays;
+import java.util.List;
+import java.util.concurrent.atomic.AtomicReference;
+import java.util.function.Consumer;
+import java.util.function.Supplier;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+
+class PropagatorsInitializerTest {
+
+  AtomicReference<ContextPropagators> seen;
+  TextMapPropagator mockPreconfigured;
+
+  @BeforeEach
+  void setup() {
+    seen = new AtomicReference<>();
+    mockPreconfigured = mock(TextMapPropagator.class);
+  }
+
+  @Test
+  void initialize_noIdsPassedNotPreconfigured() {
+    List<String> ids = emptyList();
+    Consumer<ContextPropagators> setter = seen::set;
+
+    PropagatorsInitializer.initializePropagators(ids, TextMapPropagator::noop, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .extracting("textPropagators")
+        .isInstanceOfSatisfying(
+            TextMapPropagator[].class,
+            p ->
+                assertThat(p)
+                    .containsExactlyInAnyOrder(
+                        W3CTraceContextPropagator.getInstance(),
+                        W3CBaggagePropagator.getInstance()));
+  }
+
+  @Test
+  void initialize_noIdsPassedWithPreconfigured() {
+    List<String> ids = emptyList();
+    Consumer<ContextPropagators> setter = seen::set;
+    TextMapPropagator mockPropagator = mock(TextMapPropagator.class);
+    Supplier<TextMapPropagator> preconfigured = () -> mockPropagator;
+
+    PropagatorsInitializer.initializePropagators(ids, preconfigured, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .extracting("textPropagators")
+        .isInstanceOfSatisfying(
+            TextMapPropagator[].class,
+            p ->
+                assertThat(p)
+                    .containsExactlyInAnyOrder(
+                        W3CTraceContextPropagator.getInstance(),
+                        W3CBaggagePropagator.getInstance(),
+                        mockPropagator));
+  }
+
+  @Test
+  void initialize_preconfiguredSameAsId() {
+    List<String> ids = singletonList("jaeger");
+    Consumer<ContextPropagators> setter = seen::set;
+    Supplier<TextMapPropagator> preconfigured = () -> PropagatorsInitializer.Propagator.JAEGER;
+
+    PropagatorsInitializer.initializePropagators(ids, preconfigured, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .isSameAs(PropagatorsInitializer.Propagator.JAEGER);
+  }
+
+  @Test
+  void initialize_preconfiguredDuplicatedInIds() {
+    List<String> ids = Arrays.asList("b3", "jaeger", "b3");
+    Consumer<ContextPropagators> setter = seen::set;
+    Supplier<TextMapPropagator> preconfigured = () -> PropagatorsInitializer.Propagator.JAEGER;
+
+    PropagatorsInitializer.initializePropagators(ids, preconfigured, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .extracting("textPropagators")
+        .isInstanceOfSatisfying(
+            TextMapPropagator[].class,
+            p ->
+                assertThat(p)
+                    .containsExactlyInAnyOrder(
+                        PropagatorsInitializer.Propagator.B3,
+                        PropagatorsInitializer.Propagator.JAEGER));
+  }
+
+  @Test
+  void initialize_justOneId() {
+    List<String> ids = singletonList("jaeger");
+    Consumer<ContextPropagators> setter = seen::set;
+    Supplier<TextMapPropagator> preconfigured = TextMapPropagator::noop;
+
+    PropagatorsInitializer.initializePropagators(ids, preconfigured, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .isSameAs(PropagatorsInitializer.Propagator.JAEGER);
+  }
+
+  @Test
+  void initialize_idsWithNoPreconfigured() {
+    List<String> ids = Arrays.asList("b3", "unknown-but-no-harm-done", "jaeger");
+    Consumer<ContextPropagators> setter = seen::set;
+    Supplier<TextMapPropagator> preconfigured = TextMapPropagator::noop;
+
+    PropagatorsInitializer.initializePropagators(ids, preconfigured, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .extracting("textPropagators")
+        .isInstanceOfSatisfying(
+            TextMapPropagator[].class,
+            p ->
+                assertThat(p)
+                    .containsExactlyInAnyOrder(
+                        PropagatorsInitializer.Propagator.B3,
+                        PropagatorsInitializer.Propagator.JAEGER));
+  }
+
+  @Test
+  void initialize_idsAndPreconfigured() {
+    List<String> ids = Arrays.asList("jaeger", "xray");
+    Consumer<ContextPropagators> setter = seen::set;
+    when(mockPreconfigured.fields()).thenReturn(singletonList("mocked"));
+    Supplier<TextMapPropagator> preconfigured = () -> mockPreconfigured;
+
+    PropagatorsInitializer.initializePropagators(ids, preconfigured, setter);
+
+    assertThat(seen.get().getTextMapPropagator())
+        .extracting("textPropagators")
+        .isInstanceOfSatisfying(
+            TextMapPropagator[].class,
+            p ->
+                assertThat(p)
+                    .containsExactlyInAnyOrder(
+                        mockPreconfigured,
+                        PropagatorsInitializer.Propagator.JAEGER,
+                        PropagatorsInitializer.Propagator.XRAY));
+  }
+}