From 1e40ae9e9140c6c09d8f305547fc6f24ad0f1f83 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 24 Nov 2021 19:22:39 +0200 Subject: [PATCH 1/2] Capture servlet request parameters --- docs/config/common.md | 13 +++ .../api/servlet/AppServerBridge.java | 86 +++++++++++++------ .../jetty/v11_0/Jetty11Singletons.java | 2 +- .../jetty/v8_0/Jetty8Singletons.java | 2 +- .../liberty/LibertySingletons.java | 2 +- .../servlet-3.0/javaagent/build.gradle.kts | 4 + .../test/groovy/AbstractServlet3Test.groovy | 7 ++ .../src/test/groovy/JettyServlet3Test.groovy | 5 ++ .../groovy/JettyServletHandlerTest.groovy | 3 + .../src/test/groovy/TestServlet3.groovy | 14 +++ .../src/test/groovy/TomcatServlet3Test.groovy | 5 ++ .../servlet-5.0/javaagent/build.gradle.kts | 4 + .../servlet/v5_0/Servlet5Accessor.java | 8 ++ .../test/groovy/AbstractServlet5Test.groovy | 8 ++ .../src/test/groovy/JettyServlet5Test.groovy | 6 ++ .../groovy/JettyServletHandlerTest.groovy | 3 + .../src/test/groovy/TestServlet5.groovy | 15 ++++ .../src/test/groovy/TomcatServlet5Test.groovy | 6 ++ .../servlet/BaseServletHelper.java | 21 +++++ .../servlet/ServletInstrumenterBuilder.java | 5 ++ .../ServletRequestParametersExtractor.java | 72 ++++++++++++++++ .../servlet/ServletAccessor.java | 2 + .../servlet/javax/JavaxServletAccessor.java | 8 ++ .../tomcat-10.0/javaagent/build.gradle.kts | 4 + .../tomcat/v10_0/TestServlet.java | 2 +- .../tomcat/v10_0/TomcatHandlerTest.groovy | 5 ++ .../tomcat-7.0/javaagent/build.gradle.kts | 4 + .../tomcat/v7_0/TestServlet.java | 2 +- .../tomcat/v7_0/TomcatHandlerTest.groovy | 5 ++ .../common/TomcatInstrumenterFactory.java | 6 +- .../undertow/UndertowSingletons.java | 6 +- .../test/base/HttpServerTest.groovy | 43 +++++++++- 32 files changed, 343 insertions(+), 35 deletions(-) create mode 100644 instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java diff --git a/docs/config/common.md b/docs/config/common.md index a185db3d1a88..14554bf36704 100644 --- a/docs/config/common.md +++ b/docs/config/common.md @@ -49,3 +49,16 @@ These configuration options are supported by all HTTP client and server instrume > **Note**: The property/environment variable names listed in the table are still experimental, > and thus are subject to change. + +## Capturing servlet request parameters + +You can configure the agent to capture predefined HTTP request parameter as span attributes for +requests that are handled by Servlet API. +Use the following property to define which servlet request parameters you want to capture: + +| System property | Environment variable | Description | +| ---------------------------------------------------------------------- | ---------------------------------------------------------------------- | ----------- | +| `otel.instrumentation.servlet.experimental.capture-request-parameters` | `OTEL_INSTRUMENTATION_SERVLET_EXPERIMENTAL_CAPTURE_REQUEST_PARAMETERS` | A comma-separated list of request parameter names. + +> **Note**: The property/environment variable names listed in the table are still experimental, +> and thus are subject to change. diff --git a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java index d6a5975faee0..0e55d2a37cab 100644 --- a/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java +++ b/instrumentation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java @@ -18,35 +18,13 @@ public class AppServerBridge { private static final ContextKey CONTEXT_KEY = ContextKey.named("opentelemetry-servlet-app-server-bridge"); - /** - * Attach AppServerBridge to context. - * - * @param ctx server context - * @return new context with AppServerBridge attached. - */ - public static Context init(Context ctx) { - return init(ctx, /* shouldRecordException= */ true); - } - - /** - * Attach AppServerBridge to context. - * - * @param ctx server context - * @param shouldRecordException whether servlet integration should record exception thrown during - * servlet invocation in server span. Use false on servers where exceptions - * thrown during servlet invocation are propagated to the method where server span is closed - * and can be added to server span there and true otherwise. - * @return new context with AppServerBridge attached. - */ - public static Context init(Context ctx, boolean shouldRecordException) { - return ctx.with(AppServerBridge.CONTEXT_KEY, new AppServerBridge(shouldRecordException)); - } - private final boolean servletShouldRecordException; + private boolean captureServletAttributes; private Throwable exception; - private AppServerBridge(boolean shouldRecordException) { - servletShouldRecordException = shouldRecordException; + private AppServerBridge(Builder builder) { + servletShouldRecordException = builder.recordException; + captureServletAttributes = builder.captureServletAttributes; } /** @@ -78,6 +56,23 @@ public static Throwable getException(Context context) { return null; } + /** + * Test whether servlet attributes should be captured. This method will return true only on the + * first call with given context. + * + * @param context server context + * @return true when servlet attributes should be captured + */ + public static boolean captureServletAttributes(Context context) { + AppServerBridge appServerBridge = context.get(AppServerBridge.CONTEXT_KEY); + if (appServerBridge != null) { + boolean result = appServerBridge.captureServletAttributes; + appServerBridge.captureServletAttributes = false; + return result; + } + return false; + } + /** * Class used as key in CallDepthThreadLocalMap for counting servlet invocation depth in * Servlet3Advice and Servlet2Advice. We can not use helper classes like Servlet3Advice and @@ -91,4 +86,43 @@ class Key {} return Key.class; } + + public static class Builder { + boolean recordException; + boolean captureServletAttributes; + + /** + * Use on servers where exceptions thrown during servlet invocation are not propagated to the + * method where server span is closed. Recorded exception can be retrieved by calling {@link + * #getException(Context)} + * + * @return this builder. + */ + public Builder recordException() { + recordException = true; + return this; + } + + /** + * Use on servers where server instrumentation is not based on servlet instrumentation. Setting + * this flag lets servlet instrumentation know that it should augment server span with servlet + * specific attributes. + * + * @return this builder. + */ + public Builder captureServletAttributes() { + captureServletAttributes = true; + return this; + } + + /** + * Attach AppServerBridge to context. + * + * @param context server context + * @return new context with AppServerBridge attached. + */ + public Context init(Context context) { + return context.with(AppServerBridge.CONTEXT_KEY, new AppServerBridge(this)); + } + } } diff --git a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java index db82a20d6f35..44ea727e2561 100644 --- a/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java +++ b/instrumentation/jetty/jetty-11.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java @@ -28,7 +28,7 @@ public final class Jetty11Singletons { .addContextCustomizer( (context, request, attributes) -> { context = ServerSpanNaming.init(context, CONTAINER); - return AppServerBridge.init(context, /* shouldRecordException= */ false); + return new AppServerBridge.Builder().init(context); }) .build(INSTRUMENTATION_NAME, Servlet5Accessor.INSTANCE); diff --git a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java index fc2e8844544f..881426b69993 100644 --- a/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java +++ b/instrumentation/jetty/jetty-8.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java @@ -28,7 +28,7 @@ public final class Jetty8Singletons { .addContextCustomizer( (context, request, attributes) -> { context = ServerSpanNaming.init(context, CONTAINER); - return AppServerBridge.init(context, /* shouldRecordException= */ false); + return new AppServerBridge.Builder().init(context); }) .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE); diff --git a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java index c2a8a1aa0266..c3c637896ad9 100644 --- a/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java +++ b/instrumentation/liberty/liberty/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java @@ -27,7 +27,7 @@ public final class LibertySingletons { .addContextCustomizer( (context, request, attributes) -> { context = ServerSpanNaming.init(context, CONTAINER); - return AppServerBridge.init(context); + return new AppServerBridge.Builder().recordException().init(context); }) .build(INSTRUMENTATION_NAME, Servlet3Accessor.INSTANCE); diff --git a/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts b/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts index 7a110248ff7f..84dd6acea0c7 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts +++ b/instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts @@ -35,3 +35,7 @@ dependencies { latestDepTestLibrary("org.apache.tomcat.embed:tomcat-embed-core:9.+") latestDepTestLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:9.+") } + +tasks.withType().configureEach { + jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter") +} diff --git a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy index fd7fc52a238b..8d1c9f1408c4 100644 --- a/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy +++ b/instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy @@ -14,6 +14,7 @@ import javax.servlet.Servlet import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_HEADERS +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD @@ -48,6 +49,7 @@ abstract class AbstractServlet3Test extends HttpServerTest extends HttpServerTest().configureEach { + jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter") +} diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/Servlet5Accessor.java b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/Servlet5Accessor.java index 100112c97179..40883276bf7b 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/Servlet5Accessor.java +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/Servlet5Accessor.java @@ -14,6 +14,7 @@ import jakarta.servlet.http.HttpServletResponse; import java.security.Principal; import java.util.ArrayList; +import java.util.Arrays; import java.util.Collection; import java.util.Collections; import java.util.Enumeration; @@ -95,6 +96,13 @@ public Iterable getRequestHeaderNames(HttpServletRequest httpServletRequ return Collections.list(httpServletRequest.getHeaderNames()); } + @Override + public List getRequestParameterValues( + HttpServletRequest httpServletRequest, String name) { + String[] values = httpServletRequest.getParameterValues(name); + return values == null ? Collections.emptyList() : Arrays.asList(values); + } + @Override public String getRequestServletPath(HttpServletRequest request) { return request.getServletPath(); diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5Test.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5Test.groovy index 9b48396aef1f..df5adfd5489b 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5Test.groovy +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5Test.groovy @@ -3,6 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS + import io.opentelemetry.api.common.AttributeKey import io.opentelemetry.instrumentation.test.AgentTestTrait import io.opentelemetry.instrumentation.test.asserts.TraceAssert @@ -47,6 +49,7 @@ abstract class AbstractServlet5Test extends HttpServerTest extends HttpServerTest { @Override String expectedServerSpanName(ServerEndpoint endpoint) { + if (ServerEndpoint.CAPTURE_PARAMETERS == endpoint) { + return "HTTP POST" + } "HTTP GET" } diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy index 93ac9ef48a8a..10095820d5fe 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TestServlet5.groovy @@ -3,6 +3,8 @@ * SPDX-License-Identifier: Apache-2.0 */ +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS + import io.opentelemetry.instrumentation.test.base.HttpServerTest import jakarta.servlet.RequestDispatcher import jakarta.servlet.ServletException @@ -55,6 +57,10 @@ class TestServlet5 { resp.status = endpoint.status resp.writer.print(endpoint.body) break + case CAPTURE_PARAMETERS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break case ERROR: resp.sendError(endpoint.status, endpoint.body) break @@ -105,6 +111,11 @@ class TestServlet5 { resp.writer.print(endpoint.body) context.complete() break + case CAPTURE_PARAMETERS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + context.complete() + break case ERROR: resp.status = endpoint.status resp.writer.print(endpoint.body) @@ -155,6 +166,10 @@ class TestServlet5 { resp.status = endpoint.status resp.writer.print(endpoint.body) break + case CAPTURE_PARAMETERS: + resp.status = endpoint.status + resp.writer.print(endpoint.body) + break case ERROR: resp.sendError(endpoint.status, endpoint.body) break diff --git a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy index 0d673603dd69..c5f8b5bb01e3 100644 --- a/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy +++ b/instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/TomcatServlet5Test.groovy @@ -28,6 +28,7 @@ import java.util.concurrent.TimeoutException import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.AUTH_REQUIRED import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_HEADERS +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD @@ -350,6 +351,7 @@ class TomcatServlet5TestForward extends TomcatDispatchTest { addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Forward) addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Forward) addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, RequestDispatcherServlet.Forward) + addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, RequestDispatcherServlet.Forward) addServlet(context, "/dispatch" + INDEXED_CHILD.path, RequestDispatcherServlet.Forward) } } @@ -390,6 +392,8 @@ class TomcatServlet5TestInclude extends TomcatDispatchTest { addServlet(context, "/dispatch" + ERROR.path, RequestDispatcherServlet.Include) addServlet(context, "/dispatch" + EXCEPTION.path, RequestDispatcherServlet.Include) addServlet(context, "/dispatch" + AUTH_REQUIRED.path, RequestDispatcherServlet.Include) + addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, RequestDispatcherServlet.Include) + addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, RequestDispatcherServlet.Include) addServlet(context, "/dispatch" + INDEXED_CHILD.path, RequestDispatcherServlet.Include) } } @@ -416,6 +420,7 @@ class TomcatServlet5TestDispatchImmediate extends TomcatDispatchTest { addServlet(context, "/dispatch" + REDIRECT.path, TestServlet5.DispatchImmediate) addServlet(context, "/dispatch" + AUTH_REQUIRED.path, TestServlet5.DispatchImmediate) addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, TestServlet5.DispatchImmediate) + addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, TestServlet5.DispatchImmediate) addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet5.DispatchImmediate) addServlet(context, "/dispatch/recursive", TestServlet5.DispatchRecursive) } @@ -438,6 +443,7 @@ class TomcatServlet5TestDispatchAsync extends TomcatDispatchTest { addServlet(context, "/dispatch" + REDIRECT.path, TestServlet5.DispatchAsync) addServlet(context, "/dispatch" + AUTH_REQUIRED.path, TestServlet5.DispatchAsync) addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, TestServlet5.DispatchAsync) + addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, TestServlet5.DispatchAsync) addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet5.DispatchAsync) addServlet(context, "/dispatch/recursive", TestServlet5.DispatchRecursive) } diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/BaseServletHelper.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/BaseServletHelper.java index dea7dd370c73..29740f980f2d 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/BaseServletHelper.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/BaseServletHelper.java @@ -16,6 +16,7 @@ import io.opentelemetry.instrumentation.api.servlet.MappingResolver; import io.opentelemetry.instrumentation.api.servlet.ServerSpanNaming; import io.opentelemetry.instrumentation.api.servlet.ServletContextPath; +import io.opentelemetry.instrumentation.api.tracer.ServerSpan; import io.opentelemetry.instrumentation.servlet.ServletAccessor; import io.opentelemetry.instrumentation.servlet.naming.ServletSpanNameProvider; import java.util.function.Function; @@ -26,6 +27,7 @@ public abstract class BaseServletHelper { protected final ServletAccessor accessor; private final ServletSpanNameProvider spanNameProvider; private final Function contextPathExtractor; + private final ServletRequestParametersExtractor parameterExtractor; protected BaseServletHelper( Instrumenter, ServletResponseContext> instrumenter, @@ -34,6 +36,10 @@ protected BaseServletHelper( this.accessor = accessor; this.spanNameProvider = new ServletSpanNameProvider<>(accessor); this.contextPathExtractor = accessor::getRequestContextPath; + this.parameterExtractor = + ServletRequestParametersExtractor.enabled() + ? new ServletRequestParametersExtractor<>(accessor) + : null; } public boolean shouldStart(Context parentContext, ServletRequestContext requestContext) { @@ -83,9 +89,24 @@ public Context updateContext( ServerSpanNaming.updateServerSpanName( result, servlet ? SERVLET : FILTER, spanNameProvider, mappingResolver, request); } + + captureServletAttributes(context, request); + return result; } + private void captureServletAttributes(Context context, REQUEST request) { + if (parameterExtractor == null || !AppServerBridge.captureServletAttributes(context)) { + return; + } + Span serverSpan = ServerSpan.fromContextOrNull(context); + if (serverSpan == null) { + return; + } + + parameterExtractor.setAttributes(request, (key, value) -> serverSpan.setAttribute(key, value)); + } + /* Given request already has a context associated with it. As there should not be nested spans of kind SERVER, we should NOT create a new span here. diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java index c96f4edfafe3..15adc40d52f0 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletInstrumenterBuilder.java @@ -74,6 +74,11 @@ public Instrumenter, ServletResponseContext, ServletResponseContext> + requestParametersExtractor = new ServletRequestParametersExtractor<>(accessor); + builder.addAttributesExtractor(requestParametersExtractor); + } for (ContextCustomizer> contextCustomizer : contextCustomizers) { builder.addContextCustomizer(contextCustomizer); diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java new file mode 100644 index 000000000000..b113a516b601 --- /dev/null +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java @@ -0,0 +1,72 @@ +/* + * Copyright The OpenTelemetry Authors + * SPDX-License-Identifier: Apache-2.0 + */ + +package io.opentelemetry.javaagent.instrumentation.servlet; + +import io.opentelemetry.api.common.AttributeKey; +import io.opentelemetry.api.common.AttributesBuilder; +import io.opentelemetry.instrumentation.api.caching.Cache; +import io.opentelemetry.instrumentation.api.config.Config; +import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; +import io.opentelemetry.instrumentation.servlet.ServletAccessor; +import java.util.List; +import java.util.Locale; +import java.util.function.BiConsumer; +import javax.annotation.Nullable; + +public class ServletRequestParametersExtractor + implements AttributesExtractor< + ServletRequestContext, ServletResponseContext> { + private static final List CAPTURE_REQUEST_PARAMETERS = + Config.get().getList("otel.instrumentation.servlet.experimental.capture-request-parameters"); + + private static final Cache>> parameterKeysCache = + Cache.builder().build(); + + private final ServletAccessor accessor; + + public ServletRequestParametersExtractor(ServletAccessor accessor) { + this.accessor = accessor; + } + + public static boolean enabled() { + return !CAPTURE_REQUEST_PARAMETERS.isEmpty(); + } + + public void setAttributes( + REQUEST request, BiConsumer>, List> consumer) { + for (String name : CAPTURE_REQUEST_PARAMETERS) { + List values = accessor.getRequestParameterValues(request, name); + if (!values.isEmpty()) { + consumer.accept(parameterAttributeKey(name), values); + } + } + } + + @Override + public void onStart(AttributesBuilder attributes, ServletRequestContext requestContext) { + REQUEST request = requestContext.request(); + setAttributes(request, (key, value) -> set(attributes, key, value)); + } + + @Override + public void onEnd( + AttributesBuilder attributes, + ServletRequestContext requestContext, + @Nullable ServletResponseContext responseContext, + @Nullable Throwable error) {} + + private static AttributeKey> parameterAttributeKey(String headerName) { + return parameterKeysCache.computeIfAbsent(headerName, n -> createKey(n)); + } + + private static AttributeKey> createKey(String parameterName) { + // normalize parameter name similarly as is done with header names when header values are + // captured as span attributes + parameterName = parameterName.toLowerCase(Locale.ROOT); + String key = "servlet.request.parameter." + parameterName.replace('-', '_'); + return AttributeKey.stringArrayKey(key); + } +} 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 c44fed922550..7f8f8dfa8210 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 @@ -47,6 +47,8 @@ public interface ServletAccessor { Iterable getRequestHeaderNames(REQUEST request); + List getRequestParameterValues(REQUEST request, String name); + String getRequestServletPath(REQUEST request); String getRequestPathInfo(REQUEST request); 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 0ef178e4829d..1db7cf52f613 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 @@ -7,6 +7,7 @@ import io.opentelemetry.instrumentation.servlet.ServletAccessor; import java.security.Principal; +import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.List; @@ -90,6 +91,13 @@ public Iterable getRequestHeaderNames(HttpServletRequest httpServletRequ return Collections.list(httpServletRequest.getHeaderNames()); } + @Override + public List getRequestParameterValues( + HttpServletRequest httpServletRequest, String name) { + String[] values = httpServletRequest.getParameterValues(name); + return values == null ? Collections.emptyList() : Arrays.asList(values); + } + @Override public String getRequestServletPath(HttpServletRequest request) { return request.getServletPath(); diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/build.gradle.kts b/instrumentation/tomcat/tomcat-10.0/javaagent/build.gradle.kts index c07f86048a19..6c094ff43106 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/build.gradle.kts +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/build.gradle.kts @@ -17,3 +17,7 @@ dependencies { // Make sure nothing breaks due to both 7.0 and 10.0 modules being present together testInstrumentation(project(":instrumentation:tomcat:tomcat-7.0:javaagent")) } + +tasks.withType().configureEach { + jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter") +} diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TestServlet.java b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TestServlet.java index fe74846ada0e..cf30773d9c02 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TestServlet.java +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TestServlet.java @@ -14,7 +14,7 @@ public class TestServlet extends HttpServlet { @Override - protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException { String path = req.getServletPath(); HttpServerTest.ServerEndpoint serverEndpoint = HttpServerTest.ServerEndpoint.forPath(path); diff --git a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy index 4766b4ea4133..5c3a890f6b2b 100644 --- a/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-10.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v10_0/TomcatHandlerTest.groovy @@ -43,6 +43,11 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait } } + @Override + boolean testCapturedRequestParameters() { + true + } + @Override Tomcat startServer(int port) { Tomcat tomcat = new Tomcat() diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/build.gradle.kts b/instrumentation/tomcat/tomcat-7.0/javaagent/build.gradle.kts index 9490d745b9e3..2e87102c13f5 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/build.gradle.kts +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/build.gradle.kts @@ -25,3 +25,7 @@ dependencies { latestDepTestLibrary("org.apache.tomcat.embed:tomcat-embed-core:[9.+, 10)") latestDepTestLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:[9.+, 10)") } + +tasks.withType().configureEach { + jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter") +} diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TestServlet.java b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TestServlet.java index dfe34f613f56..6fed26cab497 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TestServlet.java +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TestServlet.java @@ -14,7 +14,7 @@ public class TestServlet extends HttpServlet { @Override - protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOException { + protected void service(HttpServletRequest req, HttpServletResponse resp) throws IOException { String path = req.getServletPath(); HttpServerTest.ServerEndpoint serverEndpoint = HttpServerTest.ServerEndpoint.forPath(path); diff --git a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy index d7d9ce5fc04b..62778c397d94 100644 --- a/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy +++ b/instrumentation/tomcat/tomcat-7.0/javaagent/src/test/groovy/io/opentelemetry/javaagent/instrumentation/tomcat/v7_0/TomcatHandlerTest.groovy @@ -43,6 +43,11 @@ class TomcatHandlerTest extends HttpServerTest implements AgentTestTrait } } + @Override + boolean testCapturedRequestParameters() { + true + } + @Override Tomcat startServer(int port) { Tomcat tomcat = new Tomcat() diff --git a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java index f70aee14c5a5..ef63ea83c2b8 100644 --- a/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java +++ b/instrumentation/tomcat/tomcat-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/tomcat/common/TomcatInstrumenterFactory.java @@ -53,7 +53,11 @@ public static Instrumenter create( .addContextCustomizer( (context, request, attributes) -> { context = ServerSpanNaming.init(context, CONTAINER); - return AppServerBridge.init(context); + + return new AppServerBridge.Builder() + .captureServletAttributes() + .recordException() + .init(context); }) .addRequestMetrics(HttpServerMetrics.get()) .newServerInstrumenter(TomcatRequestGetter.INSTANCE); diff --git a/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowSingletons.java b/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowSingletons.java index 35a79683c607..bc9b35bdc680 100644 --- a/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowSingletons.java +++ b/instrumentation/undertow-1.4/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/undertow/UndertowSingletons.java @@ -48,7 +48,11 @@ public final class UndertowSingletons { // span is ended when counter reaches 0, we start from 2 which accounts for the // handler that started the span and exchange completion listener context = UndertowActiveHandlers.init(context, 2); - return AppServerBridge.init(context); + + return new AppServerBridge.Builder() + .captureServletAttributes() + .recordException() + .init(context); }) .addRequestMetrics(HttpServerMetrics.get()) .newServerInstrumenter(UndertowExchangeGetter.INSTANCE); diff --git a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy index 863c08e9d3a8..bb8758669150 100644 --- a/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy +++ b/testing-common/src/main/groovy/io/opentelemetry/instrumentation/test/base/HttpServerTest.groovy @@ -18,15 +18,20 @@ import io.opentelemetry.sdk.trace.data.SpanData import io.opentelemetry.semconv.trace.attributes.SemanticAttributes import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpRequest import io.opentelemetry.testing.internal.armeria.common.AggregatedHttpResponse +import io.opentelemetry.testing.internal.armeria.common.HttpData import io.opentelemetry.testing.internal.armeria.common.HttpMethod import io.opentelemetry.testing.internal.armeria.common.HttpRequest import io.opentelemetry.testing.internal.armeria.common.HttpRequestBuilder +import io.opentelemetry.testing.internal.armeria.common.MediaType +import io.opentelemetry.testing.internal.armeria.common.QueryParams +import io.opentelemetry.testing.internal.armeria.common.RequestHeaders import spock.lang.Unroll import java.util.concurrent.Callable import java.util.concurrent.CountDownLatch import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_HEADERS +import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.CAPTURE_PARAMETERS import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.ERROR import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.EXCEPTION import static io.opentelemetry.instrumentation.test.base.HttpServerTest.ServerEndpoint.INDEXED_CHILD @@ -119,6 +124,10 @@ abstract class HttpServerTest extends InstrumentationSpecification imple true } + boolean testCapturedRequestParameters() { + false + } + boolean testErrorBody() { true } @@ -154,6 +163,7 @@ abstract class HttpServerTest extends InstrumentationSpecification imple EXCEPTION("exception", 500, "controller exception"), NOT_FOUND("notFound", 404, "not found"), CAPTURE_HEADERS("captureHeaders", 200, "headers captured"), + CAPTURE_PARAMETERS("captureParameters", 200, "parameters captured"), // TODO: add tests for the following cases: QUERY_PARAM("query?some=query", 200, "some=query"), @@ -233,14 +243,18 @@ abstract class HttpServerTest extends InstrumentationSpecification imple } } - AggregatedHttpRequest request(ServerEndpoint uri, String method) { + String resolveAddress(ServerEndpoint uri) { def url = uri.resolvePath(address).toString() // Force HTTP/1 via h1c so upgrade requests don't show up as traces url = url.replace("http://", "h1c://") if (uri.query != null) { url += "?${uri.query}" } - return AggregatedHttpRequest.of(HttpMethod.valueOf(method), url) + return url + } + + AggregatedHttpRequest request(ServerEndpoint uri, String method) { + return AggregatedHttpRequest.of(HttpMethod.valueOf(method), resolveAddress(uri)) } static T controller(ServerEndpoint endpoint, Callable closure) { @@ -414,6 +428,28 @@ abstract class HttpServerTest extends InstrumentationSpecification imple assertTheTraces(1, null, null, "GET", CAPTURE_HEADERS, null, response) } + def "test captured request parameters"() { + setup: + assumeTrue(testCapturedRequestParameters()) + + QueryParams formBody = QueryParams.builder() + .add("test-parameter", "test value") + .build() + def request = AggregatedHttpRequest.of( + RequestHeaders.builder(HttpMethod.POST, resolveAddress(CAPTURE_PARAMETERS)) + .contentType(MediaType.FORM_DATA) + .build(), + HttpData.ofUtf8(formBody.toQueryString())) + def response = client.execute(request).aggregate().join() + + expect: + response.status().code() == CAPTURE_PARAMETERS.status + response.contentUtf8() == CAPTURE_PARAMETERS.body + + and: + assertTheTraces(1, null, null, "POST", CAPTURE_PARAMETERS, null, response) + } + /* This test fires a bunch of parallel request to the fixed backend endpoint. That endpoint is supposed to create a new child span in the context of the SERVER span. @@ -658,6 +694,9 @@ abstract class HttpServerTest extends InstrumentationSpecification imple "http.request.header.x_test_request" { it == ["test"] } "http.response.header.x_test_response" { it == ["test"] } } + if (endpoint == CAPTURE_PARAMETERS) { + "servlet.request.parameter.test_parameter" { it == ["test value"] } + } } } } From 663fb45764e80920321456ac5faf3df167a67900 Mon Sep 17 00:00:00 2001 From: Lauri Tulmin Date: Wed, 24 Nov 2021 20:19:44 +0200 Subject: [PATCH 2/2] use concurrenthashmap for cache --- .../servlet/ServletRequestParametersExtractor.java | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java index b113a516b601..1c74e5300ab0 100644 --- a/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java +++ b/instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/ServletRequestParametersExtractor.java @@ -7,12 +7,13 @@ import io.opentelemetry.api.common.AttributeKey; import io.opentelemetry.api.common.AttributesBuilder; -import io.opentelemetry.instrumentation.api.caching.Cache; import io.opentelemetry.instrumentation.api.config.Config; import io.opentelemetry.instrumentation.api.instrumenter.AttributesExtractor; import io.opentelemetry.instrumentation.servlet.ServletAccessor; import java.util.List; import java.util.Locale; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.ConcurrentMap; import java.util.function.BiConsumer; import javax.annotation.Nullable; @@ -22,8 +23,8 @@ public class ServletRequestParametersExtractor private static final List CAPTURE_REQUEST_PARAMETERS = Config.get().getList("otel.instrumentation.servlet.experimental.capture-request-parameters"); - private static final Cache>> parameterKeysCache = - Cache.builder().build(); + private static final ConcurrentMap>> parameterKeysCache = + new ConcurrentHashMap<>(); private final ServletAccessor accessor;