From 903926904d0979cc9213f1e616c92004144b7129 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 24 Nov 2021 19:20:50 +0200 Subject: [PATCH] Fix spring webmvc latest dep test (#4687) * Fix spring webmvc latest dep test * review comments * remove latest dep version restriction --- .../javaagent/build.gradle.kts | 4 - .../DispatcherServletInstrumentation.java | 3 +- .../OpenTelemetryHandlerMappingFilter.java | 129 ++++++++++++++---- 3 files changed, 102 insertions(+), 34 deletions(-) diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/build.gradle.kts b/instrumentation/spring/spring-webmvc-3.1/javaagent/build.gradle.kts index 422c06b617dc..a66d452187e0 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/build.gradle.kts +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/build.gradle.kts @@ -40,10 +40,6 @@ dependencies { testLibrary("org.springframework.boot:spring-boot-starter-web:1.5.17.RELEASE") testLibrary("org.springframework.boot:spring-boot-starter-security:1.5.17.RELEASE") - latestDepTestLibrary("org.springframework.boot:spring-boot-starter-test:2.5.+") - latestDepTestLibrary("org.springframework.boot:spring-boot-starter-web:2.5.+") - latestDepTestLibrary("org.springframework.boot:spring-boot-starter-security:2.5.+") - testImplementation("org.springframework.security.oauth:spring-security-oauth2:2.0.16.RELEASE") // For spring security diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/DispatcherServletInstrumentation.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/DispatcherServletInstrumentation.java index 112f5921441a..fec98e9861b8 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/DispatcherServletInstrumentation.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/springwebmvc/DispatcherServletInstrumentation.java @@ -52,8 +52,7 @@ public void transform(TypeTransformer transformer) { /** * This advice creates a filter that has reference to the handlerMappings from DispatcherServlet - * which allows the mappings to be evaluated at the beginning of the filter chain. This evaluation - * is done inside the Servlet3Decorator.onContext method. + * which allows the mappings to be evaluated outside of regular request processing. */ @SuppressWarnings("unused") public static class HandlerMappingAdvice { diff --git a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/org/springframework/web/servlet/OpenTelemetryHandlerMappingFilter.java b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/org/springframework/web/servlet/OpenTelemetryHandlerMappingFilter.java index 1c4f0cd286dc..e5eea9e108bb 100644 --- a/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/org/springframework/web/servlet/OpenTelemetryHandlerMappingFilter.java +++ b/instrumentation/spring/spring-webmvc-3.1/javaagent/src/main/java/org/springframework/web/servlet/OpenTelemetryHandlerMappingFilter.java @@ -12,6 +12,9 @@ import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.javaagent.instrumentation.springwebmvc.SpringWebMvcServerSpanNaming; import java.io.IOException; +import java.lang.invoke.MethodHandle; +import java.lang.invoke.MethodHandles; +import java.lang.invoke.MethodType; import java.util.ArrayList; import java.util.List; import java.util.Objects; @@ -28,18 +31,39 @@ import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping; public class OpenTelemetryHandlerMappingFilter implements Filter, Ordered { + private static final String PATH_ATTRIBUTE = getRequestPathAttribute(); + private static final MethodHandle usesPathPatternsMh = getUsesPathPatternsMh(); + private static final MethodHandle parseAndCacheMh = parseAndCacheMh(); private final ServerSpanNameSupplier serverSpanName = (context, request) -> { - if (findMapping(request)) { - // Name the parent span based on the matching pattern - // Let the parent span resource name be set with the attribute set in findMapping. - return SpringWebMvcServerSpanNaming.SERVER_SPAN_NAME.get(context, request); + Object previousValue = null; + if (this.parseRequestPath && PATH_ATTRIBUTE != null) { + previousValue = request.getAttribute(PATH_ATTRIBUTE); + // sets new value for PATH_ATTRIBUTE of request + parseAndCache(request); + } + try { + if (findMapping(request)) { + // Name the parent span based on the matching pattern + // Let the parent span resource name be set with the attribute set in findMapping. + return SpringWebMvcServerSpanNaming.SERVER_SPAN_NAME.get(context, request); + } + } finally { + // mimic spring DispatcherServlet and restore the previous value of PATH_ATTRIBUTE + if (this.parseRequestPath && PATH_ATTRIBUTE != null) { + if (previousValue == null) { + request.removeAttribute(PATH_ATTRIBUTE); + } else { + request.setAttribute(PATH_ATTRIBUTE, previousValue); + } + } } return null; }; - @Nullable private volatile List handlerMappings; + @Nullable private List handlerMappings; + private boolean parseRequestPath; @Override public void init(FilterConfig filterConfig) {} @@ -90,31 +114,16 @@ private boolean findMapping(HttpServletRequest request) { public void setHandlerMappings(List mappings) { List handlerMappings = new ArrayList<>(); for (HandlerMapping mapping : mappings) { - // it may be enticing to add all HandlerMapping classes here, but DO NOT - // - // because we call getHandler() on them above, at the very beginning of the request - // and this can be a very invasive call with application-crashing side-effects - // - // for example: org.grails.web.mapping.mvc.UrlMappingsHandlerMapping.getHandler() - // 1. uses GrailsWebRequest.lookup() to get GrailsWebRequest bound to thread local - // 2. and populates the servlet request attribute "org.grails.url.match.info" - // with GrailsControllerUrlMappingInfo - // - // which causes big problems if GrailsWebRequest thread local is leaked from prior request - // (which has been observed to happen in Grails 3.0.17 at least), because then our call to - // UrlMappingsHandlerMapping.getHandler() at the very beginning of the request: - // 1. GrailsWebRequest.lookup() gets the leaked GrailsWebRequest - // 2. servlet request attribute "org.grails.url.match.info" is populated based on this leaked - // GrailsWebRequest (so in other words, most likely the wrong route is matched) - // - // and then GrailsWebRequestFilter creates a new GrailsWebRequest and binds it to the thread - // - // and then the application calls UrlMappingsHandlerMapping.getHandler() to route the request - // but it finds servlet request attribute "org.grails.url.match.info" already populated (by - // above) and so it short cuts the matching process and uses the wrong route that the agent - // populated caused to be populated into the request attribute above + // Originally we ran findMapping at the very beginning of the request. This turned out to have + // application-crashing side-effects with grails. That is why we don't add all HandlerMapping + // classes here. Although now that we run findMapping after the request, and only when server + // span name has not been updated by a controller, the probability of bad side-effects is much + // reduced even if we did add all HandlerMapping classes here. if (mapping instanceof RequestMappingHandlerMapping) { handlerMappings.add(mapping); + if (usePathPatterns(mapping)) { + this.parseRequestPath = true; + } } } if (!handlerMappings.isEmpty()) { @@ -127,4 +136,68 @@ public int getOrder() { // Run after all HIGHEST_PRECEDENCE items return Ordered.HIGHEST_PRECEDENCE + 1; } + + private static MethodHandle getUsesPathPatternsMh() { + // Method added in spring 5.3 + try { + return MethodHandles.lookup() + .findVirtual( + HandlerMapping.class, "usesPathPatterns", MethodType.methodType(boolean.class)); + } catch (NoSuchMethodException | IllegalAccessException exception) { + return null; + } + } + + private static boolean usePathPatterns(HandlerMapping handlerMapping) { + if (usesPathPatternsMh == null) { + return false; + } + try { + return (boolean) usesPathPatternsMh.invoke(handlerMapping); + } catch (Throwable throwable) { + throw new IllegalStateException(throwable); + } + } + + private static MethodHandle parseAndCacheMh() { + // ServletRequestPathUtils added in spring 5.3 + try { + Class pathUtilsClass = + Class.forName("org.springframework.web.util.ServletRequestPathUtils"); + Class requestPathClass = Class.forName("org.springframework.http.server.RequestPath"); + return MethodHandles.lookup() + .findStatic( + pathUtilsClass, + "parseAndCache", + MethodType.methodType(requestPathClass, HttpServletRequest.class)); + } catch (ClassNotFoundException | NoSuchMethodException | IllegalAccessException exception) { + return null; + } + } + + private static void parseAndCache(HttpServletRequest request) { + if (parseAndCacheMh == null) { + return; + } + try { + parseAndCacheMh.invoke(request); + } catch (Throwable throwable) { + throw new IllegalStateException(throwable); + } + } + + private static String getRequestPathAttribute() { + try { + Class pathUtilsClass = + Class.forName("org.springframework.web.util.ServletRequestPathUtils"); + return (String) + MethodHandles.lookup() + .findStaticGetter(pathUtilsClass, "PATH_ATTRIBUTE", String.class) + .invoke(); + } catch (ClassNotFoundException | NoSuchFieldException | IllegalAccessException exception) { + return null; + } catch (Throwable throwable) { + throw new IllegalStateException(throwable); + } + } }