Update HTTP span name extractors (#7730)

Implements open-telemetry/opentelemetry-specification#2998 --------- Co-authored-by: Trask Stalnaker <[email protected]>
open-telemetry · Feb 14, 2023 · ea237e3 · ea237e3
1 parent cde6c98
commit ea237e3
Show file tree

Hide file tree

Showing 113 changed files with 637 additions and 302 deletions.
diff --git a/.../src/test/java/com/example/javaagent/instrumentation/DemoServlet3InstrumentationTest.java b/.../src/test/java/com/example/javaagent/instrumentation/DemoServlet3InstrumentationTest.java
@@ -86,7 +86,7 @@ void shouldAddCustomHeader() throws Exception {
                 trace
                     .hasSize(1)
                     .hasSpansSatisfyingExactly(
-                        span -> span.hasName("/servlet").hasKind(SpanKind.SERVER)));
+                        span -> span.hasName("GET /servlet").hasKind(SpanKind.SERVER)));
 
     var traceId = instrumentation.spans().get(0).getTraceId();
     assertEquals(traceId, response.header("X-server-id"));

diff --git a/...distro/smoke-tests/src/test/java/com/example/javaagent/smoketest/SpringBootSmokeTest.java b/...distro/smoke-tests/src/test/java/com/example/javaagent/smoketest/SpringBootSmokeTest.java
@@ -47,7 +47,7 @@ public void springBootSmokeTestOnJDK() throws IOException, InterruptedException
     Assertions.assertEquals(1, response.headers("X-server-id").size());
     Assertions.assertTrue(TraceId.isValid(response.header("X-server-id")));
     Assertions.assertEquals("Hi!", response.body().string());
-    Assertions.assertEquals(1, countSpansByName(traces, "/greeting"));
+    Assertions.assertEquals(1, countSpansByName(traces, "GET /greeting"));
     Assertions.assertEquals(0, countSpansByName(traces, "WebController.greeting"));
     Assertions.assertEquals(1, countSpansByName(traces, "WebController.withSpan"));
     Assertions.assertEquals(2, countSpansByAttributeValue(traces, "custom", "demo"));

diff --git a/...es/extension/src/test/java/com/example/javaagent/smoketest/SpringBootIntegrationTest.java b/...es/extension/src/test/java/com/example/javaagent/smoketest/SpringBootIntegrationTest.java
@@ -71,7 +71,7 @@ private void testAndVerify() throws IOException, InterruptedException {
     Assertions.assertEquals(1, response.headers("X-server-id").size());
     Assertions.assertTrue(TraceId.isValid(response.header("X-server-id")));
     Assertions.assertEquals("Hi!", response.body().string());
-    Assertions.assertEquals(1, countSpansByName(traces, "/greeting"));
+    Assertions.assertEquals(1, countSpansByName(traces, "GET /greeting"));
     Assertions.assertEquals(0, countSpansByName(traces, "WebController.greeting"));
     Assertions.assertEquals(1, countSpansByName(traces, "WebController.withSpan"));
     Assertions.assertEquals(2, countSpansByAttributeValue(traces, "custom", "demo"));

diff --git a/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java b/...src/main/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolder.java
@@ -28,13 +28,31 @@ public final class HttpRouteHolder {
   /**
    * Returns a {@link ContextCustomizer} that initializes a {@link HttpRouteHolder} in the {@link
    * Context} returned from {@link Instrumenter#start(Context, Object)}.
+   *
+   * @deprecated Use {@link #create(HttpServerAttributesGetter)} instead.
    */
+  @Deprecated
   public static <REQUEST> ContextCustomizer<REQUEST> get() {
     return (context, request, startAttributes) -> {
       if (HttpRouteState.fromContextOrNull(context) != null) {
         return context;
       }
-      return context.with(HttpRouteState.create(0, null));
+      return context.with(HttpRouteState.create(null, null, 0));
+    };
+  }
+
+  /**
+   * Returns a {@link ContextCustomizer} that initializes a {@link HttpRouteHolder} in the {@link
+   * Context} returned from {@link Instrumenter#start(Context, Object)}.
+   */
+  public static <REQUEST> ContextCustomizer<REQUEST> create(
+      HttpServerAttributesGetter<REQUEST, ?> getter) {
+    return (context, request, startAttributes) -> {
+      if (HttpRouteState.fromContextOrNull(context) != null) {
+        return context;
+      }
+      String method = getter.getMethod(request);
+      return context.with(HttpRouteState.create(method, null, 0));
     };
   }
 
@@ -103,10 +121,10 @@ public static <T, U> void updateHttpRoute(
     }
     HttpRouteState httpRouteState = HttpRouteState.fromContextOrNull(context);
     if (httpRouteState == null) {
+      // TODO: remove this branch?
       String httpRoute = httpRouteGetter.get(context, arg1, arg2);
       if (httpRoute != null && !httpRoute.isEmpty()) {
-        // update both span name and attribute, since there's no HttpRouteHolder in the context
-        serverSpan.updateName(httpRoute);
+        // update just the attribute - without http.method we can't compute a proper span name here
         serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, httpRoute);
       }
       return;
@@ -123,7 +141,7 @@ public static <T, U> void updateHttpRoute(
 
         // update just the span name - the attribute will be picked up by the
         // HttpServerAttributesExtractor at the end of request processing
-        serverSpan.updateName(route);
+        updateSpanName(serverSpan, httpRouteState, route);
 
         httpRouteState.update(context, source.order, route);
       }
@@ -138,6 +156,15 @@ private static boolean isBetterRoute(HttpRouteState httpRouteState, String name)
     return name.length() > routeLength;
   }
 
+  private static void updateSpanName(Span serverSpan, HttpRouteState httpRouteState, String route) {
+    String method = httpRouteState.getMethod();
+    // method should never really be null - but in case it for some reason is, we'll rely on the
+    // span name extractor behavior
+    if (method != null) {
+      serverSpan.updateName(method + " " + route);
+    }
+  }
+
   /**
    * Returns the {@code http.route} attribute value that's stored in the {@code context}, or null if
    * it was not set before.

diff --git a/...in/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanNameExtractor.java b/...in/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanNameExtractor.java
@@ -33,15 +33,12 @@ private HttpSpanNameExtractor(HttpCommonAttributesGetter<REQUEST, ?> getter) {
 
   @Override
   public String extract(REQUEST request) {
-    String route = extractRoute(request);
-    if (route != null) {
-      return route;
-    }
     String method = getter.getMethod(request);
+    String route = extractRoute(request);
     if (method != null) {
-      return "HTTP " + method;
+      return route == null ? method : method + " " + route;
     }
-    return "HTTP request";
+    return "HTTP";
   }
 
   @Nullable

diff --git a/...test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java b/...test/java/io/opentelemetry/instrumentation/api/instrumenter/http/HttpRouteHolderTest.java
@@ -5,34 +5,170 @@
 
 package io.opentelemetry.instrumentation.api.instrumenter.http;
 
+import static io.opentelemetry.sdk.testing.assertj.OpenTelemetryAssertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.mockito.Mockito.when;
 
+import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
 import io.opentelemetry.instrumentation.api.instrumenter.Instrumenter;
 import io.opentelemetry.sdk.testing.junit5.OpenTelemetryExtension;
+import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.mockito.Mock;
+import org.mockito.junit.jupiter.MockitoExtension;
 
+@ExtendWith(MockitoExtension.class)
 class HttpRouteHolderTest {
 
-  @RegisterExtension final OpenTelemetryExtension testing = OpenTelemetryExtension.create();
+  @RegisterExtension static final OpenTelemetryExtension testing = OpenTelemetryExtension.create();
 
-  @Test
-  void shouldSetRouteEvenIfSpanIsNotSampled() {
-    Instrumenter<String, Void> instrumenter =
+  @Mock HttpServerAttributesGetter<String, Void> getter;
+  Instrumenter<String, Void> instrumenter;
+
+  @BeforeEach
+  void setUp() {
+    instrumenter =
         Instrumenter.<String, Void>builder(testing.getOpenTelemetry(), "test", s -> s)
-            .addContextCustomizer(HttpRouteHolder.get())
+            .addContextCustomizer(HttpRouteHolder.create(getter))
             .buildInstrumenter();
+  }
 
-    Context context = instrumenter.start(Context.root(), "test");
+  @Test
+  void noLocalRootSpan() {
+    Span parentSpan =
+        testing.getOpenTelemetry().getTracer("test").spanBuilder("parent").startSpan();
+    parentSpan.end();
+
+    Context context = instrumenter.start(Context.root().with(parentSpan), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/get/:id");
+
+    instrumenter.end(context, "test", null, null);
 
+    assertNull(HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(
+            span -> assertThat(span).hasName("parent"), span -> assertThat(span).hasName("test"));
+  }
+
+  @Test
+  void shouldSetRoute() {
+    when(getter.getMethod("test")).thenReturn("GET");
+
+    Context context = instrumenter.start(Context.root(), "test");
     assertNull(HttpRouteHolder.getRoute(context));
 
     HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/get/:id");
 
+    instrumenter.end(context, "test", null, null);
+
     assertEquals("/get/:id", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("GET /get/:id"));
+  }
+
+  @Test
+  void shouldNotUpdateRoute_sameSource() {
+    when(getter.getMethod("test")).thenReturn("GET");
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/route1");
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/route2");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/route1", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("GET /route1"));
+  }
+
+  @Test
+  void shouldNotUpdateRoute_lowerOrderSource() {
+    when(getter.getMethod("test")).thenReturn("GET");
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.CONTROLLER, "/route1");
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/route2");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/route1", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("GET /route1"));
+  }
+
+  @Test
+  void shouldUpdateRoute_higherOrderSource() {
+    when(getter.getMethod("test")).thenReturn("GET");
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/route1");
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.CONTROLLER, "/route2");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/route2", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("GET /route2"));
+  }
+
+  @Test
+  void shouldUpdateRoute_betterMatch() {
+    when(getter.getMethod("test")).thenReturn("GET");
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.FILTER, "/a/route");
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.FILTER, "/a/much/better/route");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/a/much/better/route", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("GET /a/much/better/route"));
+  }
+
+  @Test
+  void shouldNotUpdateRoute_worseMatch() {
+    when(getter.getMethod("test")).thenReturn("GET");
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.FILTER, "/a/pretty/good/route");
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.FILTER, "/a");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/a/pretty/good/route", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans())
+        .satisfiesExactly(span -> assertThat(span).hasName("GET /a/pretty/good/route"));
   }
 
-  // TODO(mateusz): add more unit tests for HttpRouteHolder
+  @Test
+  void shouldNotUpdateSpanName_noMethod() {
+    when(getter.getMethod("test")).thenReturn(null);
+
+    Context context = instrumenter.start(Context.root(), "test");
+    assertNull(HttpRouteHolder.getRoute(context));
+
+    HttpRouteHolder.updateHttpRoute(context, HttpRouteSource.SERVLET, "/get/:id");
+
+    instrumenter.end(context, "test", null, null);
+
+    assertEquals("/get/:id", HttpRouteHolder.getRoute(context));
+    assertThat(testing.getSpans()).satisfiesExactly(span -> assertThat(span).hasName("test"));
+  }
 }
diff --git a/...ava/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanNameExtractorTest.java b/...ava/io/opentelemetry/instrumentation/api/instrumenter/http/HttpSpanNameExtractorTest.java
@@ -31,19 +31,19 @@ void routeAndMethod() {
     when(serverGetter.getRoute(anyMap())).thenReturn("/cats/{id}");
     when(serverGetter.getMethod(anyMap())).thenReturn("GET");
     assertThat(HttpSpanNameExtractor.create(serverGetter).extract(Collections.emptyMap()))
-        .isEqualTo("/cats/{id}");
+        .isEqualTo("GET /cats/{id}");
   }
 
   @Test
   void method() {
     when(clientGetter.getMethod(anyMap())).thenReturn("GET");
     assertThat(HttpSpanNameExtractor.create(clientGetter).extract(Collections.emptyMap()))
-        .isEqualTo("HTTP GET");
+        .isEqualTo("GET");
   }
 
   @Test
   void nothing() {
     assertThat(HttpSpanNameExtractor.create(clientGetter).extract(Collections.emptyMap()))
-        .isEqualTo("HTTP request");
+        .isEqualTo("HTTP");
   }
 }
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/internal/HttpRouteState.java
@@ -24,14 +24,18 @@ public static HttpRouteState fromContextOrNull(Context context) {
     return context.get(KEY);
   }
 
-  public static HttpRouteState create(int updatedBySourceOrder, @Nullable String route) {
-    return new HttpRouteState(updatedBySourceOrder, route);
+  public static HttpRouteState create(
+      @Nullable String method, @Nullable String route, int updatedBySourceOrder) {
+    return new HttpRouteState(method, route, updatedBySourceOrder);
   }
 
-  private volatile int updatedBySourceOrder;
+  @Nullable private final String method;
   @Nullable private volatile String route;
+  private volatile int updatedBySourceOrder;
 
-  private HttpRouteState(int updatedBySourceOrder, @Nullable String route) {
+  private HttpRouteState(
+      @Nullable String method, @Nullable String route, int updatedBySourceOrder) {
+    this.method = method;
     this.updatedBySourceOrder = updatedBySourceOrder;
     this.route = route;
   }
@@ -41,6 +45,11 @@ public Context storeInContext(Context context) {
     return context.with(KEY, this);
   }
 
+  @Nullable
+  public String getMethod() {
+    return method;
+  }
+
   public int getUpdatedBySourceOrder() {
     return updatedBySourceOrder;
   }

diff --git a/.../io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerSingletons.java b/.../io/opentelemetry/javaagent/instrumentation/akkahttp/server/AkkaHttpServerSingletons.java
@@ -36,7 +36,7 @@ httpAttributesGetter, new AkkaNetServerAttributesGetter())
                     .setCapturedResponseHeaders(CommonConfig.get().getServerResponseHeaders())
                     .build())
             .addOperationMetrics(HttpServerMetrics.get())
-            .addContextCustomizer(HttpRouteHolder.get())
+            .addContextCustomizer(HttpRouteHolder.create(httpAttributesGetter))
             .buildServerInstrumenter(AkkaHttpServerHeaders.INSTANCE);
   }
 

diff --git a/.../io/opentelemetry/javaagent/instrumentation/apachecamel/decorators/HttpSpanDecorator.java b/.../io/opentelemetry/javaagent/instrumentation/apachecamel/decorators/HttpSpanDecorator.java
@@ -74,11 +74,11 @@ protected static String getHttpMethod(Exchange exchange, Endpoint endpoint) {
   public String getOperationName(
       Exchange exchange, Endpoint endpoint, CamelDirection camelDirection) {
     // Based on HTTP component documentation:
-    String spanName = null;
-    if (shouldSetPathAsName(camelDirection)) {
-      spanName = getPath(exchange, endpoint);
+    String spanName = getHttpMethod(exchange, endpoint);
+    if (shouldAppendHttpRoute(camelDirection)) {
+      spanName = spanName + " " + getPath(exchange, endpoint);
     }
-    return (spanName == null ? getHttpMethod(exchange, endpoint) : spanName);
+    return spanName;
   }
 
   @Override
@@ -97,7 +97,7 @@ public void pre(
     attributes.put(SemanticAttributes.HTTP_METHOD, getHttpMethod(exchange, endpoint));
   }
 
-  static boolean shouldSetPathAsName(CamelDirection camelDirection) {
+  private static boolean shouldAppendHttpRoute(CamelDirection camelDirection) {
     return camelDirection == CamelDirection.INBOUND;
   }
 
@@ -119,7 +119,7 @@ public void updateServerSpanName(
       Exchange camelExchange,
       Endpoint camelEndpoint,
       CamelDirection camelDirection) {
-    if (!shouldSetPathAsName(camelDirection)) {
+    if (!shouldAppendHttpRoute(camelDirection)) {
       return;
     }
     HttpRouteHolder.updateHttpRoute(