Add http.route to the server span when ServerSpanNaming is updated (

open-telemetry#5086) * Add `http.route` to the server span when `ServerSpanNaming` is updated * fix camel tests * fix test compilation failure * assert route in camel instrumentation
RashmiRam · May 23, 2022 · e640958 · e640958
1 parent 0434bad
commit e640958
Show file tree

Hide file tree

Showing 53 changed files with 419 additions and 132 deletions.
diff --git a/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/server/ServerSpanNaming.java b/...ation-api/src/main/java/io/opentelemetry/instrumentation/api/server/ServerSpanNaming.java
@@ -9,13 +9,15 @@
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.ContextKey;
 import io.opentelemetry.instrumentation.api.instrumenter.ContextCustomizer;
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes;
 import javax.annotation.Nullable;
 
+// TODO: move to ...instrumenter.http and rename to HttpRouteHolder (?)
 /** Helper container for tracking whether instrumentation should update server span name or not. */
 public final class ServerSpanNaming {
 
   private static final ContextKey<ServerSpanNaming> CONTEXT_KEY =
-      ContextKey.named("opentelemetry-servlet-span-naming-key");
+      ContextKey.named("opentelemetry-http-server-route-key");
 
   public static <REQUEST> ContextCustomizer<REQUEST> get() {
     return (context, request, startAttributes) -> {
@@ -27,9 +29,7 @@ public static <REQUEST> ContextCustomizer<REQUEST> get() {
   }
 
   private volatile Source updatedBySource;
-  // Length of the currently set name. This is used when setting name from a servlet filter
-  // to pick the most descriptive (longest) name.
-  private volatile int nameLength;
+  @Nullable private volatile String route;
 
   private ServerSpanNaming(Source initialSource) {
     this.updatedBySource = initialSource;
@@ -78,7 +78,7 @@ public static <T, U> void updateServerSpanName(
     if (serverSpanNaming == null) {
       String name = serverSpanName.get(context, arg1, arg2);
       if (name != null && !name.isEmpty()) {
-        serverSpan.updateName(name);
+        updateSpanData(serverSpan, name);
       }
       return;
     }
@@ -91,15 +91,33 @@ public static <T, U> void updateServerSpanName(
       if (name != null
           && !name.isEmpty()
           && (!onlyIfBetterName || serverSpanNaming.isBetterName(name))) {
-        serverSpan.updateName(name);
+        updateSpanData(serverSpan, name);
         serverSpanNaming.updatedBySource = source;
-        serverSpanNaming.nameLength = name.length();
+        serverSpanNaming.route = name;
       }
     }
   }
 
+  // TODO: instead of calling setAttribute() consider storing the route in context end retrieving it
+  // in the AttributesExtractor
+  private static void updateSpanData(Span serverSpan, String route) {
+    serverSpan.updateName(route);
+    serverSpan.setAttribute(SemanticAttributes.HTTP_ROUTE, route);
+  }
+
+  // This is used when setting name from a servlet filter to pick the most descriptive (longest)
+  // route.
   private boolean isBetterName(String name) {
-    return name.length() > nameLength;
+    String route = this.route;
+    int routeLength = route == null ? 0 : route.length();
+    return name.length() > routeLength;
+  }
+
+  // TODO: use that in HttpServerMetrics
+  @Nullable
+  public static String getRoute(Context context) {
+    ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
+    return serverSpanNaming == null ? null : serverSpanNaming.route;
   }
 
   public enum Source {

diff --git a/...on/akka/akka-http-10.0/javaagent/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy b/...on/akka/akka-http-10.0/javaagent/src/test/groovy/AkkaHttpServerInstrumentationTest.groovy
@@ -3,8 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import io.opentelemetry.api.common.AttributeKey
 import io.opentelemetry.instrumentation.test.AgentTestTrait
 import io.opentelemetry.instrumentation.test.base.HttpServerTest
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
 
 abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest<Object> implements AgentTestTrait {
 
@@ -28,6 +30,13 @@ abstract class AkkaHttpServerInstrumentationTest extends HttpServerTest<Object>
   boolean testCapturedHttpHeaders() {
     false
   }
+
+  @Override
+  Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
+    def attributes = super.httpAttributes(endpoint)
+    attributes.remove(SemanticAttributes.HTTP_ROUTE)
+    attributes
+  }
 }
 
 class AkkaHttpServerInstrumentationTestSync extends AkkaHttpServerInstrumentationTest {

diff --git a/.../io/opentelemetry/javaagent/instrumentation/apachecamel/decorators/HttpSpanDecorator.java b/.../io/opentelemetry/javaagent/instrumentation/apachecamel/decorators/HttpSpanDecorator.java
@@ -121,6 +121,7 @@ private static boolean shouldUpdateServerSpanName(
   private void updateServerSpanName(Span serverSpan, Exchange exchange, Endpoint endpoint) {
     String path = getPath(exchange, endpoint);
     if (path != null) {
+      // TODO should update SERVER span name/route using ServerSpanNaming
       serverSpan.updateName(path);
     }
   }

diff --git a/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy b/...c/test/groovy/io/opentelemetry/javaagent/instrumentation/apachecamel/RestCamelTest.groovy
@@ -5,8 +5,6 @@
 
 package io.opentelemetry.javaagent.instrumentation.apachecamel
 
-import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
-
 import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
 import io.opentelemetry.instrumentation.test.RetryOnAddressAlreadyInUseTrait
 import io.opentelemetry.instrumentation.test.utils.PortUtils
@@ -20,6 +18,7 @@ import spock.lang.Shared
 import static io.opentelemetry.api.trace.SpanKind.CLIENT
 import static io.opentelemetry.api.trace.SpanKind.INTERNAL
 import static io.opentelemetry.api.trace.SpanKind.SERVER
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
 
 class RestCamelTest extends AgentInstrumentationSpecification implements RetryOnAddressAlreadyInUseTrait {
 
@@ -100,6 +99,8 @@ class RestCamelTest extends AgentInstrumentationSpecification implements RetryOn
             "$SemanticAttributes.NET_PEER_PORT" Long
             "$SemanticAttributes.HTTP_SERVER_NAME" String
             "$SemanticAttributes.NET_TRANSPORT" IP_TCP
+            // TODO: camel instrumentation does not use ServerSpanNaming to update the route, so the matched route is provided by the servlet instrumentation
+            "$SemanticAttributes.HTTP_ROUTE" "/*"
           }
         }
         it.span(3) {

diff --git a/...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy b/...lemetry/javaagent/instrumentation/apachecamel/TwoServicesWithDirectClientCamelTest.groovy
@@ -5,8 +5,6 @@
 
 package io.opentelemetry.javaagent.instrumentation.apachecamel
 
-import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
-
 import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
 import io.opentelemetry.instrumentation.test.RetryOnAddressAlreadyInUseTrait
 import io.opentelemetry.instrumentation.test.utils.PortUtils
@@ -22,6 +20,7 @@ import spock.lang.Shared
 import static io.opentelemetry.api.trace.SpanKind.CLIENT
 import static io.opentelemetry.api.trace.SpanKind.INTERNAL
 import static io.opentelemetry.api.trace.SpanKind.SERVER
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
 
 class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecification implements RetryOnAddressAlreadyInUseTrait {
 
@@ -136,6 +135,8 @@ class TwoServicesWithDirectClientCamelTest extends AgentInstrumentationSpecifica
             "$SemanticAttributes.HTTP_SERVER_NAME" String
             "$SemanticAttributes.NET_TRANSPORT" IP_TCP
             "$SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH" Long
+            // TODO: camel instrumentation does not use ServerSpanNaming to update the route, so the matched route is provided by the servlet instrumentation
+            "$SemanticAttributes.HTTP_ROUTE" "/*"
           }
         }
         it.span(5) {

diff --git a/...groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy b/...groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy
@@ -29,6 +29,7 @@ import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEn
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD
+import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.NOT_FOUND
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.QUERY_PARAM
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.REDIRECT
@@ -38,12 +39,23 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest<Server> {
 
   abstract ServerBuilder configureServer(ServerBuilder serverBuilder)
 
+  @Override
+  String expectedHttpRoute(ServerEndpoint endpoint) {
+    switch (endpoint) {
+      case NOT_FOUND:
+        // TODO(anuraaga): Revisit this when applying instrumenters to more libraries, Armeria
+        // currently reports '/*' which is a fallback route.
+        return "/*"
+      default:
+        return super.expectedHttpRoute(endpoint)
+    }
+  }
+
   @Override
   List<AttributeKey<?>> extraAttributes() {
     [
       SemanticAttributes.HTTP_REQUEST_CONTENT_LENGTH,
       SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH,
-      SemanticAttributes.HTTP_ROUTE,
       SemanticAttributes.HTTP_SERVER_NAME,
       SemanticAttributes.NET_PEER_NAME,
       SemanticAttributes.NET_TRANSPORT

diff --git a/instrumentation/dropwizard/dropwizard-testing/src/test/groovy/DropwizardTest.groovy b/instrumentation/dropwizard/dropwizard-testing/src/test/groovy/DropwizardTest.groovy
@@ -93,14 +93,14 @@ class DropwizardTest extends HttpServerTest<DropwizardTestSupport> implements Ag
   }
 
   @Override
-  String expectedServerSpanName(ServerEndpoint endpoint) {
+  String expectedHttpRoute(ServerEndpoint endpoint) {
     switch (endpoint) {
-      case PATH_PARAM:
-        return "/path/{id}/param"
       case NOT_FOUND:
-        return "/*"
+        return getContextPath() + "/*"
+      case PATH_PARAM:
+        return getContextPath() + "/path/{id}/param"
       default:
-        return endpoint.resolvePath(address).path
+        return super.expectedHttpRoute(endpoint)
     }
   }
 

diff --git a/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy b/instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy
@@ -66,7 +66,7 @@ class GrailsTest extends HttpServerTest<ConfigurableApplicationContext> implemen
   }
 
   @Override
-  String expectedServerSpanName(ServerEndpoint endpoint) {
+  String expectedHttpRoute(ServerEndpoint endpoint) {
     if (endpoint == PATH_PARAM) {
       return getContextPath() + "/test/path"
     } else if (endpoint == QUERY_PARAM) {

diff --git a/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy b/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy
@@ -3,9 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import io.opentelemetry.api.common.AttributeKey
 import io.opentelemetry.instrumentation.test.AgentTestTrait
 import io.opentelemetry.instrumentation.test.base.HttpServerTest
-import java.nio.charset.StandardCharsets
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
 import org.glassfish.grizzly.filterchain.BaseFilter
 import org.glassfish.grizzly.filterchain.FilterChain
 import org.glassfish.grizzly.filterchain.FilterChainBuilder
@@ -25,6 +26,7 @@ import org.glassfish.grizzly.nio.transport.TCPNIOTransportBuilder
 import org.glassfish.grizzly.utils.DelayedExecutor
 import org.glassfish.grizzly.utils.IdleTimeoutFilter
 
+import java.nio.charset.StandardCharsets
 import java.util.concurrent.Executors
 
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED
@@ -61,6 +63,18 @@ class GrizzlyFilterchainServerTest extends HttpServerTest<HttpServer> implements
     transport.shutdownNow()
   }
 
+  @Override
+  Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
+    def attributes = super.httpAttributes(endpoint)
+    attributes.remove(SemanticAttributes.HTTP_ROUTE)
+    attributes
+  }
+
+  @Override
+  String expectedServerSpanName(ServerEndpoint endpoint) {
+    return "HTTP GET"
+  }
+
   @Override
   boolean testException() {
     // justification: grizzly async closes the channel which
@@ -230,9 +244,4 @@ class GrizzlyFilterchainServerTest extends HttpServerTest<HttpServer> implements
       }
     }
   }
-
-  @Override
-  String expectedServerSpanName(ServerEndpoint endpoint) {
-    return "HTTP GET"
-  }
 }
diff --git a/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyTest.groovy b/instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyTest.groovy
@@ -3,8 +3,10 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
+import io.opentelemetry.api.common.AttributeKey
 import io.opentelemetry.instrumentation.test.AgentTestTrait
 import io.opentelemetry.instrumentation.test.base.HttpServerTest
+import io.opentelemetry.semconv.trace.attributes.SemanticAttributes
 import org.glassfish.grizzly.http.server.HttpHandler
 import org.glassfish.grizzly.http.server.HttpServer
 import org.glassfish.grizzly.http.server.Request
@@ -42,6 +44,18 @@ class GrizzlyTest extends HttpServerTest<HttpServer> implements AgentTestTrait {
     return server
   }
 
+  @Override
+  Set<AttributeKey<?>> httpAttributes(ServerEndpoint endpoint) {
+    def attributes = super.httpAttributes(endpoint)
+    attributes.remove(SemanticAttributes.HTTP_ROUTE)
+    attributes
+  }
+
+  @Override
+  String expectedServerSpanName(ServerEndpoint endpoint) {
+    return "HTTP GET"
+  }
+
   @Override
   void stopServer(HttpServer server) {
     server.stop()
@@ -108,11 +122,6 @@ class GrizzlyTest extends HttpServerTest<HttpServer> implements AgentTestTrait {
     }
   }
 
-  @Override
-  String expectedServerSpanName(ServerEndpoint endpoint) {
-    return "HTTP GET"
-  }
-
   static class ExceptionHttpHandler extends HttpHandler {
 
     @Override

diff --git a/...ion/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy b/...ion/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JaxRsAnnotations1InstrumentationTest.groovy
@@ -36,6 +36,7 @@ class JaxRsAnnotations1InstrumentationTest extends AgentInstrumentationSpecifica
           kind SERVER
           hasNoParent()
           attributes {
+            "$SemanticAttributes.HTTP_ROUTE" paramName
           }
         }
         span(1) {

diff --git a/instrumentation/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JerseyTest.groovy b/instrumentation/jaxrs/jaxrs-1.0/javaagent/src/test/groovy/JerseyTest.groovy
@@ -38,9 +38,10 @@ class JerseyTest extends AgentInstrumentationSpecification {
     assertTraces(1) {
       trace(0, 2) {
         span(0) {
-          name expectedSpanName
+          name expectedRoute
           kind SERVER
           attributes {
+            "$SemanticAttributes.HTTP_ROUTE" expectedRoute
           }
         }
 
@@ -56,7 +57,7 @@ class JerseyTest extends AgentInstrumentationSpecification {
     }
 
     where:
-    resource           | expectedSpanName      | controllerName | expectedResponse
+    resource           | expectedRoute         | controllerName | expectedResponse
     "/test/hello/bob"  | "/test/hello/{name}"  | "Test1.hello"  | "Test1 bob!"
     "/test2/hello/bob" | "/test2/hello/{name}" | "Test2.hello"  | "Test2 bob!"
     "/test3/hi/bob"    | "/test3/hi/{name}"    | "Test3.hello"  | "Test3 bob!"
@@ -76,9 +77,10 @@ class JerseyTest extends AgentInstrumentationSpecification {
     assertTraces(1) {
       trace(0, 2) {
         span(0) {
-          name expectedSpanName
+          name expectedRoute
           kind SERVER
           attributes {
+            "$SemanticAttributes.HTTP_ROUTE" expectedRoute
           }
         }
         span(1) {
@@ -94,7 +96,7 @@ class JerseyTest extends AgentInstrumentationSpecification {
     }
 
     where:
-    resource        | expectedSpanName | controller1Name | expectedResponse
-    "/test3/nested" | "/test3/nested"  | "Test3.nested"  | "Test3 nested!"
+    resource        | expectedRoute   | controller1Name | expectedResponse
+    "/test3/nested" | "/test3/nested" | "Test3.nested"  | "Test3 nested!"
   }
 }
diff --git a/...mentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxRsFilterTest.groovy b/...mentation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxRsFilterTest.groovy
@@ -135,6 +135,7 @@ abstract class JaxRsFilterTest extends AgentInstrumentationSpecification {
           kind SERVER
           if (!runsOnServer()) {
             attributes {
+              "$SemanticAttributes.HTTP_ROUTE" parentResourceName
             }
           }
         }

diff --git a/...ation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxRsHttpServerTest.groovy b/...ation/jaxrs/jaxrs-2.0/jaxrs-2.0-common/testing/src/main/groovy/JaxRsHttpServerTest.groovy
@@ -3,8 +3,6 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
-
 import io.opentelemetry.api.common.AttributeKey
 import io.opentelemetry.instrumentation.test.AgentTestTrait
 import io.opentelemetry.instrumentation.test.asserts.TraceAssert
@@ -20,6 +18,7 @@ import static io.opentelemetry.api.trace.StatusCode.ERROR
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.PATH_PARAM
 import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.SUCCESS
+import static io.opentelemetry.semconv.trace.attributes.SemanticAttributes.NetTransportValues.IP_TCP
 import static java.util.concurrent.TimeUnit.SECONDS
 import static org.junit.jupiter.api.Assumptions.assumeTrue
 
@@ -287,6 +286,7 @@ abstract class JaxRsHttpServerTest<S> extends HttpServerTest<S> implements Agent
         "$SemanticAttributes.HTTP_SERVER_NAME" String
         "$SemanticAttributes.NET_TRANSPORT" IP_TCP
         "$SemanticAttributes.HTTP_RESPONSE_CONTENT_LENGTH" { it == null || it instanceof Long } // Optional
+        "$SemanticAttributes.HTTP_ROUTE" path
         if (fullUrl.getPath().endsWith(ServerEndpoint.CAPTURE_HEADERS.getPath())) {
           "http.request.header.x_test_request" { it == ["test"] }
           "http.response.header.x_test_response" { it == ["test"] }