Introduce new incubating InstrumenterBuilder methods (#8392)

Mateusz Rzeszutek · web-flow · commit d22dd8c5f132 · 2023-04-28T14:19:15.000-07:00
diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java
@@ -11,6 +11,8 @@
 import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.api.trace.Tracer;
 import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.internal.InstrumenterAccess;
+import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil;
 import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics;
 import java.time.Instant;
 import java.util.ArrayList;
@@ -250,4 +252,22 @@ private static long getNanos(@Nullable Instant time) {
     }
     return TimeUnit.SECONDS.toNanos(time.getEpochSecond()) + time.getNano();
   }
+
+  static {
+    InstrumenterUtil.setInstrumenterAccess(
+        new InstrumenterAccess() {
+          @Override
+          public <RQ, RS> Context startAndEnd(
+              Instrumenter<RQ, RS> instrumenter,
+              Context parentContext,
+              RQ request,
+              @Nullable RS response,
+              @Nullable Throwable error,
+              Instant startTime,
+              Instant endTime) {
+            return instrumenter.startAndEnd(
+                parentContext, request, response, error, startTime, endTime);
+          }
+        });
+  }
 }
diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterBuilder.java
@@ -20,6 +20,8 @@
 import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.internal.ConfigPropertiesUtil;
 import io.opentelemetry.instrumentation.api.internal.EmbeddedInstrumentationProperties;
+import io.opentelemetry.instrumentation.api.internal.InstrumenterBuilderAccess;
+import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil;
 import io.opentelemetry.instrumentation.api.internal.SpanKey;
 import io.opentelemetry.instrumentation.api.internal.SpanKeyProvider;
 import java.util.ArrayList;
@@ -187,7 +189,7 @@ public InstrumenterBuilder<REQUEST, RESPONSE> setEnabled(boolean enabled) {
 
   /**
    * Returns a new {@link Instrumenter} which will create {@linkplain SpanKind#CLIENT client} spans
-   * and inject context into requests.
+   * and inject context into requests with the passed {@link TextMapSetter}.
    */
   public Instrumenter<REQUEST, RESPONSE> buildClientInstrumenter(TextMapSetter<REQUEST> setter) {
     return buildInstrumenter(
@@ -197,7 +199,7 @@ public Instrumenter<REQUEST, RESPONSE> buildClientInstrumenter(TextMapSetter<REQ
 
   /**
    * Returns a new {@link Instrumenter} which will create {@linkplain SpanKind#SERVER server} spans
-   * and extract context from requests.
+   * and extract context from requests with the passed {@link TextMapGetter}.
    */
   public Instrumenter<REQUEST, RESPONSE> buildServerInstrumenter(TextMapGetter<REQUEST> getter) {
     return buildInstrumenter(
@@ -207,7 +209,7 @@ public Instrumenter<REQUEST, RESPONSE> buildServerInstrumenter(TextMapGetter<REQ
 
   /**
    * Returns a new {@link Instrumenter} which will create {@linkplain SpanKind#PRODUCER producer}
-   * spans and inject context into requests.
+   * spans and inject context into requests with the passed {@link TextMapSetter}.
    */
   public Instrumenter<REQUEST, RESPONSE> buildProducerInstrumenter(TextMapSetter<REQUEST> setter) {
     return buildInstrumenter(
@@ -217,14 +219,40 @@ public Instrumenter<REQUEST, RESPONSE> buildProducerInstrumenter(TextMapSetter<R
 
   /**
    * Returns a new {@link Instrumenter} which will create {@linkplain SpanKind#CONSUMER consumer}
-   * spans and extract context from requests.
+   * spans and extract context from requests with the passed {@link TextMapGetter}.
    */
   public Instrumenter<REQUEST, RESPONSE> buildConsumerInstrumenter(TextMapGetter<REQUEST> getter) {
     return buildInstrumenter(
         InstrumenterConstructor.propagatingFromUpstream(requireNonNull(getter, "getter")),
         SpanKindExtractor.alwaysConsumer());
   }
 
+  /**
+   * Returns a new {@link Instrumenter} which will create spans with kind determined by the passed
+   * {@link SpanKindExtractor} and extract context from requests with the passed {@link
+   * TextMapGetter}.
+   */
+  // TODO: candidate for public API
+  Instrumenter<REQUEST, RESPONSE> buildUpstreamInstrumenter(
+      TextMapGetter<REQUEST> getter, SpanKindExtractor<REQUEST> spanKindExtractor) {
+    return buildInstrumenter(
+        InstrumenterConstructor.propagatingFromUpstream(requireNonNull(getter, "getter")),
+        spanKindExtractor);
+  }
+
+  /**
+   * Returns a new {@link Instrumenter} which will create spans with kind determined by the passed
+   * {@link SpanKindExtractor} and inject context into requests with the passed {@link
+   * TextMapSetter}.
+   */
+  // TODO: candidate for public API
+  Instrumenter<REQUEST, RESPONSE> buildDownstreamInstrumenter(
+      TextMapSetter<REQUEST> setter, SpanKindExtractor<REQUEST> spanKindExtractor) {
+    return buildInstrumenter(
+        InstrumenterConstructor.propagatingToDownstream(requireNonNull(setter, "setter")),
+        spanKindExtractor);
+  }
+
   /**
    * Returns a new {@link Instrumenter} which will create {@linkplain SpanKind#INTERNAL internal}
    * spans and do no context propagation.
@@ -321,4 +349,25 @@ static <RQ, RS> InstrumenterConstructor<RQ, RS> propagatingFromUpstream(
       return builder -> new PropagatingFromUpstreamInstrumenter<>(builder, getter);
     }
   }
+
+  static {
+    InstrumenterUtil.setInstrumenterBuilderAccess(
+        new InstrumenterBuilderAccess() {
+          @Override
+          public <RQ, RS> Instrumenter<RQ, RS> buildUpstreamInstrumenter(
+              InstrumenterBuilder<RQ, RS> builder,
+              TextMapGetter<RQ> getter,
+              SpanKindExtractor<RQ> spanKindExtractor) {
+            return builder.buildUpstreamInstrumenter(getter, spanKindExtractor);
+          }
+
+          @Override
+          public <RQ, RS> Instrumenter<RQ, RS> buildDownstreamInstrumenter(
+              InstrumenterBuilder<RQ, RS> builder,
+              TextMapSetter<RQ> setter,
+              SpanKindExtractor<RQ> spanKindExtractor) {
+            return builder.buildDownstreamInstrumenter(setter, spanKindExtractor);
+          }
+        });
+  }
 }
diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterAccess.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterAccess.java
@@ -0,0 +1,27 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.internal;
+
+import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import java.time.Instant;
+import javax.annotation.Nullable;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public interface InstrumenterAccess {
+
+  <REQUEST, RESPONSE> Context startAndEnd(
+      Instrumenter<REQUEST, RESPONSE> instrumenter,
+      Context parentContext,
+      REQUEST request,
+      @Nullable RESPONSE response,
+      @Nullable Throwable error,
+      Instant startTime,
+      Instant endTime);
+}
diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterBuilderAccess.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterBuilderAccess.java
@@ -0,0 +1,29 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.internal;
+
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.context.propagation.TextMapSetter;
+import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
+import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
+import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor;
+
+/**
+ * This class is internal and is hence not for public use. Its APIs are unstable and can change at
+ * any time.
+ */
+public interface InstrumenterBuilderAccess {
+
+  <REQUEST, RESPONSE> Instrumenter<REQUEST, RESPONSE> buildUpstreamInstrumenter(
+      InstrumenterBuilder<REQUEST, RESPONSE> builder,
+      TextMapGetter<REQUEST> getter,
+      SpanKindExtractor<REQUEST> spanKindExtractor);
+
+  <REQUEST, RESPONSE> Instrumenter<REQUEST, RESPONSE> buildDownstreamInstrumenter(
+      InstrumenterBuilder<REQUEST, RESPONSE> builder,
+      TextMapSetter<REQUEST> setter,
+      SpanKindExtractor<REQUEST> spanKindExtractor);
+}
diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterUtil.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/InstrumenterUtil.java
@@ -6,12 +6,12 @@
 package io.opentelemetry.instrumentation.api.internal;
 
 import io.opentelemetry.context.Context;
+import io.opentelemetry.context.propagation.TextMapGetter;
+import io.opentelemetry.context.propagation.TextMapSetter;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
-import java.lang.reflect.InvocationTargetException;
-import java.lang.reflect.Method;
+import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
+import io.opentelemetry.instrumentation.api.instrumenter.SpanKindExtractor;
 import java.time.Instant;
-import java.util.logging.Level;
-import java.util.logging.Logger;
 import javax.annotation.Nullable;
 
 /**
@@ -20,28 +20,16 @@
  */
 public final class InstrumenterUtil {
 
-  private static final Logger logger = Logger.getLogger(InstrumenterUtil.class.getName());
+  private static InstrumenterAccess instrumenterAccess;
+  private static InstrumenterBuilderAccess instrumenterBuilderAccess;
 
-  private static final Method startAndEndMethod;
+  public static void setInstrumenterAccess(InstrumenterAccess instrumenterAccess) {
+    InstrumenterUtil.instrumenterAccess = instrumenterAccess;
+  }
 
-  static {
-    Method method = null;
-    try {
-      method =
-          Instrumenter.class.getDeclaredMethod(
-              "startAndEnd",
-              Context.class,
-              Object.class,
-              Object.class,
-              Throwable.class,
-              Instant.class,
-              Instant.class);
-      method.setAccessible(true);
-    } catch (NoSuchMethodException e) {
-      logger.log(
-          Level.WARNING, "Could not get Instrumenter#startAndEnd() method with reflection", e);
-    }
-    startAndEndMethod = method;
+  public static void setInstrumenterBuilderAccess(
+      InstrumenterBuilderAccess instrumenterBuilderAccess) {
+    InstrumenterUtil.instrumenterBuilderAccess = instrumenterBuilderAccess;
   }
 
   public static <REQUEST, RESPONSE> Context startAndEnd(
@@ -52,19 +40,26 @@ public static <REQUEST, RESPONSE> Context startAndEnd(
       @Nullable Throwable error,
       Instant startTime,
       Instant endTime) {
+    // instrumenterAccess is guaranteed to be non-null here
+    return instrumenterAccess.startAndEnd(
+        instrumenter, parentContext, request, response, error, startTime, endTime);
+  }
+
+  public static <REQUEST, RESPONSE> Instrumenter<REQUEST, RESPONSE> buildUpstreamInstrumenter(
+      InstrumenterBuilder<REQUEST, RESPONSE> builder,
+      TextMapGetter<REQUEST> getter,
+      SpanKindExtractor<REQUEST> spanKindExtractor) {
+    // instrumenterBuilderAccess is guaranteed to be non-null here
+    return instrumenterBuilderAccess.buildUpstreamInstrumenter(builder, getter, spanKindExtractor);
+  }
 
-    if (startAndEndMethod == null) {
-      // already logged a warning when this class initialized
-      return parentContext;
-    }
-    try {
-      return (Context)
-          startAndEndMethod.invoke(
-              instrumenter, parentContext, request, response, error, startTime, endTime);
-    } catch (InvocationTargetException | IllegalAccessException e) {
-      logger.log(Level.WARNING, "Error occurred when calling Instrumenter#startAndEnd()", e);
-      return parentContext;
-    }
+  public static <REQUEST, RESPONSE> Instrumenter<REQUEST, RESPONSE> buildDownstreamInstrumenter(
+      InstrumenterBuilder<REQUEST, RESPONSE> builder,
+      TextMapSetter<REQUEST> setter,
+      SpanKindExtractor<REQUEST> spanKindExtractor) {
+    // instrumenterBuilderAccess is guaranteed to be non-null here
+    return instrumenterBuilderAccess.buildDownstreamInstrumenter(
+        builder, setter, spanKindExtractor);
   }
 
   private InstrumenterUtil() {}
diff --git a/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java b/instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/instrumenter/InstrumenterTest.java
@@ -7,6 +7,7 @@
 
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.equalTo;
+import static java.util.Collections.emptyMap;
 import static org.assertj.core.api.Assertions.entry;
 import static org.mockito.Mockito.when;
 
@@ -361,6 +362,58 @@ void client_parent() {
                             .hasParentSpanId("090a0b0c0d0e0f00")));
   }
 
+  @Test
+  void upstream_customSpanKind() {
+    Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
+        Instrumenter.<Map<String, String>, Map<String, String>>builder(
+                otelTesting.getOpenTelemetry(), "test", unused -> "span")
+            .buildUpstreamInstrumenter(new MapGetter(), SpanKindExtractor.alwaysInternal());
+
+    Map<String, String> request = new HashMap<>();
+    request.put("traceparent", "00-ff01020304050600ff0a0b0c0d0e0f00-090a0b0c0d0e0f00-01");
+    Context context = instrumenter.start(Context.root(), request);
+
+    SpanContext spanContext = Span.fromContext(context).getSpanContext();
+    assertThat(spanContext.isValid()).isTrue();
+
+    instrumenter.end(context, request, emptyMap(), null);
+
+    otelTesting
+        .assertTraces()
+        .hasTracesSatisfyingExactly(
+            trace ->
+                trace.hasSpansSatisfyingExactly(
+                    span ->
+                        span.hasName("span")
+                            .hasKind(SpanKind.INTERNAL)
+                            .hasTraceId("ff01020304050600ff0a0b0c0d0e0f00")
+                            .hasParentSpanId("090a0b0c0d0e0f00")));
+  }
+
+  @Test
+  void downstream_customSpanKind() {
+    Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
+        Instrumenter.<Map<String, String>, Map<String, String>>builder(
+                otelTesting.getOpenTelemetry(), "test", unused -> "span")
+            .buildDownstreamInstrumenter(Map::put, SpanKindExtractor.alwaysInternal());
+
+    Map<String, String> request = new HashMap<>();
+    Context context = instrumenter.start(Context.root(), request);
+
+    SpanContext spanContext = Span.fromContext(context).getSpanContext();
+    assertThat(spanContext.isValid()).isTrue();
+    assertThat(request).containsKey("traceparent");
+
+    instrumenter.end(context, request, emptyMap(), null);
+
+    otelTesting
+        .assertTraces()
+        .hasTracesSatisfyingExactly(
+            trace ->
+                trace.hasSpansSatisfyingExactly(
+                    span -> span.hasName("span").hasKind(SpanKind.INTERNAL)));
+  }
+
   @Test
   void operationListeners() {
     AtomicReference<Boolean> startContext = new AtomicReference<>();
@@ -486,10 +539,10 @@ void instrumentationVersion_default() {
     Instrumenter<Map<String, String>, Map<String, String>> instrumenter =
         builder.buildInstrumenter();
 
-    Context context = instrumenter.start(Context.root(), Collections.emptyMap());
+    Context context = instrumenter.start(Context.root(), emptyMap());
     assertThat(Span.fromContext(context)).isNotNull();
 
-    instrumenter.end(context, Collections.emptyMap(), Collections.emptyMap(), null);
+    instrumenter.end(context, emptyMap(), emptyMap(), null);
 
     // see the test-instrumentation.properties file
     InstrumentationScopeInfo expectedLibraryInfo =
@@ -511,10 +564,10 @@ void instrumentationVersion_custom() {
             .setInstrumentationVersion("1.0")
             .buildInstrumenter();
 
-    Context context = instrumenter.start(Context.root(), Collections.emptyMap());
+    Context context = instrumenter.start(Context.root(), emptyMap());
     assertThat(Span.fromContext(context)).isNotNull();
 
-    instrumenter.end(context, Collections.emptyMap(), Collections.emptyMap(), null);
+    instrumenter.end(context, emptyMap(), emptyMap(), null);
 
     otelTesting
         .assertTraces()
@@ -537,10 +590,10 @@ void schemaUrl() {
             .setSchemaUrl("https://opentelemetry.io/schemas/1.0.0")
             .buildInstrumenter();
 
-    Context context = instrumenter.start(Context.root(), Collections.emptyMap());
+    Context context = instrumenter.start(Context.root(), emptyMap());
     assertThat(Span.fromContext(context)).isNotNull();
 
-    instrumenter.end(context, Collections.emptyMap(), Collections.emptyMap(), null);
+    instrumenter.end(context, emptyMap(), emptyMap(), null);
 
     InstrumentationScopeInfo expectedLibraryInfo =
         InstrumentationScopeInfo.builder("test")
