Set route only on the SERVER span (open-telemetry#10290)

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/instrumenter/Instrumenter.java

@@ -11,6 +11,7 @@
 import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.api.trace.Tracer;
 import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.internal.HttpRouteState;
 import io.opentelemetry.instrumentation.api.internal.InstrumenterAccess;
 import io.opentelemetry.instrumentation.api.internal.InstrumenterUtil;
 import io.opentelemetry.instrumentation.api.internal.SupportabilityMetrics;
@@ -203,6 +204,9 @@ private Context doStart(Context parentContext, REQUEST request, @Nullable Instan
 
     if (localRoot) {
       context = LocalRootSpan.store(context, span);
+      if (spanKind == SpanKind.SERVER) {
+        HttpRouteState.updateSpan(context, span);
+      }
     }
 
     return spanSuppressor.storeInContext(context, spanKind, span);

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java

@@ -5,6 +5,7 @@
 
 package io.opentelemetry.instrumentation.api.internal;
 
+import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.ContextKey;
 import io.opentelemetry.context.ImplicitContextKeyed;
@@ -24,20 +25,36 @@ public static HttpRouteState fromContextOrNull(Context context) {
     return context.get(KEY);
   }
 
+  public static void updateSpan(Context context, Span span) {
+    HttpRouteState state = fromContextOrNull(context);
+    if (state != null) {
+      state.span = span;
+    }
+  }
+
+  // this method is used reflectively from InstrumentationApiContextBridging
   public static HttpRouteState create(
       @Nullable String method, @Nullable String route, int updatedBySourceOrder) {
-    return new HttpRouteState(method, route, updatedBySourceOrder);
+    return create(method, route, updatedBySourceOrder, null);
+  }
+
+  // this method is used reflectively from InstrumentationApiContextBridging
+  public static HttpRouteState create(
+      @Nullable String method, @Nullable String route, int updatedBySourceOrder, Span span) {
+    return new HttpRouteState(method, route, updatedBySourceOrder, span);
   }
 
   @Nullable private final String method;
   @Nullable private volatile String route;
   private volatile int updatedBySourceOrder;
+  @Nullable private volatile Span span;
 
   private HttpRouteState(
-      @Nullable String method, @Nullable String route, int updatedBySourceOrder) {
+      @Nullable String method, @Nullable String route, int updatedBySourceOrder, Span span) {
     this.method = method;
     this.updatedBySourceOrder = updatedBySourceOrder;
     this.route = route;
+    this.span = span;
   }
 
   @Override
@@ -59,6 +76,11 @@ public String getRoute() {
     return route;
   }
 
+  @Nullable
+  public Span getSpan() {
+    return span;
+  }
+
   public void update(
       @SuppressWarnings("unused")
           Context context, // context is used by the javaagent bridge instrumentation

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRoute.java

@@ -10,7 +10,6 @@
 import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.instrumentation.api.instrumenter.InstrumenterBuilder;
-import io.opentelemetry.instrumentation.api.instrumenter.LocalRootSpan;
 import io.opentelemetry.instrumentation.api.internal.HttpRouteState;
 import javax.annotation.Nullable;
 
@@ -97,16 +96,17 @@ public static <T, U> void update(
       HttpServerRouteBiGetter<T, U> httpRouteGetter,
       T arg1,
       U arg2) {
-    Span serverSpan = LocalRootSpan.fromContextOrNull(context);
+    HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
+    if (httpRouteState == null) {
+      return;
+    }
+    Span serverSpan = httpRouteState.getSpan();
     // even if the server span is not sampled, we have to continue - we need to compute the
     // http.route properly so that it can be captured by the server metrics
     if (serverSpan == null) {
       return;
     }
-    HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
-    if (httpRouteState == null) {
-      return;
-    }
+
     // special case for servlet filters, even when we have a route from previous filter see whether
     // the new route is better and if so use it instead
     boolean onlyIfBetterRoute =

instrumentation-api/src/test/java/io/opentelemetry/instrumentation/api/semconv/http/HttpServerRouteTest.java

@@ -12,6 +12,7 @@
 import static org.mockito.Mockito.when;
 
 import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.api.trace.SpanKind;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
@@ -39,7 +40,7 @@ void setUp() {
                 HttpServerRoute.builder(getter)
                     .setKnownMethods(new HashSet<>(singletonList("GET")))
                     .build())
-            .buildInstrumenter();
+            .buildInstrumenter(s -> SpanKind.SERVER);
   }
 
   @Test
@@ -61,6 +62,27 @@ void noLocalRootSpan() {
             span -> assertThat(span).hasName("parent"), span -> assertThat(span).hasName("test"));
   }
 
+  @Test
+  void nonServerRootSpan() {
+    Instrumenter<String, Void> testInstrumenter =
+        Instrumenter.<String, Void>builder(testing.getOpenTelemetry(), "test", s -> s)
+            .addContextCustomizer(
+                HttpServerRoute.builder(getter)
+                    .setKnownMethods(new HashSet<>(singletonList("GET")))
+                    .build())
+            .buildInstrumenter(s -> SpanKind.INTERNAL);
+
+    Context context = testInstrumenter.start(Context.root(), "test");
+    assertNull(HttpServerRoute.get(context));
+
+    HttpServerRoute.update(context, HttpServerRouteSource.SERVER, "/get/:id");
+
+    testInstrumenter.end(context, "test", null, null);
+
+    assertNull(HttpServerRoute.get(context));
+    assertThat(testing.getSpans()).satisfiesExactly(span -> assertThat(span).hasName("test"));
+  }
+
   @Test
   void shouldSetRoute() {
     when(getter.getHttpRequestMethod("test")).thenReturn("GET");

instrumentation/opentelemetry-api/opentelemetry-api-1.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/opentelemetryapi/context/InstrumentationApiContextBridging.java

@@ -59,12 +59,14 @@ final class InstrumentationApiContextBridging {
   private static final MethodHandle AGENT_GET_METHOD;
   private static final MethodHandle AGENT_GET_ROUTE;
   private static final MethodHandle AGENT_GET_UPDATED_BY_SOURCE_ORDER;
+  private static final MethodHandle AGENT_GET_SPAN;
 
   private static final Class<?> APPLICATION_HTTP_ROUTE_STATE;
   private static final MethodHandle APPLICATION_CREATE;
   private static final MethodHandle APPLICATION_GET_METHOD;
   private static final MethodHandle APPLICATION_GET_ROUTE;
   private static final MethodHandle APPLICATION_GET_UPDATED_BY_SOURCE_ORDER;
+  private static final MethodHandle APPLICATION_GET_SPAN;
 
   static {
     MethodHandles.Lookup lookup = MethodHandles.lookup();
@@ -74,11 +76,13 @@ final class InstrumentationApiContextBridging {
     MethodHandle agentGetMethod = null;
     MethodHandle agentGetRoute = null;
     MethodHandle agentGetUpdatedBySourceOrder = null;
+    MethodHandle agentGetSpan = null;
     Class<?> applicationHttpRouteState = null;
     MethodHandle applicationCreate = null;
     MethodHandle applicationGetMethod = null;
     MethodHandle applicationGetRoute = null;
     MethodHandle applicationGetUpdatedBySourceOrder = null;
+    MethodHandle applicationGetSpan = null;
 
     try {
       agentHttpRouteState =
@@ -87,23 +91,43 @@ final class InstrumentationApiContextBridging {
           lookup.findStatic(
               agentHttpRouteState,
               "create",
-              MethodType.methodType(agentHttpRouteState, String.class, String.class, int.class));
+              MethodType.methodType(
+                  agentHttpRouteState,
+                  String.class,
+                  String.class,
+                  int.class,
+                  io.opentelemetry.api.trace.Span.class));
       agentGetMethod =
           lookup.findVirtual(agentHttpRouteState, "getMethod", MethodType.methodType(String.class));
       agentGetRoute =
           lookup.findVirtual(agentHttpRouteState, "getRoute", MethodType.methodType(String.class));
       agentGetUpdatedBySourceOrder =
           lookup.findVirtual(
               agentHttpRouteState, "getUpdatedBySourceOrder", MethodType.methodType(int.class));
+      agentGetSpan =
+          lookup.findVirtual(
+              agentHttpRouteState,
+              "getSpan",
+              MethodType.methodType(io.opentelemetry.api.trace.Span.class));
 
       applicationHttpRouteState =
           Class.forName("application.io.opentelemetry.instrumentation.api.internal.HttpRouteState");
-      applicationCreate =
-          lookup.findStatic(
-              applicationHttpRouteState,
-              "create",
-              MethodType.methodType(
-                  applicationHttpRouteState, String.class, String.class, int.class));
+      try {
+        applicationCreate =
+            lookup.findStatic(
+                applicationHttpRouteState,
+                "create",
+                MethodType.methodType(
+                    applicationHttpRouteState, String.class, String.class, int.class, Span.class));
+      } catch (NoSuchMethodException exception) {
+        // older instrumentation-api has only the variant that does not take span
+        applicationCreate =
+            lookup.findStatic(
+                applicationHttpRouteState,
+                "create",
+                MethodType.methodType(
+                    applicationHttpRouteState, String.class, String.class, int.class));
+      }
       applicationGetMethod =
           lookup.findVirtual(
               applicationHttpRouteState, "getMethod", MethodType.methodType(String.class));
@@ -115,6 +139,13 @@ final class InstrumentationApiContextBridging {
               applicationHttpRouteState,
               "getUpdatedBySourceOrder",
               MethodType.methodType(int.class));
+      try {
+        applicationGetSpan =
+            lookup.findVirtual(
+                applicationHttpRouteState, "getSpan", MethodType.methodType(Span.class));
+      } catch (NoSuchMethodException ignored) {
+        // not present in older instrumentation-api
+      }
     } catch (Throwable ignored) {
       // instrumentation-api may be absent on the classpath, or it might be an older version
     }
@@ -124,11 +155,13 @@ final class InstrumentationApiContextBridging {
     AGENT_GET_METHOD = agentGetMethod;
     AGENT_GET_ROUTE = agentGetRoute;
     AGENT_GET_UPDATED_BY_SOURCE_ORDER = agentGetUpdatedBySourceOrder;
+    AGENT_GET_SPAN = agentGetSpan;
     APPLICATION_HTTP_ROUTE_STATE = applicationHttpRouteState;
     APPLICATION_CREATE = applicationCreate;
     APPLICATION_GET_METHOD = applicationGetMethod;
     APPLICATION_GET_ROUTE = applicationGetRoute;
     APPLICATION_GET_UPDATED_BY_SOURCE_ORDER = applicationGetUpdatedBySourceOrder;
+    APPLICATION_GET_SPAN = applicationGetSpan;
   }
 
   @Nullable
@@ -151,12 +184,16 @@ final class InstrumentationApiContextBridging {
               APPLICATION_CREATE,
               AGENT_GET_METHOD,
               AGENT_GET_ROUTE,
-              AGENT_GET_UPDATED_BY_SOURCE_ORDER),
+              AGENT_GET_UPDATED_BY_SOURCE_ORDER,
+              AGENT_GET_SPAN,
+              o -> o != null ? Bridging.toApplication((io.opentelemetry.api.trace.Span) o) : null),
           httpRouteStateConvert(
               AGENT_CREATE,
               APPLICATION_GET_METHOD,
               APPLICATION_GET_ROUTE,
-              APPLICATION_GET_UPDATED_BY_SOURCE_ORDER));
+              APPLICATION_GET_UPDATED_BY_SOURCE_ORDER,
+              APPLICATION_GET_SPAN,
+              o -> o != null ? Bridging.toAgentOrNull((Span) o) : null));
     } catch (Throwable ignored) {
       return null;
     }
@@ -166,12 +203,18 @@ private static Function<Object, Object> httpRouteStateConvert(
       MethodHandle create,
       MethodHandle getMethod,
       MethodHandle getRoute,
-      MethodHandle getUpdatedBySourceOrder) {
+      MethodHandle getUpdatedBySourceOrder,
+      MethodHandle getSpan,
+      Function<Object, Object> convertSpan) {
     return httpRouteHolder -> {
       try {
         String method = (String) getMethod.invoke(httpRouteHolder);
         String route = (String) getRoute.invoke(httpRouteHolder);
         int updatedBySourceOrder = (int) getUpdatedBySourceOrder.invoke(httpRouteHolder);
+        if (create.type().parameterCount() == 4 && getSpan != null) {
+          Object span = convertSpan.apply(getSpan.invoke(httpRouteHolder));
+          return create.invoke(method, route, updatedBySourceOrder, span);
+        }
         return create.invoke(method, route, updatedBySourceOrder);
       } catch (Throwable e) {
         return null;

instrumentation/opentelemetry-instrumentation-api/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/instrumentationapi/HttpRouteStateInstrumentation.java

@@ -8,10 +8,12 @@
 import static net.bytebuddy.matcher.ElementMatchers.named;
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 
+import application.io.opentelemetry.api.trace.Span;
 import application.io.opentelemetry.context.Context;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
 import io.opentelemetry.javaagent.extension.instrumentation.TypeTransformer;
 import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.context.AgentContextStorage;
+import io.opentelemetry.javaagent.instrumentation.opentelemetryapi.trace.Bridging;
 import net.bytebuddy.asm.Advice;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.matcher.ElementMatcher;
@@ -31,6 +33,11 @@ public void transform(TypeTransformer transformer) {
             .and(takesArgument(1, int.class))
             .and(takesArgument(2, String.class)),
         this.getClass().getName() + "$UpdateAdvice");
+    transformer.applyAdviceToMethod(
+        named("updateSpan")
+            .and(takesArgument(0, named("application.io.opentelemetry.context.Context")))
+            .and(takesArgument(1, named("application.io.opentelemetry.api.trace.Span"))),
+        this.getClass().getName() + "$UpdateSpanAdvice");
   }
 
   @SuppressWarnings("unused")
@@ -54,4 +61,17 @@ public static void onEnter(
       agentRouteState.update(agentContext, updatedBySourceOrder, route);
     }
   }
+
+  @SuppressWarnings("unused")
+  public static class UpdateSpanAdvice {
+    @Advice.OnMethodEnter(suppress = Throwable.class)
+    public static void onEnter(
+        @Advice.Argument(0) Context applicationContext, @Advice.Argument(1) Span applicationSpan) {
+
+      io.opentelemetry.context.Context agentContext =
+          AgentContextStorage.getAgentContext(applicationContext);
+      io.opentelemetry.instrumentation.api.internal.HttpRouteState.updateSpan(
+          agentContext, Bridging.toAgentOrNull(applicationSpan));
+    }
+  }
 }