Wrap request to avoid modifying attributes of the original request (#10389)

Author: laurit

instrumentation/spring/spring-webmvc/spring-webmvc-3.1/javaagent/src/main/java/org/springframework/web/servlet/v3_1/OpenTelemetryHandlerMappingFilter.java

@@ -16,7 +16,9 @@
 import java.lang.invoke.MethodHandles;
 import java.lang.invoke.MethodType;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import javax.annotation.Nullable;
 import javax.servlet.Filter;
@@ -26,41 +28,29 @@
 import javax.servlet.ServletRequest;
 import javax.servlet.ServletResponse;
 import javax.servlet.http.HttpServletRequest;
+import javax.servlet.http.HttpServletRequestWrapper;
 import javax.servlet.http.HttpServletResponse;
 import org.springframework.core.Ordered;
 import org.springframework.web.servlet.HandlerExecutionChain;
 import org.springframework.web.servlet.HandlerMapping;
 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 HttpServerRouteGetter<HttpServletRequest> serverSpanName =
       (context, request) -> {
-        Object previousValue = null;
-        if (this.parseRequestPath && PATH_ATTRIBUTE != null) {
-          previousValue = request.getAttribute(PATH_ATTRIBUTE);
+        if (this.parseRequestPath) {
           // 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);
-            }
-          }
+        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);
         }
+
         return null;
       };
 
@@ -84,14 +74,39 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
     } finally {
       if (handlerMappings != null) {
         Context context = Context.current();
-        HttpServerRoute.update(context, CONTROLLER, serverSpanName, (HttpServletRequest) request);
+        HttpServerRoute.update(context, CONTROLLER, serverSpanName, prepareRequest(request));
       }
     }
   }
 
   @Override
   public void destroy() {}
 
+  private static HttpServletRequest prepareRequest(ServletRequest request) {
+    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/10379
+    // Finding the request handler modifies request attributes. We are wrapping the request to avoid
+    // this.
+    return new HttpServletRequestWrapper((HttpServletRequest) request) {
+      private final Map<String, Object> attributes = new HashMap<>();
+
+      @Override
+      public void setAttribute(String name, Object o) {
+        attributes.put(name, o);
+      }
+
+      @Override
+      public Object getAttribute(String name) {
+        Object value = attributes.get(name);
+        return value != null ? value : super.getAttribute(name);
+      }
+
+      @Override
+      public void removeAttribute(String name) {
+        attributes.remove(name);
+      }
+    };
+  }
+
   /**
    * When a HandlerMapping matches a request, it sets HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE
    * as an attribute on the request. This attribute is read by SpringWebMvcDecorator.onRequest and
@@ -186,19 +201,4 @@ private static void parseAndCache(HttpServletRequest request) {
       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);
-    }
-  }
 }

instrumentation/spring/spring-webmvc/spring-webmvc-6.0/javaagent/src/main/java/org/springframework/web/servlet/v6_0/OpenTelemetryHandlerMappingFilter.java

@@ -18,14 +18,16 @@
 import jakarta.servlet.ServletRequest;
 import jakarta.servlet.ServletResponse;
 import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletRequestWrapper;
 import jakarta.servlet.http.HttpServletResponse;
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 import java.util.Objects;
 import javax.annotation.Nullable;
 import org.springframework.core.Ordered;
-import org.springframework.http.server.RequestPath;
 import org.springframework.web.servlet.HandlerExecutionChain;
 import org.springframework.web.servlet.HandlerMapping;
 import org.springframework.web.servlet.mvc.method.annotation.RequestMappingHandlerMapping;
@@ -35,25 +37,16 @@ public class OpenTelemetryHandlerMappingFilter implements Filter, Ordered {
 
   private final HttpServerRouteGetter<HttpServletRequest> serverSpanName =
       (context, request) -> {
-        RequestPath previousValue = null;
         if (this.parseRequestPath) {
-          previousValue =
-              (RequestPath) request.getAttribute(ServletRequestPathUtils.PATH_ATTRIBUTE);
           // sets new value for PATH_ATTRIBUTE of request
           ServletRequestPathUtils.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) {
-            ServletRequestPathUtils.setParsedRequestPath(previousValue, 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);
         }
+
         return null;
       };
 
@@ -77,14 +70,39 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha
     } finally {
       if (handlerMappings != null) {
         Context context = Context.current();
-        HttpServerRoute.update(context, CONTROLLER, serverSpanName, (HttpServletRequest) request);
+        HttpServerRoute.update(context, CONTROLLER, serverSpanName, prepareRequest(request));
       }
     }
   }
 
   @Override
   public void destroy() {}
 
+  private static HttpServletRequest prepareRequest(ServletRequest request) {
+    // https://github.com/open-telemetry/opentelemetry-java-instrumentation/issues/10379
+    // Finding the request handler modifies request attributes. We are wrapping the request to avoid
+    // this.
+    return new HttpServletRequestWrapper((HttpServletRequest) request) {
+      private final Map<String, Object> attributes = new HashMap<>();
+
+      @Override
+      public void setAttribute(String name, Object o) {
+        attributes.put(name, o);
+      }
+
+      @Override
+      public Object getAttribute(String name) {
+        Object value = attributes.get(name);
+        return value != null ? value : super.getAttribute(name);
+      }
+
+      @Override
+      public void removeAttribute(String name) {
+        attributes.remove(name);
+      }
+    };
+  }
+
   /**
    * When a HandlerMapping matches a request, it sets HandlerMapping.BEST_MATCHING_PATTERN_ATTRIBUTE
    * as an attribute on the request. This attribute is read by SpringWebMvcDecorator.onRequest and