Better server span name for Grails and Wicket (#2814)

instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java

@@ -0,0 +1,94 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.api.servlet;
+
+import io.opentelemetry.api.trace.Span;
+import io.opentelemetry.context.Context;
+import io.opentelemetry.context.ContextKey;
+import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
+import java.util.function.Supplier;
+
+/** 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");
+
+  public static Context init(Context context, Source initialSource) {
+    ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
+    if (serverSpanNaming != null) {
+      // TODO (trask) does this ever happen?
+      serverSpanNaming.updatedBySource = initialSource;
+      return context;
+    }
+    return context.with(CONTEXT_KEY, new ServerSpanNaming(initialSource));
+  }
+
+  private volatile Source updatedBySource;
+
+  private ServerSpanNaming(Source initialSource) {
+    this.updatedBySource = initialSource;
+  }
+
+  /**
+   * If there is a server span in the context, and {@link #init(Context, Source)} has been called to
+   * populate a {@code ServerSpanName} into the context, then this method will update the server
+   * span name using the provided {@link Supplier} if and only if the last {@link Source} to update
+   * the span name using this method has strictly lower priority than the provided {@link Source},
+   * and the value returned from the {@link Supplier} is non-null.
+   *
+   * <p>If there is a server span in the context, and {@link #init(Context, Source)} has NOT been
+   * called to populate a {@code ServerSpanName} into the context, then this method will update the
+   * server span name using the provided {@link Supplier} if the value returned from it is non-null.
+   */
+  public static void updateServerSpanName(
+      Context context, Source source, Supplier<String> serverSpanName) {
+    Span serverSpan = ServerSpan.fromContextOrNull(context);
+    if (serverSpan == null) {
+      return;
+    }
+    ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
+    if (serverSpanNaming == null) {
+      String name = serverSpanName.get();
+      if (name != null) {
+        serverSpan.updateName(name);
+      }
+      return;
+    }
+    if (source.order > serverSpanNaming.updatedBySource.order) {
+      String name = serverSpanName.get();
+      if (name != null) {
+        serverSpan.updateName(name);
+        serverSpanNaming.updatedBySource = source;
+      }
+    }
+  }
+
+  // TODO (trask) migrate the one usage (ServletHttpServerTracer) to ServerSpanNaming.init() once we
+  // migrate to new Instrumenters (see
+  // https://github.com/open-telemetry/opentelemetry-java-instrumentation/pull/2814#discussion_r617351334
+  // for the challenge with doing this now in the current Tracer structure, at least without some
+  // bigger changes, which we want to avoid in the Tracers as they are already deprecated)
+  @Deprecated
+  public static void updateSource(Context context, Source source) {
+    ServerSpanNaming serverSpanNaming = context.get(CONTEXT_KEY);
+    if (serverSpanNaming != null && source.order > serverSpanNaming.updatedBySource.order) {
+      serverSpanNaming.updatedBySource = source;
+    }
+  }
+
+  public enum Source {
+    CONTAINER(1),
+    SERVLET(2),
+    CONTROLLER(3);
+
+    private final int order;
+
+    Source(int order) {
+      this.order = order;
+    }
+  }
+}

instrumentation/armeria-1.3/testing/src/main/groovy/io/opentelemetry/instrumentation/armeria/v1_3/AbstractArmeriaHttpServerTest.groovy

@@ -49,6 +49,12 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest<Server> {
     ]
   }
 
+  @Override
+  boolean testNotFound() {
+    // currently span name is /notFound which indicates it won't be low-cardinality
+    false
+  }
+
   @Override
   Server startServer(int port) {
     ServerBuilder sb = Server.builder()
@@ -61,31 +67,31 @@ abstract class AbstractArmeriaHttpServerTest extends HttpServerTest<Server> {
       }
     }
 
-    sb.service(REDIRECT.path) {ctx, req ->
+    sb.service(REDIRECT.path) { ctx, req ->
       controller(REDIRECT) {
         HttpResponse.of(ResponseHeaders.of(HttpStatus.valueOf(REDIRECT.status), HttpHeaderNames.LOCATION, REDIRECT.body))
       }
     }
 
-    sb.service(ERROR.path) {ctx, req ->
+    sb.service(ERROR.path) { ctx, req ->
       controller(ERROR) {
         HttpResponse.of(HttpStatus.valueOf(ERROR.status), MediaType.PLAIN_TEXT_UTF_8, ERROR.body)
       }
     }
 
-    sb.service(EXCEPTION.path) {ctx, req ->
+    sb.service(EXCEPTION.path) { ctx, req ->
       controller(EXCEPTION) {
         throw new Exception(EXCEPTION.body)
       }
     }
 
-    sb.service("/query") {ctx, req ->
+    sb.service("/query") { ctx, req ->
       controller(QUERY_PARAM) {
         HttpResponse.of(HttpStatus.valueOf(QUERY_PARAM.status), MediaType.PLAIN_TEXT_UTF_8, "some=${QueryParams.fromQueryString(ctx.query()).get("some")}")
       }
     }
 
-    sb.service("/path/:id/param") {ctx, req ->
+    sb.service("/path/:id/param") { ctx, req ->
       controller(PATH_PARAM) {
         HttpResponse.of(HttpStatus.valueOf(PATH_PARAM.status), MediaType.PLAIN_TEXT_UTF_8, ctx.pathParam("id"))
       }

instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsTracer.java

@@ -5,8 +5,10 @@
 
 package io.opentelemetry.javaagent.instrumentation.grails;
 
-import io.opentelemetry.api.trace.Span;
+import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTROLLER;
+
 import io.opentelemetry.context.Context;
+import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming;
 import io.opentelemetry.instrumentation.api.servlet.ServletContextPath;
 import io.opentelemetry.instrumentation.api.tracer.BaseTracer;
 import org.grails.web.mapping.mvc.GrailsControllerUrlMappingInfo;
@@ -23,14 +25,17 @@ public Context startSpan(Object controller, String action) {
     return startSpan(spanNameForClass(controller.getClass()) + "." + action);
   }
 
-  public void nameServerSpan(
-      Context context, Span serverSpan, GrailsControllerUrlMappingInfo info) {
+  public void updateServerSpanName(Context context, GrailsControllerUrlMappingInfo info) {
+    ServerSpanNaming.updateServerSpanName(
+        context, CONTROLLER, () -> getServerSpanName(context, info));
+  }
+
+  private static String getServerSpanName(Context context, GrailsControllerUrlMappingInfo info) {
     String action =
         info.getActionName() != null
             ? info.getActionName()
             : info.getControllerClass().getDefaultAction();
-    serverSpan.updateName(
-        ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action));
+    return ServletContextPath.prepend(context, "/" + info.getControllerName() + "/" + action);
   }
 
   @Override

instrumentation/grails-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/grails/UrlMappingsInfoHandlerAdapterInstrumentation.java

@@ -13,9 +13,7 @@
 import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
 import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
 
-import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
-import io.opentelemetry.instrumentation.api.tracer.ServerSpan;
 import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
 import io.opentelemetry.javaagent.tooling.TypeInstrumentation;
 import java.util.Map;
@@ -48,11 +46,7 @@ public static void nameSpan(@Advice.Argument(2) Object handler) {
 
       if (handler instanceof GrailsControllerUrlMappingInfo) {
         Context parentContext = Java8BytecodeBridge.currentContext();
-        Span serverSpan = ServerSpan.fromContextOrNull(parentContext);
-        if (serverSpan != null) {
-          tracer()
-              .nameServerSpan(parentContext, serverSpan, (GrailsControllerUrlMappingInfo) handler);
-        }
+        tracer().updateServerSpanName(parentContext, (GrailsControllerUrlMappingInfo) handler);
       }
     }
   }

instrumentation/grails-3.0/javaagent/src/test/groovy/test/GrailsTest.groovy

@@ -58,8 +58,8 @@ class GrailsTest extends HttpServerTest<ConfigurableApplicationContext> implemen
       return getContextPath() + "/test/path"
     } else if (endpoint == QUERY_PARAM) {
       return getContextPath() + "/test/query"
-    } else if (endpoint == ERROR || endpoint == EXCEPTION) {
-      return getContextPath() + "/error/index"
+    } else if (endpoint == ERROR) {
+      return getContextPath() + "/test/error"
     } else if (endpoint == NOT_FOUND) {
       return getContextPath() + "/**"
     }
@@ -101,24 +101,17 @@ class GrailsTest extends HttpServerTest<ConfigurableApplicationContext> implemen
     endpoint == ERROR || endpoint == EXCEPTION || endpoint == NOT_FOUND
   }
 
-  @Override
-  int getErrorPageSpansCount(ServerEndpoint endpoint) {
-    endpoint == NOT_FOUND ? 0 : 1
-  }
-
   @Override
   boolean testPathParam() {
     true
   }
 
   @Override
   void errorPageSpans(TraceAssert trace, int index, Object parent, String method = "GET", ServerEndpoint endpoint) {
-    if (endpoint != NOT_FOUND) {
-      trace.span(index) {
-        name "ErrorController.index"
-        kind INTERNAL
-        attributes {
-        }
+    trace.span(index) {
+      name endpoint == NOT_FOUND ? "BasicErrorController.error" : "ErrorController.index"
+      kind INTERNAL
+      attributes {
       }
     }
   }

instrumentation/grails-3.0/javaagent/src/test/groovy/test/UrlMappings.groovy

@@ -7,15 +7,14 @@ package test
 
 class UrlMappings {
 
-    static mappings = {
-      "/success"(controller: 'test', action: 'success')
-      "/query"(controller: 'test', action: 'query')
-      "/redirect"(controller: 'test', action: 'redirect')
-      "/error-status"(controller: 'test', action: 'error')
-      "/exception"(controller: 'test', action: 'exception')
-      "/path/$id/param"(controller: 'test', action: 'path')
+  static mappings = {
+    "/success"(controller: 'test', action: 'success')
+    "/query"(controller: 'test', action: 'query')
+    "/redirect"(controller: 'test', action: 'redirect')
+    "/error-status"(controller: 'test', action: 'error')
+    "/exception"(controller: 'test', action: 'exception')
+    "/path/$id/param"(controller: 'test', action: 'path')
 
-      "500"(controller: 'error')
-      "404"(controller: 'error', action: 'notFound')
-    }
+    "500"(controller: 'error')
+  }
 }

instrumentation/grizzly-2.0/javaagent/src/test/groovy/GrizzlyFilterchainServerTest.groovy

@@ -141,9 +141,6 @@ class GrizzlyFilterchainServerTest extends HttpServerTest<HttpServer> implements
           break
         case "/exception":
           throw new Exception(EXCEPTION.body)
-        case "/notFound":
-          endpoint = NOT_FOUND
-          break
         case "/query?some=query":
           endpoint = QUERY_PARAM
           break

instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11HttpServerTracer.java

@@ -18,7 +18,7 @@ public static Jetty11HttpServerTracer tracer() {
   }
 
   public Context startServerSpan(HttpServletRequest request) {
-    return startSpan(request, "HTTP " + request.getMethod());
+    return startSpan(request, "HTTP " + request.getMethod(), false);
   }
 
   @Override

instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8HttpServerTracer.java

@@ -18,7 +18,7 @@ public static Jetty8HttpServerTracer tracer() {
   }
 
   public Context startServerSpan(HttpServletRequest request) {
-    return startSpan(request, "HTTP " + request.getMethod());
+    return startSpan(request, "HTTP " + request.getMethod(), false);
   }
 
   @Override

instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertyHttpServerTracer.java

@@ -17,7 +17,7 @@ public static LibertyHttpServerTracer tracer() {
   }
 
   public Context startSpan(HttpServletRequest request) {
-    return startSpan(request, "HTTP " + request.getMethod());
+    return startSpan(request, "HTTP " + request.getMethod(), false);
   }
 
   @Override

instrumentation/servlet/glassfish-testing/src/test/groovy/GlassFishServerTest.groovy

@@ -77,12 +77,4 @@ class GlassFishServerTest extends HttpServerTest<GlassFish> implements AgentTest
         break
     }
   }
-
-  @Override
-  String expectedServerSpanName(ServerEndpoint endpoint) {
-    if (endpoint == NOT_FOUND) {
-      return getContextPath() + "/*"
-    }
-    return super.expectedServerSpanName(endpoint)
-  }
 }