Server span naming for servlet filters (#2887)

* Server span naming for servlet filters * wildfly default servlet has empty mappings * jetty11 requires java11 * try a differnt way to disable jetty11 tests on java8 * Update instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5FilterMappingTest.groovy Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> * review fix * rework to use InstrumentationContext * remove debugging code * move MappingResolver to avoid ClassCastException on wildfly * Update instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3FilterMappingTest.groovy Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com> * review fixes Co-authored-by: Trask Stalnaker <trask.stalnaker@gmail.com> Co-authored-by: Mateusz Rzeszutek <mrzeszutek@splunk.com>
open-telemetry · May 6, 2021 · 357140c · 357140c
1 parent c9a3793
commit 357140c
Show file tree

Hide file tree

Showing 37 changed files with 1,078 additions and 213 deletions.
diff --git a/...trumentation/servlet/MappingResolver.java → ...entation/api/servlet/MappingResolver.java b/...trumentation/servlet/MappingResolver.java → ...entation/api/servlet/MappingResolver.java
@@ -3,7 +3,7 @@
  * SPDX-License-Identifier: Apache-2.0
  */
 
-package io.opentelemetry.instrumentation.servlet;
+package io.opentelemetry.instrumentation.api.servlet;
 
 import java.util.ArrayList;
 import java.util.Collection;
@@ -12,7 +12,10 @@
 import java.util.List;
 import java.util.Set;
 
-public class MappingResolver {
+/**
+ * Helper class for finding a mapping that matches current request from a collection of mappings.
+ */
+public final class MappingResolver {
   private final Set<String> exactMatches;
   private final List<WildcardMatcher> wildcardMatchers;
   private final boolean hasDefault;

diff --git a/...tion-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/...tion-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 onlyIfBetterName =
+        !source.useFirst && source.order == serverSpanNaming.updatedBySource.order;
+    if (source.order > serverSpanNaming.updatedBySource.order || onlyIfBetterName) {
       String name = serverSpanName.get();
-      if (name != null) {
+      if (name != null && (!onlyIfBetterName || 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;
     }
   }
 }
diff --git a/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java b/...src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java
@@ -10,9 +10,13 @@
 import io.opentelemetry.context.Context;
 import io.opentelemetry.context.Scope;
 import io.opentelemetry.instrumentation.api.servlet.AppServerBridge;
+import io.opentelemetry.instrumentation.api.servlet.MappingResolver;
 import io.opentelemetry.javaagent.instrumentation.api.CallDepthThreadLocalMap;
+import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
 import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
 import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterAdviceHelper;
+import javax.servlet.Filter;
+import javax.servlet.Servlet;
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
@@ -36,12 +40,24 @@ public static void onEnter(
 
     HttpServletRequest httpServletRequest = (HttpServletRequest) request;
 
+    boolean servlet = servletOrFilter instanceof Servlet;
+    MappingResolver mappingResolver;
+    if (servlet) {
+      mappingResolver =
+          InstrumentationContext.get(Servlet.class, MappingResolver.class)
+              .get((Servlet) servletOrFilter);
+    } else {
+      mappingResolver =
+          InstrumentationContext.get(Filter.class, MappingResolver.class)
+              .get((Filter) servletOrFilter);
+    }
+
     Context attachedContext = tracer().getServerContext(httpServletRequest);
     if (attachedContext != null) {
       // We are inside nested servlet/filter/app-server span, don't create new span
       if (tracer().needsRescoping(attachedContext)) {
         attachedContext =
-            tracer().updateContext(attachedContext, servletOrFilter, httpServletRequest);
+            tracer().updateContext(attachedContext, httpServletRequest, mappingResolver, servlet);
         scope = attachedContext.makeCurrent();
         return;
       }
@@ -50,7 +66,7 @@ public static void onEnter(
       // instrumentation, if needed update span with info from current request.
       Context currentContext = Java8BytecodeBridge.currentContext();
       Context updatedContext =
-          tracer().updateContext(currentContext, servletOrFilter, httpServletRequest);
+          tracer().updateContext(currentContext, httpServletRequest, mappingResolver, servlet);
       if (updatedContext != currentContext) {
         // runOnceUnderAppServer updated context, need to re-scope
         scope = updatedContext.makeCurrent();
@@ -65,15 +81,15 @@ public static void onEnter(
       // In case it was created by app server integration we need to update it with info from
       // current request.
       Context updatedContext =
-          tracer().updateContext(currentContext, servletOrFilter, httpServletRequest);
+          tracer().updateContext(currentContext, httpServletRequest, mappingResolver, servlet);
       if (currentContext != updatedContext) {
         // updateContext updated context, need to re-scope
         scope = updatedContext.makeCurrent();
       }
       return;
     }
 
-    context = tracer().startSpan(servletOrFilter, httpServletRequest);
+    context = tracer().startSpan(httpServletRequest, mappingResolver, servlet);
     scope = context.makeCurrent();
   }
 

diff --git a/...ava/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterInitAdvice.java b/...ava/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterInitAdvice.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
+
+import io.opentelemetry.instrumentation.api.servlet.MappingResolver;
+import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
+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;
+    }
+    InstrumentationContext.get(Filter.class, MappingResolver.class)
+        .putIfAbsent(filter, new Servlet3FilterMappingResolverFactory(filterConfig));
+  }
+}
diff --git a/...elemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterMappingResolverFactory.java b/...elemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterMappingResolverFactory.java
@@ -0,0 +1,55 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
+
+import io.opentelemetry.instrumentation.api.servlet.MappingResolver;
+import io.opentelemetry.instrumentation.servlet.naming.ServletFilterMappingResolverFactory;
+import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
+import java.util.Collection;
+import javax.servlet.FilterConfig;
+import javax.servlet.FilterRegistration;
+import javax.servlet.ServletContext;
+import javax.servlet.ServletRegistration;
+
+public class Servlet3FilterMappingResolverFactory
+    extends ServletFilterMappingResolverFactory<FilterRegistration>
+    implements ContextStore.Factory<MappingResolver> {
+  private final FilterConfig filterConfig;
+
+  public Servlet3FilterMappingResolverFactory(FilterConfig filterConfig) {
+    this.filterConfig = filterConfig;
+  }
+
+  @Override
+  protected FilterRegistration getFilterRegistration() {
+    String filterName = filterConfig.getFilterName();
+    ServletContext servletContext = filterConfig.getServletContext();
+    if (filterName == null || servletContext == null) {
+      return null;
+    }
+    return servletContext.getFilterRegistration(filterName);
+  }
+
+  @Override
+  protected Collection<String> getUrlPatternMappings(FilterRegistration filterRegistration) {
+    return filterRegistration.getUrlPatternMappings();
+  }
+
+  @Override
+  protected Collection<String> getServletNameMappings(FilterRegistration filterRegistration) {
+    return filterRegistration.getServletNameMappings();
+  }
+
+  @Override
+  protected Collection<String> getServletMappings(String servletName) {
+    ServletRegistration servletRegistration =
+        filterConfig.getServletContext().getServletRegistration(servletName);
+    if (servletRegistration == null) {
+      return null;
+    }
+    return servletRegistration.getMappings();
+  }
+}
diff --git a/...main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InitAdvice.java b/...main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InitAdvice.java
@@ -0,0 +1,25 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
+
+import io.opentelemetry.instrumentation.api.servlet.MappingResolver;
+import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext;
+import javax.servlet.Servlet;
+import javax.servlet.ServletConfig;
+import net.bytebuddy.asm.Advice;
+
+public class Servlet3InitAdvice {
+
+  @Advice.OnMethodEnter(suppress = Throwable.class)
+  public static void servletInit(
+      @Advice.This Servlet servlet, @Advice.Argument(0) ServletConfig servletConfig) {
+    if (servletConfig == null) {
+      return;
+    }
+    InstrumentationContext.get(Servlet.class, MappingResolver.class)
+        .putIfAbsent(servlet, new Servlet3MappingResolverFactory(servletConfig));
+  }
+}
diff --git a/...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java b/...o/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java
@@ -26,7 +26,11 @@ 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(".Servlet3InitAdvice"),
+            adviceClassName(".Servlet3FilterInitAdvice")));
   }
 
   private static String adviceClassName(String suffix) {

diff --git a/.../opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3MappingResolverFactory.java b/.../opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3MappingResolverFactory.java
@@ -0,0 +1,37 @@
+/*
+ * Copyright The OpenTelemetry Authors
+ * SPDX-License-Identifier: Apache-2.0
+ */
+
+package io.opentelemetry.javaagent.instrumentation.servlet.v3_0;
+
+import io.opentelemetry.instrumentation.api.servlet.MappingResolver;
+import io.opentelemetry.instrumentation.servlet.naming.ServletMappingResolverFactory;
+import io.opentelemetry.javaagent.instrumentation.api.ContextStore;
+import java.util.Collection;
+import javax.servlet.ServletConfig;
+import javax.servlet.ServletContext;
+import javax.servlet.ServletRegistration;
+
+public class Servlet3MappingResolverFactory extends ServletMappingResolverFactory
+    implements ContextStore.Factory<MappingResolver> {
+  private final ServletConfig servletConfig;
+
+  public Servlet3MappingResolverFactory(ServletConfig servletConfig) {
+    this.servletConfig = servletConfig;
+  }
+
+  public Collection<String> getMappings() {
+    String servletName = servletConfig.getServletName();
+    ServletContext servletContext = servletConfig.getServletContext();
+    if (servletName == null || servletContext == null) {
+      return null;
+    }
+
+    ServletRegistration servletRegistration = servletContext.getServletRegistration(servletName);
+    if (servletRegistration == null) {
+      return null;
+    }
+    return servletRegistration.getMappings();
+  }
+}
diff --git a/.../groovy/AbstractServletMappingTest.groovy → ...groovy/AbstractServlet3MappingTest.groovy b/.../groovy/AbstractServletMappingTest.groovy → ...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)
 

diff --git a/...est/groovy/JettyServletMappingTest.groovy → ...st/groovy/JettyServlet3MappingTest.groovy b/...est/groovy/JettyServletMappingTest.groovy → ...st/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) {