Capture servlet request parameters at the end of the request (open-te…

…lemetry#5019)

* Capture servlet request parameters at the end of the request

* add comment

instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/HttpServletResponseInstrumentation.java

@@ -30,9 +30,9 @@
  * created span using just response object.
  *
  * <p>This instrumentation intercepts status setting methods from Servlet 2.0 specification and
- * stores that status into context store. Then {@link Servlet2Advice#stopSpan(ServletResponse,
- * Throwable, CallDepth, ServletRequestContext, Context, Scope)} can get it from context and set
- * required span attribute.
+ * stores that status into context store. Then {@link Servlet2Advice#stopSpan(ServletRequest,
+ * ServletResponse, Throwable, CallDepth, ServletRequestContext, Context, Scope)} can get it from
+ * context and set required span attribute.
  */
 public class HttpServletResponseInstrumentation implements TypeInstrumentation {
   @Override

instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Advice.java

@@ -32,15 +32,16 @@ public static void onEnter(
       @Advice.Local("otelRequest") ServletRequestContext<HttpServletRequest> requestContext,
       @Advice.Local("otelContext") Context context,
       @Advice.Local("otelScope") Scope scope) {
-    callDepth = CallDepth.forClass(AppServerBridge.getCallDepthKey());
-    callDepth.getAndIncrement();
 
     if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
       return;
     }
 
     HttpServletRequest httpServletRequest = (HttpServletRequest) request;
 
+    callDepth = CallDepth.forClass(AppServerBridge.getCallDepthKey());
+    callDepth.getAndIncrement();
+
     Context serverContext = helper().getServerContext(httpServletRequest);
     if (serverContext != null) {
       Context updatedContext = helper().updateContext(serverContext, httpServletRequest);
@@ -67,26 +68,32 @@ public static void onEnter(
 
   @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
   public static void stopSpan(
+      @Advice.Argument(0) ServletRequest request,
       @Advice.Argument(1) ServletResponse response,
       @Advice.Thrown Throwable throwable,
       @Advice.Local("otelCallDepth") CallDepth callDepth,
       @Advice.Local("otelRequest") ServletRequestContext<HttpServletRequest> requestContext,
       @Advice.Local("otelContext") Context context,
       @Advice.Local("otelScope") Scope scope) {
 
-    int depth = callDepth.decrementAndGet();
+    if (!(request instanceof HttpServletRequest) || !(response instanceof HttpServletResponse)) {
+      return;
+    }
 
     if (scope != null) {
       scope.close();
     }
 
-    if (context == null && depth == 0) {
+    boolean topLevel = callDepth.decrementAndGet() == 0;
+    if (context == null && topLevel) {
       Context currentContext = Java8BytecodeBridge.currentContext();
       // Something else is managing the context, we're in the outermost level of Servlet
       // instrumentation and we have an uncaught throwable. Let's add it to the current span.
       if (throwable != null) {
         helper().recordException(currentContext, throwable);
       }
+      // also capture request parameters as servlet attributes
+      helper().captureServletAttributes(currentContext, (HttpServletRequest) request);
     }
 
     if (scope == null || context == null) {
@@ -100,7 +107,7 @@ public static void stopSpan(
     }
 
     helper()
-        .stopSpan(
+        .end(
             context, requestContext, (HttpServletResponse) response, responseStatusCode, throwable);
   }
 }

instrumentation/servlet/servlet-2.2/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v2_2/Servlet2Helper.java

@@ -23,7 +23,7 @@ public class Servlet2Helper extends BaseServletHelper<HttpServletRequest, HttpSe
     super(instrumenter, Servlet2Accessor.INSTANCE);
   }
 
-  public void stopSpan(
+  public void end(
       Context context,
       ServletRequestContext<HttpServletRequest> requestContext,
       HttpServletResponse response,

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

@@ -56,6 +56,11 @@ class TestServlet3 {
             resp.writer.print(endpoint.body)
             break
           case CAPTURE_PARAMETERS:
+            req.setCharacterEncoding("UTF8")
+            def value = req.getParameter("test-parameter")
+            if (value != "test value õäöü") {
+              throw new ServletException("request parameter does not have expected value " + value)
+            }
             resp.status = endpoint.status
             resp.writer.print(endpoint.body)
             break
@@ -110,6 +115,11 @@ class TestServlet3 {
                 context.complete()
                 break
               case CAPTURE_PARAMETERS:
+                req.setCharacterEncoding("UTF8")
+                def value = req.getParameter("test-parameter")
+                if (value != "test value õäöü") {
+                  throw new ServletException("request parameter does not have expected value " + value)
+                }
                 resp.status = endpoint.status
                 resp.writer.print(endpoint.body)
                 context.complete()
@@ -171,6 +181,11 @@ class TestServlet3 {
               resp.writer.print(endpoint.body)
               break
             case CAPTURE_PARAMETERS:
+              req.setCharacterEncoding("UTF8")
+              def value = req.getParameter("test-parameter")
+              if (value != "test value õäöü") {
+                throw new ServletException("request parameter does not have expected value " + value)
+              }
               resp.status = endpoint.status
               resp.writer.print(endpoint.body)
               break

instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy

@@ -58,6 +58,11 @@ class TestServlet5 {
             resp.writer.print(endpoint.body)
             break
           case CAPTURE_PARAMETERS:
+            req.setCharacterEncoding("UTF8")
+            def value = req.getParameter("test-parameter")
+            if (value != "test value õäöü") {
+              throw new ServletException("request parameter does not have expected value " + value)
+            }
             resp.status = endpoint.status
             resp.writer.print(endpoint.body)
             break
@@ -112,6 +117,11 @@ class TestServlet5 {
                 context.complete()
                 break
               case CAPTURE_PARAMETERS:
+                req.setCharacterEncoding("UTF8")
+                def value = req.getParameter("test-parameter")
+                if (value != "test value õäöü") {
+                  throw new ServletException("request parameter does not have expected value " + value)
+                }
                 resp.status = endpoint.status
                 resp.writer.print(endpoint.body)
                 context.complete()
@@ -173,6 +183,11 @@ class TestServlet5 {
               resp.writer.print(endpoint.body)
               break
             case CAPTURE_PARAMETERS:
+              req.setCharacterEncoding("UTF8")
+              def value = req.getParameter("test-parameter")
+              if (value != "test value õäöü") {
+                throw new ServletException("request parameter does not have expected value " + value)
+              }
               resp.status = endpoint.status
               resp.writer.print(endpoint.body)
               break

instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/BaseServletHelper.java

@@ -88,12 +88,18 @@ public Context updateContext(
           result, servlet ? SERVLET : FILTER, spanNameProvider, mappingResolver, request);
     }
 
-    captureServletAttributes(context, request);
-
     return result;
   }
 
-  private void captureServletAttributes(Context context, REQUEST request) {
+  /**
+   * Capture servlet request parameters as span attributes when SERVER span is not create by servlet
+   * instrumentation.
+   *
+   * <p>When SERVER span is created by servlet instrumentation we register {@link
+   * ServletRequestParametersExtractor} as an attribute extractor. When SERVER span is not created
+   * by servlet instrumentation we call this method on exit from the last servlet or filter.
+   */
+  public void captureServletAttributes(Context context, REQUEST request) {
     if (parameterExtractor == null || !AppServerBridge.captureServletAttributes(context)) {
       return;
     }

instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletHelper.java

@@ -49,6 +49,8 @@ public void end(
           recordAsyncException(request, throwable);
         }
       }
+      // also capture request parameters as servlet attributes
+      captureServletAttributes(currentContext, request);
     }
 
     if (scope == null || context == null) {

instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java

@@ -50,17 +50,20 @@ public void setAttributes(
   }
 
   @Override
-  public void onStart(AttributesBuilder attributes, ServletRequestContext<REQUEST> requestContext) {
-    REQUEST request = requestContext.request();
-    setAttributes(request, (key, value) -> set(attributes, key, value));
-  }
+  public void onStart(
+      AttributesBuilder attributes, ServletRequestContext<REQUEST> requestContext) {}
 
   @Override
   public void onEnd(
       AttributesBuilder attributes,
       ServletRequestContext<REQUEST> requestContext,
       @Nullable ServletResponseContext<RESPONSE> responseContext,
-      @Nullable Throwable error) {}
+      @Nullable Throwable error) {
+    // request parameters are extracted at the end of the request to make sure that we don't access
+    // them before request encoding has been set
+    REQUEST request = requestContext.request();
+    setAttributes(request, (key, value) -> set(attributes, key, value));
+  }
 
   private static AttributeKey<List<String>> parameterAttributeKey(String headerName) {
     return parameterKeysCache.computeIfAbsent(headerName, n -> createKey(n));

instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TestServlet.java

@@ -6,6 +6,7 @@
 package io.opentelemetry.javaagent.instrumentation.tomcat.v10_0;
 
 import io.opentelemetry.instrumentation.test.base.HttpServerTest;
+import jakarta.servlet.ServletException;
 import jakarta.servlet.http.HttpServlet;
 import jakarta.servlet.http.HttpServletRequest;
 import jakarta.servlet.http.HttpServletResponse;
@@ -28,6 +29,14 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
             if (serverEndpoint == HttpServerTest.ServerEndpoint.CAPTURE_HEADERS) {
               resp.setHeader("X-Test-Response", req.getHeader("X-Test-Request"));
             }
+            if (serverEndpoint == HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS) {
+              req.setCharacterEncoding("UTF8");
+              String value = req.getParameter("test-parameter");
+              if (!"test value õäöü".equals(value)) {
+                throw new ServletException(
+                    "request parameter does not have expected value " + value);
+              }
+            }
             if (serverEndpoint == HttpServerTest.ServerEndpoint.INDEXED_CHILD) {
               HttpServerTest.ServerEndpoint.INDEXED_CHILD.collectSpanAttributes(req::getParameter);
             }

instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TestServlet.java

@@ -7,6 +7,7 @@
 
 import io.opentelemetry.instrumentation.test.base.HttpServerTest;
 import java.io.IOException;
+import javax.servlet.ServletException;
 import javax.servlet.http.HttpServlet;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
@@ -28,6 +29,14 @@ protected void service(HttpServletRequest req, HttpServletResponse resp) throws
             if (serverEndpoint == HttpServerTest.ServerEndpoint.CAPTURE_HEADERS) {
               resp.setHeader("X-Test-Response", req.getHeader("X-Test-Request"));
             }
+            if (serverEndpoint == HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS) {
+              req.setCharacterEncoding("UTF8");
+              String value = req.getParameter("test-parameter");
+              if (!"test value õäöü".equals(value)) {
+                throw new ServletException(
+                    "request parameter does not have expected value " + value);
+              }
+            }
             if (serverEndpoint == HttpServerTest.ServerEndpoint.INDEXED_CHILD) {
               HttpServerTest.ServerEndpoint.INDEXED_CHILD.collectSpanAttributes(req::getParameter);
             }

testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy

@@ -456,7 +456,7 @@ abstract class HttpServerTest<SERVER> extends InstrumentationSpecification imple
     assumeTrue(testCapturedRequestParameters())
 
     QueryParams formBody = QueryParams.builder()
-      .add("test-parameter", "test value")
+      .add("test-parameter", "test value õäöü")
       .build()
     def request = AggregatedHttpRequest.of(
       RequestHeaders.builder(HttpMethod.POST, resolveAddress(CAPTURE_PARAMETERS))
@@ -717,7 +717,7 @@ abstract class HttpServerTest<SERVER> extends InstrumentationSpecification imple
           "http.response.header.x_test_response" { it == ["test"] }
         }
         if (endpoint == CAPTURE_PARAMETERS) {
-          "servlet.request.parameter.test_parameter" { it == ["test value"] }
+          "servlet.request.parameter.test_parameter" { it == ["test value õäöü"] }
         }
       }
     }