Server span naming for servlet filters

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

@@ -28,6 +28,9 @@ public static Context init(Context context, Source initialSource) {
   }
 
   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;
 
   private ServerSpanNaming(Source initialSource) {
     this.updatedBySource = initialSource;
@@ -58,15 +61,24 @@ public static void updateServerSpanName(
       }
       return;
     }
-    if (source.order > serverSpanNaming.updatedBySource.order) {
+    // special case for servlet filters, even when we have a name from previous filter see whether
+    // the new name is better and if so use it instead
+    boolean tryToFindBetterName =
+        !source.useFirst && source.order == serverSpanNaming.updatedBySource.order;
+    if (source.order > serverSpanNaming.updatedBySource.order || tryToFindBetterName) {
       String name = serverSpanName.get();
-      if (name != null) {
+      if (name != null && (!tryToFindBetterName || serverSpanNaming.isBetterName(name))) {
         serverSpan.updateName(name);
         serverSpanNaming.updatedBySource = source;
+        serverSpanNaming.nameLength = name.length();
       }
     }
   }
 
+  private boolean isBetterName(String name) {
+    return name.length() > nameLength;
+  }
+
   // 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
@@ -82,13 +94,22 @@ public static void updateSource(Context context, Source source) {
 
   public enum Source {
     CONTAINER(1),
-    SERVLET(2),
-    CONTROLLER(3);
+    // for servlet filters we try to find the best name which isn't necessarily from the first
+    // filter that is called
+    FILTER(2, false),
+    SERVLET(3),
+    CONTROLLER(4);
 
     private final int order;
+    private final boolean useFirst;
 
     Source(int order) {
+      this(order, true);
+    }
+
+    Source(int order, boolean useFirst) {
       this.order = order;
+      this.useFirst = useFirst;
     }
   }
 }

instrumentation/jetty/jetty-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/common/JettyHandlerAdviceHelper.java

@@ -14,8 +14,8 @@
 
 public class JettyHandlerAdviceHelper {
   /** Shared method exit implementation for Jetty handler advices. */
-  public static <REQUEST, RESPONSE> void stopSpan(
-      ServletHttpServerTracer<REQUEST, RESPONSE> tracer,
+  public static <SERVLETCONTEXT, REQUEST, RESPONSE> void stopSpan(
+      ServletHttpServerTracer<SERVLETCONTEXT, REQUEST, RESPONSE> tracer,
       REQUEST request,
       RESPONSE response,
       Throwable throwable,
@@ -46,7 +46,8 @@ public static <REQUEST, RESPONSE> void stopSpan(
     }
 
     AtomicBoolean responseHandled = new AtomicBoolean(false);
-    ServletAccessor<REQUEST, RESPONSE> servletAccessor = tracer.getServletAccessor();
+    ServletAccessor<SERVLETCONTEXT, REQUEST, RESPONSE> servletAccessor =
+        tracer.getServletAccessor();
 
     // In case of async servlets wait for the actual response to be ready
     if (servletAccessor.isRequestAsyncStarted(request)) {

instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterInitAdvice.java

@@ -0,0 +1,23 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
+
+import io.opentelemetry.instrumentation.servlet.v3_0.Servlet3FilterConfigHolder;
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+import net.bytebuddy.asm.Advice;
+
+public class Servlet3FilterInitAdvice {
+
+  @Advice.OnMethodEnter(suppress = Throwable.class)
+  public static void filterInit(
+      @Advice.This Filter filter, @Advice.Argument(0) FilterConfig filterConfig) {
+    if (filterConfig == null) {
+      return;
+    }
+    Servlet3FilterConfigHolder.setFilterConfig(filter, filterConfig);
+  }
+}

instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java

@@ -26,7 +26,10 @@ public Servlet3InstrumentationModule() {
   public List<TypeInstrumentation> typeInstrumentations() {
     return asList(
         new AsyncContextInstrumentation(BASE_PACKAGE, adviceClassName(".AsyncDispatchAdvice")),
-        new ServletAndFilterInstrumentation(BASE_PACKAGE, adviceClassName(".Servlet3Advice")));
+        new ServletAndFilterInstrumentation(
+            BASE_PACKAGE,
+            adviceClassName(".Servlet3Advice"),
+            adviceClassName(".Servlet3FilterInitAdvice")));
   }
 
   private static String adviceClassName(String suffix) {

instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServletMappingTest.groovy → instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3MappingTest.groovy

@@ -19,7 +19,7 @@ import okhttp3.RequestBody
 import okhttp3.Response
 import spock.lang.Unroll
 
-abstract class AbstractServletMappingTest<SERVER, CONTEXT> extends AgentInstrumentationSpecification implements HttpServerTestTrait<SERVER> {
+abstract class AbstractServlet3MappingTest<SERVER, CONTEXT> extends AgentInstrumentationSpecification implements HttpServerTestTrait<SERVER> {
 
   abstract void addServlet(CONTEXT context, String path, Class<Servlet> servlet)
 

instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServletMappingTest.groovy → instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3MappingTest.groovy

@@ -11,7 +11,7 @@ import javax.servlet.http.HttpServletResponse
 import org.eclipse.jetty.server.Server
 import org.eclipse.jetty.servlet.ServletContextHandler
 
-class JettyServletMappingTest extends AbstractServletMappingTest<Server, ServletContextHandler> {
+class JettyServlet3MappingTest extends AbstractServlet3MappingTest<Server, ServletContextHandler> {
 
   @Override
   Server startServer(int port) {

instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3FilterMappingTest.groovy

@@ -0,0 +1,135 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+import javax.servlet.Filter
+import javax.servlet.FilterChain
+import javax.servlet.FilterConfig
+import javax.servlet.ServletException
+import javax.servlet.ServletRequest
+import javax.servlet.ServletResponse
+import javax.servlet.http.HttpServlet
+import javax.servlet.http.HttpServletRequest
+import javax.servlet.http.HttpServletResponse
+import org.apache.catalina.Context
+import org.apache.catalina.startup.Tomcat
+import org.apache.tomcat.util.descriptor.web.FilterDef
+import org.apache.tomcat.util.descriptor.web.FilterMap
+
+abstract class TomcatServlet3FilterMappingTest extends TomcatServlet3MappingTest {
+
+  void addFilter(Context servletContext, String path, Class<Filter> filter) {
+    String name = UUID.randomUUID()
+    FilterDef filterDef = new FilterDef()
+    filterDef.setFilter(filter.newInstance())
+    filterDef.setFilterName(name)
+    servletContext.addFilterDef(filterDef)
+    FilterMap filterMap = new FilterMap()
+    filterMap.setFilterName(name)
+    filterMap.addURLPattern(path)
+    servletContext.addFilterMap(filterMap)
+  }
+
+  void addFilterWithServletName(Context servletContext, String servletName, Class<Filter> filter) {
+    String name = UUID.randomUUID()
+    FilterDef filterDef = new FilterDef()
+    filterDef.setFilter(filter.newInstance())
+    filterDef.setFilterName(name)
+    servletContext.addFilterDef(filterDef)
+    FilterMap filterMap = new FilterMap()
+    filterMap.setFilterName(name)
+    filterMap.addServletName(servletName)
+    servletContext.addFilterMap(filterMap)
+  }
+
+  static class TestFilter implements Filter {
+    @Override
+    void init(FilterConfig filterConfig) throws ServletException {
+    }
+
+    @Override
+    void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException {
+      if (servletRequest.getAttribute("firstFilterCalled") != null) {
+        servletRequest.setAttribute("testFilterCalled", Boolean.TRUE)
+        filterChain.doFilter(servletRequest, servletResponse)
+      } else {
+        throw new IllegalStateException("First filter should have been called.")
+      }
+    }
+
+    @Override
+    void destroy() {
+    }
+  }
+
+  static class FirstFilter implements Filter {
+    @Override
+    void init(FilterConfig filterConfig) throws ServletException {
+    }
+
+    @Override
+    void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException {
+      servletRequest.setAttribute("firstFilterCalled", Boolean.TRUE)
+      filterChain.doFilter(servletRequest, servletResponse)
+    }
+
+    @Override
+    void destroy() {
+    }
+  }
+
+  static class LastFilter implements Filter {
+
+    @Override
+    void init(FilterConfig filterConfig) throws ServletException {
+    }
+
+    @Override
+    void doFilter(ServletRequest servletRequest, ServletResponse servletResponse, FilterChain filterChain) throws IOException, ServletException {
+      if (servletRequest.getAttribute("testFilterCalled") != null) {
+        HttpServletResponse response = (HttpServletResponse) servletResponse
+        response.getWriter().write("Ok")
+        response.setStatus(HttpServletResponse.SC_OK)
+      } else {
+        filterChain.doFilter(servletRequest, servletResponse)
+      }
+    }
+
+    @Override
+    void destroy() {
+    }
+  }
+
+  static class DefaultServlet extends HttpServlet {
+    protected void service(HttpServletRequest req, HttpServletResponse resp) {
+      throw new IllegalStateException("Servlet should not have been called, filter should have handled the request.")
+    }
+  }
+}
+
+class TomcatServlet3FilterUrlPatternMappingTest extends TomcatServlet3FilterMappingTest {
+  @Override
+  protected void setupServlets(Context context) {
+    addFilter(context, "/*", FirstFilter)
+    addFilter(context, "/prefix/*", TestFilter)
+    addFilter(context, "*.suffix", TestFilter)
+    addFilter(context, "/*", LastFilter)
+  }
+}
+
+class TomcatServlet3FilterServletNameMappingTest extends TomcatServlet3FilterMappingTest {
+  @Override
+  protected void setupServlets(Context context) {
+    Tomcat.addServlet(context, "prefix-servlet", DefaultServlet.newInstance())
+    context.addServletMappingDecoded("/prefix/*", "prefix-servlet")
+    Tomcat.addServlet(context, "suffix-servlet", DefaultServlet.newInstance())
+    context.addServletMappingDecoded("*.suffix", "suffix-servlet")
+
+    addFilter(context, "/*", FirstFilter)
+    addFilterWithServletName(context, "prefix-servlet", TestFilter)
+    addFilterWithServletName(context, "suffix-servlet", TestFilter)
+    addFilterWithServletName(context, "prefix-servlet", LastFilter)
+    addFilterWithServletName(context, "suffix-servlet", LastFilter)
+  }
+}

instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServletMappingTest.groovy → instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3MappingTest.groovy

@@ -10,7 +10,7 @@ import org.apache.catalina.startup.Tomcat
 import org.apache.tomcat.JarScanFilter
 import org.apache.tomcat.JarScanType
 
-class TomcatServletMappingTest extends AbstractServletMappingTest<Tomcat, Context> {
+class TomcatServlet3MappingTest extends AbstractServlet3MappingTest<Tomcat, Context> {
 
   @Override
   Tomcat startServer(int port) {

instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3FilterConfigHolder.java

@@ -0,0 +1,26 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.instrumentation.servlet.v3_0;
+
+import io.opentelemetry.instrumentation.api.caching.Cache;
+import javax.servlet.Filter;
+import javax.servlet.FilterConfig;
+
+/** Helper class for keeping track of FilterConfiguration instance passed to Filter init method. */
+public final class Servlet3FilterConfigHolder {
+  private static final Cache<Filter, FilterConfig> filterConfigs =
+      Cache.newBuilder().setWeakKeys().build();
+
+  public static FilterConfig getFilterConfig(Filter filter) {
+    return filterConfigs.get(filter);
+  }
+
+  public static void setFilterConfig(Filter filter, FilterConfig filterConfig) {
+    filterConfigs.put(filter, filterConfig);
+  }
+
+  private Servlet3FilterConfigHolder() {}
+}