diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/MappingResolver.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/MappingResolver.java similarity index 94% rename from instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/MappingResolver.java rename to instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/MappingResolver.java index 1ddc071feaad..e771ce7d682f 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/MappingResolver.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/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 exactMatches; private final List wildcardMatchers; private final boolean hasDefault; diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java index cba442923d52..d3e3c0724322 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/ServerSpanNaming.java +++ b/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 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/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java index 50d46713accc..eb60c83ce65e 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3Advice.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/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,7 +81,7 @@ 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(); @@ -73,7 +89,7 @@ public static void onEnter( return; } - context = tracer().startSpan(servletOrFilter, httpServletRequest); + context = tracer().startSpan(httpServletRequest, mappingResolver, servlet); scope = context.makeCurrent(); } diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterInitAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterInitAdvice.java new file mode 100644 index 000000000000..fba39cad4802 --- /dev/null +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/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/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterMappingResolverFactory.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3FilterMappingResolverFactory.java new file mode 100644 index 000000000000..37e4a4d9454a --- /dev/null +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/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 + implements ContextStore.Factory { + 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 getUrlPatternMappings(FilterRegistration filterRegistration) { + return filterRegistration.getUrlPatternMappings(); + } + + @Override + protected Collection getServletNameMappings(FilterRegistration filterRegistration) { + return filterRegistration.getServletNameMappings(); + } + + @Override + protected Collection getServletMappings(String servletName) { + ServletRegistration servletRegistration = + filterConfig.getServletContext().getServletRegistration(servletName); + if (servletRegistration == null) { + return null; + } + return servletRegistration.getMappings(); + } +} diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InitAdvice.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InitAdvice.java new file mode 100644 index 000000000000..43dbec45717f --- /dev/null +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/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/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java index a9e01ca855fd..4fc8bfebb65b 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3InstrumentationModule.java @@ -26,7 +26,11 @@ public Servlet3InstrumentationModule() { public List 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/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3MappingResolverFactory.java b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v3_0/Servlet3MappingResolverFactory.java new file mode 100644 index 000000000000..49e3aa62ad80 --- /dev/null +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/main/java/io/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 { + private final ServletConfig servletConfig; + + public Servlet3MappingResolverFactory(ServletConfig servletConfig) { + this.servletConfig = servletConfig; + } + + public Collection 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/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServletMappingTest.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3MappingTest.groovy similarity index 93% rename from instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServletMappingTest.groovy rename to instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3MappingTest.groovy index c3300caebf68..2ad93cefb30d 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServletMappingTest.groovy +++ b/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 extends AgentInstrumentationSpecification implements HttpServerTestTrait { +abstract class AbstractServlet3MappingTest extends AgentInstrumentationSpecification implements HttpServerTestTrait { abstract void addServlet(CONTEXT context, String path, Class servlet) diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServletMappingTest.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3MappingTest.groovy similarity index 93% rename from instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServletMappingTest.groovy rename to instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3MappingTest.groovy index aa153893cba3..994621eb1d48 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServletMappingTest.groovy +++ b/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 { +class JettyServlet3MappingTest extends AbstractServlet3MappingTest { @Override Server startServer(int port) { diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3FilterMappingTest.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3FilterMappingTest.groovy new file mode 100644 index 000000000000..dbbdb6715501 --- /dev/null +++ b/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) { + 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) { + 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", new DefaultServlet()) + context.addServletMappingDecoded("/prefix/*", "prefix-servlet") + Tomcat.addServlet(context, "suffix-servlet", new DefaultServlet()) + 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) + } +} diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServletMappingTest.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3MappingTest.groovy similarity index 95% rename from instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServletMappingTest.groovy rename to instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3MappingTest.groovy index c99c3809d792..4e05b2f77e02 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServletMappingTest.groovy +++ b/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 { +class TomcatServlet3MappingTest extends AbstractServlet3MappingTest { @Override Tomcat startServer(int port) { diff --git a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java index 08a44a535c10..94d79d2ca354 100644 --- a/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java +++ b/instrumentation/servlet/servlet-3.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/v3_0/Servlet3HttpServerTracer.java @@ -5,22 +5,21 @@ package io.opentelemetry.instrumentation.servlet.v3_0; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.FILTER; import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; import io.opentelemetry.context.Context; +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; -import io.opentelemetry.instrumentation.servlet.MappingResolver; import io.opentelemetry.instrumentation.servlet.javax.JavaxServletHttpServerTracer; -import java.util.Collection; -import javax.servlet.Servlet; -import javax.servlet.ServletConfig; -import javax.servlet.ServletContext; -import javax.servlet.ServletRegistration; +import io.opentelemetry.instrumentation.servlet.naming.ServletSpanNameProvider; import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; public class Servlet3HttpServerTracer extends JavaxServletHttpServerTracer { private static final Servlet3HttpServerTracer TRACER = new Servlet3HttpServerTracer(); + private static final ServletSpanNameProvider SPAN_NAME_PROVIDER = + new ServletSpanNameProvider<>(Servlet3Accessor.INSTANCE); protected Servlet3HttpServerTracer() { super(Servlet3Accessor.INSTANCE); @@ -30,89 +29,20 @@ public static Servlet3HttpServerTracer tracer() { return TRACER; } - public Context startSpan(Object servletOrFilter, HttpServletRequest request) { - return startSpan( - request, getSpanName(servletOrFilter, request), servletOrFilter instanceof Servlet); - } - - private String getSpanName(Object servletOrFilter, HttpServletRequest request) { - String spanName = getSpanNameFromPath(servletOrFilter, request); - if (spanName == null) { - String contextPath = request.getContextPath(); - if (contextPath == null || contextPath.isEmpty() || contextPath.equals("/")) { - return "HTTP " + request.getMethod(); - } - return contextPath; - } - return spanName; - } - - private static String getSpanNameFromPath(Object servletOrFilter, HttpServletRequest request) { - // we are only interested in Servlets - if (!(servletOrFilter instanceof Servlet)) { - return null; - } - Servlet servlet = (Servlet) servletOrFilter; - - String mapping = getMapping(servlet, request.getServletPath(), request.getPathInfo()); - // mapping was not found - if (mapping == null) { - return null; - } - - // prepend context path - String contextPath = request.getContextPath(); - if (contextPath == null || contextPath.isEmpty() || contextPath.equals("/")) { - return mapping; - } - return contextPath + mapping; - } - - private static String getMapping(Servlet servlet, String servletPath, String pathInfo) { - ServletConfig servletConfig = servlet.getServletConfig(); - if (servletConfig == null) { - return null; - } - String servletName = servletConfig.getServletName(); - ServletContext servletContext = servletConfig.getServletContext(); - MappingResolver mappingResolver = getMappingResolver(servletContext, servletName); - if (mappingResolver == null) { - return null; - } - - return mappingResolver.resolve(servletPath, pathInfo); - } - - private static MappingResolver getMappingResolver( - ServletContext servletContext, String servletName) { - if (servletContext == null || servletName == null) { - return null; - } - String key = MappingResolver.class.getName() + "." + servletName; - MappingResolver mappingResolver = (MappingResolver) servletContext.getAttribute(key); - if (mappingResolver != null) { - return mappingResolver; - } - - ServletRegistration servletRegistration = servletContext.getServletRegistration(servletName); - if (servletRegistration == null) { - return null; - } - Collection mappings = servletRegistration.getMappings(); - if (mappings == null) { - return null; - } - - mappingResolver = MappingResolver.build(mappings); - servletContext.setAttribute(key, mappingResolver); - - return mappingResolver; + public Context startSpan( + HttpServletRequest request, MappingResolver mappingResolver, boolean servlet) { + return startSpan(request, SPAN_NAME_PROVIDER.getSpanName(mappingResolver, request), servlet); } public Context updateContext( - Context context, Object servletOrFilter, HttpServletRequest request) { + Context context, + HttpServletRequest request, + MappingResolver mappingResolver, + boolean servlet) { ServerSpanNaming.updateServerSpanName( - context, SERVLET, () -> getSpanNameFromPath(servletOrFilter, request)); + context, + servlet ? SERVLET : FILTER, + () -> SPAN_NAME_PROVIDER.getSpanNameOrNull(mappingResolver, request)); return updateContext(context, request); } diff --git a/instrumentation/servlet/servlet-5.0/javaagent/servlet-5.0-javaagent.gradle b/instrumentation/servlet/servlet-5.0/javaagent/servlet-5.0-javaagent.gradle index e7c2ee8c1b5e..7d0f5bf98e49 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/servlet-5.0-javaagent.gradle +++ b/instrumentation/servlet/servlet-5.0/javaagent/servlet-5.0-javaagent.gradle @@ -13,4 +13,9 @@ dependencies { api(project(':instrumentation:servlet:servlet-5.0:library')) implementation(project(':instrumentation:servlet:servlet-common:javaagent')) compileOnly group: 'jakarta.servlet', name: 'jakarta.servlet-api', version: '5.0.0' + + testLibrary group: 'org.eclipse.jetty', name: 'jetty-server', version: '11.0.0' + testLibrary group: 'org.eclipse.jetty', name: 'jetty-servlet', version: '11.0.0' + testLibrary group: 'org.apache.tomcat.embed', name: 'tomcat-embed-core', version: '10.0.0' + testLibrary group: 'org.apache.tomcat.embed', name: 'tomcat-embed-jasper', version: '10.0.0' } diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java index b5593cb79e0e..eaccc959dd3e 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java @@ -29,7 +29,10 @@ public List typeInstrumentations() { new AsyncContextInstrumentation( BASE_PACKAGE, adviceClassName(".async.AsyncDispatchAdvice")), new ServletAndFilterInstrumentation( - BASE_PACKAGE, adviceClassName(".service.JakartaServletServiceAdvice")), + BASE_PACKAGE, + adviceClassName(".service.JakartaServletServiceAdvice"), + adviceClassName(".service.JakartaServletInitAdvice"), + adviceClassName(".service.JakartaServletFilterInitAdvice")), new HttpServletResponseInstrumentation( BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")), new RequestDispatcherInstrumentation( diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletFilterInitAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletFilterInitAdvice.java new file mode 100644 index 000000000000..84a77a485e44 --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletFilterInitAdvice.java @@ -0,0 +1,25 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.service; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext; +import jakarta.servlet.Filter; +import jakarta.servlet.FilterConfig; +import net.bytebuddy.asm.Advice; + +public class JakartaServletFilterInitAdvice { + + @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 JakartaServletFilterMappingResolverFactory(filterConfig)); + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletFilterMappingResolverFactory.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletFilterMappingResolverFactory.java new file mode 100644 index 000000000000..a5dfffe9780d --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletFilterMappingResolverFactory.java @@ -0,0 +1,55 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.service; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import io.opentelemetry.instrumentation.servlet.naming.ServletFilterMappingResolverFactory; +import io.opentelemetry.javaagent.instrumentation.api.ContextStore; +import jakarta.servlet.FilterConfig; +import jakarta.servlet.FilterRegistration; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletRegistration; +import java.util.Collection; + +public class JakartaServletFilterMappingResolverFactory + extends ServletFilterMappingResolverFactory + implements ContextStore.Factory { + private final FilterConfig filterConfig; + + public JakartaServletFilterMappingResolverFactory(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 getUrlPatternMappings(FilterRegistration filterRegistration) { + return filterRegistration.getUrlPatternMappings(); + } + + @Override + protected Collection getServletNameMappings(FilterRegistration filterRegistration) { + return filterRegistration.getServletNameMappings(); + } + + @Override + protected Collection getServletMappings(String servletName) { + ServletRegistration servletRegistration = + filterConfig.getServletContext().getServletRegistration(servletName); + if (servletRegistration == null) { + return null; + } + return servletRegistration.getMappings(); + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletInitAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletInitAdvice.java new file mode 100644 index 000000000000..ff9a27770c4c --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletInitAdvice.java @@ -0,0 +1,25 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.service; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import io.opentelemetry.javaagent.instrumentation.api.InstrumentationContext; +import jakarta.servlet.Servlet; +import jakarta.servlet.ServletConfig; +import net.bytebuddy.asm.Advice; + +public class JakartaServletInitAdvice { + + @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 JakartaServletMappingResolverFactory(servletConfig)); + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletMappingResolverFactory.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletMappingResolverFactory.java new file mode 100644 index 000000000000..d8b7bbdaac87 --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletMappingResolverFactory.java @@ -0,0 +1,37 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.service; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import io.opentelemetry.instrumentation.servlet.naming.ServletMappingResolverFactory; +import io.opentelemetry.javaagent.instrumentation.api.ContextStore; +import jakarta.servlet.ServletConfig; +import jakarta.servlet.ServletContext; +import jakarta.servlet.ServletRegistration; +import java.util.Collection; + +public class JakartaServletMappingResolverFactory extends ServletMappingResolverFactory + implements ContextStore.Factory { + private final ServletConfig servletConfig; + + public JakartaServletMappingResolverFactory(ServletConfig servletConfig) { + this.servletConfig = servletConfig; + } + + public Collection 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/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java index e27848c1e173..9f05f7901e5f 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/service/JakartaServletServiceAdvice.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 jakarta.servlet.Filter; +import jakarta.servlet.Servlet; import jakarta.servlet.ServletRequest; import jakarta.servlet.ServletResponse; import jakarta.servlet.http.HttpServletRequest; @@ -37,12 +41,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; } @@ -51,7 +67,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(); @@ -66,7 +82,7 @@ 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(); @@ -74,7 +90,7 @@ public static void onEnter( return; } - context = tracer().startSpan(servletOrFilter, httpServletRequest); + context = tracer().startSpan(httpServletRequest, mappingResolver, servlet); scope = context.makeCurrent(); } diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5MappingTest.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5MappingTest.groovy new file mode 100644 index 000000000000..6c1edf8013dd --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5MappingTest.groovy @@ -0,0 +1,82 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import static io.opentelemetry.api.trace.StatusCode.ERROR + +import io.opentelemetry.api.trace.SpanKind +import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification +import io.opentelemetry.instrumentation.test.base.HttpServerTestTrait +import jakarta.servlet.Servlet +import jakarta.servlet.ServletException +import jakarta.servlet.http.HttpServlet +import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletResponse +import okhttp3.HttpUrl +import okhttp3.Request +import okhttp3.RequestBody +import okhttp3.Response +import spock.lang.Unroll + +abstract class AbstractServlet5MappingTest extends AgentInstrumentationSpecification implements HttpServerTestTrait { + + abstract void addServlet(CONTEXT context, String path, Class servlet) + + protected void setupServlets(CONTEXT context) { + addServlet(context, "/prefix/*", TestServlet) + addServlet(context, "*.suffix", TestServlet) + } + + static class TestServlet extends HttpServlet { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + response.getWriter().write("Ok") + } + } + + Request.Builder request(HttpUrl url, String method, RequestBody body) { + return new Request.Builder() + .url(url) + .method(method, body) + } + + @Unroll + def "test path #path"() { + setup: + def url = HttpUrl.get(address.resolve(path)).newBuilder().build() + def request = request(url, "GET", null).build() + Response response = client.newCall(request).execute() + + expect: + response.code() == success ? 200 : 404 + + and: + def spanCount = success ? 1 : 2 + assertTraces(1) { + trace(0, spanCount) { + span(0) { + name getContextPath() + spanName + kind SpanKind.SERVER + if (!success) { + status ERROR + } + } + if (!success) { + span(1) { + } + } + } + } + + where: + path | spanName | success + 'prefix' | '/prefix/*' | true + 'prefix/' | '/prefix/*' | true + 'prefix/a' | '/prefix/*' | true + 'prefixa' | '/*' | false + 'a.suffix' | '/*.suffix' | true + '.suffix' | '/*.suffix' | true + 'suffix' | '/*' | false + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5MappingTest.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5MappingTest.groovy new file mode 100644 index 000000000000..bac23eae685a --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/JettyServlet5MappingTest.groovy @@ -0,0 +1,60 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import jakarta.servlet.Servlet +import jakarta.servlet.ServletException +import jakarta.servlet.http.HttpServlet +import jakarta.servlet.http.HttpServletRequest +import jakarta.servlet.http.HttpServletResponse +import org.eclipse.jetty.server.Server +import org.eclipse.jetty.servlet.ServletContextHandler +import spock.lang.IgnoreIf + +@IgnoreIf({ !jvm.java11Compatible }) +class JettyServlet5MappingTest extends AbstractServlet5MappingTest { + + @Override + Object startServer(int port) { + Server server = new Server(port) + ServletContextHandler handler = new ServletContextHandler(null, contextPath) + setupServlets(handler) + server.setHandler(handler) + server.start() + return server + } + + @Override + void stopServer(Object serverObject) { + Server server = (Server) serverObject + server.stop() + server.destroy() + } + + @Override + protected void setupServlets(Object handlerObject) { + ServletContextHandler handler = (ServletContextHandler) handlerObject + super.setupServlets(handler) + + addServlet(handler, "/", DefaultServlet) + } + + @Override + void addServlet(Object handlerObject, String path, Class servlet) { + ServletContextHandler handler = (ServletContextHandler) handlerObject + handler.addServlet(servlet, path) + } + + static class DefaultServlet extends HttpServlet { + @Override + protected void doGet(HttpServletRequest request, HttpServletResponse response) throws ServletException, IOException { + response.sendError(404) + } + } + + @Override + String getContextPath() { + "/jetty-context" + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5FilterMappingTest.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5FilterMappingTest.groovy new file mode 100644 index 000000000000..71278c52f270 --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5FilterMappingTest.groovy @@ -0,0 +1,135 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import jakarta.servlet.Filter +import jakarta.servlet.FilterChain +import jakarta.servlet.FilterConfig +import jakarta.servlet.ServletException +import jakarta.servlet.ServletRequest +import jakarta.servlet.ServletResponse +import jakarta.servlet.http.HttpServlet +import jakarta.servlet.http.HttpServletRequest +import jakarta.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 TomcatServlet5FilterMappingTest extends TomcatServlet5MappingTest { + + void addFilter(Context servletContext, String path, Class 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) { + 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 TomcatServlet5FilterUrlPatternMappingTest extends TomcatServlet5FilterMappingTest { + @Override + protected void setupServlets(Context context) { + addFilter(context, "/*", FirstFilter) + addFilter(context, "/prefix/*", TestFilter) + addFilter(context, "*.suffix", TestFilter) + addFilter(context, "/*", LastFilter) + } +} + +class TomcatServlet5FilterServletNameMappingTest extends TomcatServlet5FilterMappingTest { + @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) + } +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5MappingTest.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5MappingTest.groovy new file mode 100644 index 000000000000..f328855ebdcb --- /dev/null +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5MappingTest.groovy @@ -0,0 +1,64 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +import jakarta.servlet.Servlet +import java.nio.file.Files +import org.apache.catalina.Context +import org.apache.catalina.startup.Tomcat +import org.apache.tomcat.JarScanFilter +import org.apache.tomcat.JarScanType + +class TomcatServlet5MappingTest extends AbstractServlet5MappingTest { + + @Override + Tomcat startServer(int port) { + def tomcatServer = new Tomcat() + + def baseDir = Files.createTempDirectory("tomcat").toFile() + baseDir.deleteOnExit() + tomcatServer.setBaseDir(baseDir.getAbsolutePath()) + + tomcatServer.setPort(port) + tomcatServer.getConnector().enableLookups = true // get localhost instead of 127.0.0.1 + + File applicationDir = new File(baseDir, "/webapps/ROOT") + if (!applicationDir.exists()) { + applicationDir.mkdirs() + applicationDir.deleteOnExit() + } + Context servletContext = tomcatServer.addWebapp(contextPath, applicationDir.getAbsolutePath()) + // Speed up startup by disabling jar scanning: + servletContext.getJarScanner().setJarScanFilter(new JarScanFilter() { + @Override + boolean check(JarScanType jarScanType, String jarName) { + return false + } + }) + + setupServlets(servletContext) + + tomcatServer.start() + + return tomcatServer + } + + @Override + void stopServer(Tomcat server) { + server.stop() + server.destroy() + } + + @Override + void addServlet(Context servletContext, String path, Class servlet) { + String name = UUID.randomUUID() + Tomcat.addServlet(servletContext, name, servlet.newInstance()) + servletContext.addServletMappingDecoded(path, name) + } + + @Override + String getContextPath() { + return "/tomcat-context" + } +} diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java index bb4dd619ab4c..99c03fd4ea23 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletAccessor.java @@ -85,6 +85,11 @@ public String getRequestServletPath(HttpServletRequest request) { return request.getServletPath(); } + @Override + public String getRequestPathInfo(HttpServletRequest request) { + return request.getPathInfo(); + } + @Override public Principal getRequestUserPrincipal(HttpServletRequest request) { return request.getUserPrincipal(); diff --git a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java index d4bca7c71ffe..a5b3ba467260 100644 --- a/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-5.0/library/src/main/java/io/opentelemetry/instrumentation/servlet/jakarta/v5_0/JakartaServletHttpServerTracer.java @@ -5,25 +5,24 @@ package io.opentelemetry.instrumentation.servlet.jakarta.v5_0; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.FILTER; import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; import io.opentelemetry.context.Context; import io.opentelemetry.context.propagation.TextMapGetter; +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; -import io.opentelemetry.instrumentation.servlet.MappingResolver; import io.opentelemetry.instrumentation.servlet.ServletHttpServerTracer; +import io.opentelemetry.instrumentation.servlet.naming.ServletSpanNameProvider; import jakarta.servlet.RequestDispatcher; -import jakarta.servlet.Servlet; -import jakarta.servlet.ServletConfig; -import jakarta.servlet.ServletContext; -import jakarta.servlet.ServletRegistration; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import java.util.Collection; public class JakartaServletHttpServerTracer extends ServletHttpServerTracer { private static final JakartaServletHttpServerTracer TRACER = new JakartaServletHttpServerTracer(); + private static final ServletSpanNameProvider SPAN_NAME_PROVIDER = + new ServletSpanNameProvider<>(JakartaServletAccessor.INSTANCE); public JakartaServletHttpServerTracer() { super(JakartaServletAccessor.INSTANCE); @@ -33,89 +32,20 @@ public static JakartaServletHttpServerTracer tracer() { return TRACER; } - public Context startSpan(Object servletOrFilter, HttpServletRequest request) { - return startSpan( - request, getSpanName(servletOrFilter, request), servletOrFilter instanceof Servlet); - } - - private String getSpanName(Object servletOrFilter, HttpServletRequest request) { - String spanName = getSpanNameFromPath(servletOrFilter, request); - if (spanName == null) { - String contextPath = request.getContextPath(); - if (contextPath == null || contextPath.isEmpty() || contextPath.equals("/")) { - return "HTTP " + request.getMethod(); - } - return contextPath; - } - return spanName; - } - - private static String getSpanNameFromPath(Object servletOrFilter, HttpServletRequest request) { - // we are only interested in Servlets - if (!(servletOrFilter instanceof Servlet)) { - return null; - } - Servlet servlet = (Servlet) servletOrFilter; - - String mapping = getMapping(servlet, request.getServletPath(), request.getPathInfo()); - // mapping was not found - if (mapping == null) { - return null; - } - - // prepend context path - String contextPath = request.getContextPath(); - if (contextPath == null || contextPath.isEmpty() || contextPath.equals("/")) { - return mapping; - } - return contextPath + mapping; - } - - private static String getMapping(Servlet servlet, String servletPath, String pathInfo) { - ServletConfig servletConfig = servlet.getServletConfig(); - if (servletConfig == null) { - return null; - } - String servletName = servletConfig.getServletName(); - ServletContext servletContext = servletConfig.getServletContext(); - MappingResolver mappingResolver = getMappingResolver(servletContext, servletName); - if (mappingResolver == null) { - return null; - } - - return mappingResolver.resolve(servletPath, pathInfo); - } - - private static MappingResolver getMappingResolver( - ServletContext servletContext, String servletName) { - if (servletContext == null || servletName == null) { - return null; - } - String key = MappingResolver.class.getName() + "." + servletName; - MappingResolver mappingResolver = (MappingResolver) servletContext.getAttribute(key); - if (mappingResolver != null) { - return mappingResolver; - } - - ServletRegistration servletRegistration = servletContext.getServletRegistration(servletName); - if (servletRegistration == null) { - return null; - } - Collection mappings = servletRegistration.getMappings(); - if (mappings == null) { - return null; - } - - mappingResolver = MappingResolver.build(mappings); - servletContext.setAttribute(key, mappingResolver); - - return mappingResolver; + public Context startSpan( + HttpServletRequest request, MappingResolver mappingResolver, boolean servlet) { + return startSpan(request, SPAN_NAME_PROVIDER.getSpanName(mappingResolver, request), servlet); } public Context updateContext( - Context context, Object servletOrFilter, HttpServletRequest request) { + Context context, + HttpServletRequest request, + MappingResolver mappingResolver, + boolean servlet) { ServerSpanNaming.updateServerSpanName( - context, SERVLET, () -> getSpanNameFromPath(servletOrFilter, request)); + context, + servlet ? SERVLET : FILTER, + () -> SPAN_NAME_PROVIDER.getSpanNameOrNull(mappingResolver, request)); return updateContext(context, request); } diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterInstrumentation.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterInstrumentation.java index 431629b368fd..9fe41ff211b6 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterInstrumentation.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/service/ServletAndFilterInstrumentation.java @@ -8,12 +8,12 @@ import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.AgentElementMatchers.safeHasSuperType; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.ClassLoaderMatcher.hasClassesNamed; import static io.opentelemetry.javaagent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf; -import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isPublic; import static net.bytebuddy.matcher.ElementMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import io.opentelemetry.javaagent.tooling.TypeInstrumentation; +import java.util.HashMap; import java.util.Map; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -22,10 +22,22 @@ public class ServletAndFilterInstrumentation implements TypeInstrumentation { private final String basePackageName; private final String adviceClassName; + private final String servletInitAdviceClassName; + private final String filterInitAdviceClassName; - public ServletAndFilterInstrumentation(String basePackageName, String adviceClassName) { + public ServletAndFilterInstrumentation( + String basePackageName, + String adviceClassName, + String servletInitAdviceClassName, + String filterInitAdviceClassName) { this.basePackageName = basePackageName; this.adviceClassName = adviceClassName; + this.servletInitAdviceClassName = servletInitAdviceClassName; + this.filterInitAdviceClassName = filterInitAdviceClassName; + } + + public ServletAndFilterInstrumentation(String basePackageName, String adviceClassName) { + this(basePackageName, adviceClassName, null, null); } @Override @@ -40,11 +52,23 @@ public ElementMatcher typeMatcher() { @Override public Map, String> transformers() { - return singletonMap( + Map, String> transformers = new HashMap<>(); + transformers.put( namedOneOf("doFilter", "service") .and(takesArgument(0, named(basePackageName + ".ServletRequest"))) .and(takesArgument(1, named(basePackageName + ".ServletResponse"))) .and(isPublic()), adviceClassName); + if (servletInitAdviceClassName != null) { + transformers.put( + named("init").and(takesArgument(0, named(basePackageName + ".ServletConfig"))), + servletInitAdviceClassName); + } + if (filterInitAdviceClassName != null) { + transformers.put( + named("init").and(takesArgument(0, named(basePackageName + ".FilterConfig"))), + filterInitAdviceClassName); + } + return transformers; } } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java index 1c50ba4b349b..49ec785a0298 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletAccessor.java @@ -8,10 +8,11 @@ import java.security.Principal; /** - * This interface is used to access methods of HttpServletRequest and HttpServletResponse classes in - * shared code that is used for both jakarta.servlet and javax.servlet versions of those classes. A - * wrapper class with extra information attached may be used as well in cases where the class itself - * does not provide some field (such as response status for Servlet API 2.2). + * This interface is used to access methods of ServletContext, HttpServletRequest and + * HttpServletResponse classes in shared code that is used for both jakarta.servlet and + * javax.servlet versions of those classes. A wrapper class with extra information attached may be + * used as well in cases where the class itself does not provide some field (such as response status + * for Servlet API 2.2). * * @param HttpServletRequest class (or a wrapper) * @param HttpServletResponse class (or a wrapper) @@ -43,6 +44,8 @@ public interface ServletAccessor { String getRequestServletPath(REQUEST request); + String getRequestPathInfo(REQUEST request); + Principal getRequestUserPrincipal(REQUEST request); Integer getRequestRemotePort(REQUEST request); diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java index d3b783268722..4376f6eebb6d 100644 --- a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/ServletHttpServerTracer.java @@ -6,6 +6,7 @@ package io.opentelemetry.instrumentation.servlet; import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.CONTAINER; +import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.FILTER; import static io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming.Source.SERVLET; import io.opentelemetry.api.trace.Span; @@ -48,11 +49,10 @@ public Context startSpan(REQUEST request, String spanName, boolean servlet) { accessor.setRequestAttribute(request, "trace_id", spanContext.getTraceId()); accessor.setRequestAttribute(request, "span_id", spanContext.getSpanId()); - if (servlet) { - // server span name shouldn't be updated when server span was created from a call to Servlet - // (if created from a call to Filter then name may be updated from updateContext) - ServerSpanNaming.updateSource(context, SERVLET); - } + // server span name shouldn't be updated when server span was created from a call to Servlet + // (if created from a call to Filter then name may be updated from updateContext) + ServerSpanNaming.updateSource(context, servlet ? SERVLET : FILTER); + return addServletContextPath(context, request); } diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletFilterMappingResolverFactory.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletFilterMappingResolverFactory.java new file mode 100644 index 000000000000..60bc9c46ee9d --- /dev/null +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletFilterMappingResolverFactory.java @@ -0,0 +1,67 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.servlet.naming; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import java.util.ArrayList; +import java.util.Collection; +import java.util.Collections; +import java.util.HashSet; +import java.util.List; +import java.util.Set; + +public abstract class ServletFilterMappingResolverFactory { + + protected abstract FILTERREGISTRATION getFilterRegistration(); + + protected abstract Collection getUrlPatternMappings( + FILTERREGISTRATION filterRegistration); + + protected abstract Collection getServletNameMappings( + FILTERREGISTRATION filterRegistration); + + protected abstract Collection getServletMappings(String servletName); + + private Collection getMappings() { + FILTERREGISTRATION filterRegistration = getFilterRegistration(); + if (filterRegistration == null) { + return null; + } + Set mappings = new HashSet<>(); + Collection urlPatternMappings = getUrlPatternMappings(filterRegistration); + if (urlPatternMappings != null) { + mappings.addAll(urlPatternMappings); + } + Collection servletNameMappings = getServletNameMappings(filterRegistration); + if (servletNameMappings != null) { + for (String servletName : servletNameMappings) { + Collection servletMappings = getServletMappings(servletName); + if (servletMappings != null) { + mappings.addAll(servletMappings); + } + } + } + + if (mappings.isEmpty()) { + return null; + } + + List mappingsList = new ArrayList<>(mappings); + // sort longest mapping first + Collections.sort(mappingsList, (s1, s2) -> s2.length() - s1.length()); + + return mappingsList; + } + + public final MappingResolver create() { + Collection mappings = getMappings(); + if (mappings == null) { + return null; + } + + return MappingResolver.build(mappings); + } +} diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletMappingResolverFactory.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletMappingResolverFactory.java new file mode 100644 index 000000000000..ea051b876fa3 --- /dev/null +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletMappingResolverFactory.java @@ -0,0 +1,23 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.servlet.naming; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import java.util.Collection; + +public abstract class ServletMappingResolverFactory { + + protected abstract Collection getMappings(); + + public final MappingResolver create() { + Collection mappings = getMappings(); + if (mappings == null) { + return null; + } + + return MappingResolver.build(mappings); + } +} diff --git a/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletSpanNameProvider.java b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletSpanNameProvider.java new file mode 100644 index 000000000000..32ba2e17c7e5 --- /dev/null +++ b/instrumentation/servlet/servlet-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/naming/ServletSpanNameProvider.java @@ -0,0 +1,51 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.instrumentation.servlet.naming; + +import io.opentelemetry.instrumentation.api.servlet.MappingResolver; +import io.opentelemetry.instrumentation.servlet.ServletAccessor; + +/** Helper class for constructing span name for given servlet/filter mapping and request. */ +public class ServletSpanNameProvider { + private final ServletAccessor servletAccessor; + + public ServletSpanNameProvider(ServletAccessor servletAccessor) { + this.servletAccessor = servletAccessor; + } + + public String getSpanName(MappingResolver mappingResolver, REQUEST request) { + String spanName = getSpanNameOrNull(mappingResolver, request); + if (spanName == null) { + String contextPath = servletAccessor.getRequestContextPath(request); + if (contextPath == null || contextPath.isEmpty() || contextPath.equals("/")) { + return "HTTP " + servletAccessor.getRequestMethod(request); + } + return contextPath + "/*"; + } + return spanName; + } + + public String getSpanNameOrNull(MappingResolver mappingResolver, REQUEST request) { + if (mappingResolver == null) { + return null; + } + + String servletPath = servletAccessor.getRequestServletPath(request); + String pathInfo = servletAccessor.getRequestPathInfo(request); + String mapping = mappingResolver.resolve(servletPath, pathInfo); + // mapping was not found + if (mapping == null) { + return null; + } + + // prepend context path + String contextPath = servletAccessor.getRequestContextPath(request); + if (contextPath == null || contextPath.isEmpty() || contextPath.equals("/")) { + return mapping; + } + return contextPath + mapping; + } +} diff --git a/instrumentation/servlet/servlet-javax-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/javax/JavaxServletAccessor.java b/instrumentation/servlet/servlet-javax-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/javax/JavaxServletAccessor.java index c90a2761e21f..f1310f9a3522 100644 --- a/instrumentation/servlet/servlet-javax-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/javax/JavaxServletAccessor.java +++ b/instrumentation/servlet/servlet-javax-common/library/src/main/java/io/opentelemetry/instrumentation/servlet/javax/JavaxServletAccessor.java @@ -76,6 +76,11 @@ public String getRequestServletPath(HttpServletRequest request) { return request.getServletPath(); } + @Override + public String getRequestPathInfo(HttpServletRequest request) { + return request.getPathInfo(); + } + @Override public Principal getRequestUserPrincipal(HttpServletRequest request) { return request.getUserPrincipal(); diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy index e98f236d9adf..f5557881a2dc 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/test/groovy/test/boot/SpringBootBasedTest.groovy @@ -87,7 +87,7 @@ class SpringBootBasedTest extends HttpServerTest case NOT_FOUND: return getContextPath() + "/**" case LOGIN: - return "HTTP POST" + return getContextPath() + "/*" default: return super.expectedServerSpanName(endpoint) } diff --git a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy index 3ea5556c663e..9b5ddd6dc997 100644 --- a/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy +++ b/instrumentation/struts-2.3/javaagent/src/test/groovy/Struts2ActionSpanTest.groovy @@ -68,7 +68,7 @@ class Struts2ActionSpanTest extends HttpServerTest implements AgentTestT case PATH_PARAM: return getContextPath() + "/path/{id}/param" case NOT_FOUND: - return "HTTP GET" + return getContextPath() + "/*" default: return endpoint.resolvePath(address).path } diff --git a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertyServletOnlySmokeTest.groovy b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertyServletOnlySmokeTest.groovy index 8adf2daae1f4..402811a34cb7 100644 --- a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertyServletOnlySmokeTest.groovy +++ b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertyServletOnlySmokeTest.groovy @@ -16,4 +16,13 @@ class LibertyServletOnlySmokeTest extends LibertySmokeTest { return ["liberty-servlet.xml": "/config/server.xml"] } + @Override + protected String getSpanName(String path) { + switch (path) { + case "/app/hello.txt": + case "/app/file-that-does-not-exist": + return "HTTP GET" + } + return super.getSpanName(path) + } } diff --git a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertySmokeTest.groovy b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertySmokeTest.groovy index bd0c447eae77..1b27d17631d3 100644 --- a/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertySmokeTest.groovy +++ b/smoke-tests/src/test/groovy/io/opentelemetry/smoketest/LibertySmokeTest.groovy @@ -21,14 +21,4 @@ class LibertySmokeTest extends AppServerTest { protected TargetWaitStrategy getWaitStrategy() { return new TargetWaitStrategy.Log(Duration.ofMinutes(3), ".*server is ready to run a smarter planet.*") } - - @Override - protected String getSpanName(String path) { - switch (path) { - case "/app/hello.txt": - case "/app/file-that-does-not-exist": - return "HTTP GET" - } - return super.getSpanName(path) - } }