Skip to content

Commit

Permalink
Capture servlet request parameters
Browse files Browse the repository at this point in the history
  • Loading branch information
@laurit
laurit committed Nov 24, 2021
1 parent a532634 commit 93f1823
Show file tree
Hide file tree
Showing 32 changed files with 343 additions and 35 deletions.
13 changes: 13 additions & 0 deletions docs/config/common.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
86 changes: 60 additions & 26 deletions ...ation-api/src/main/java/io/opentelemetry/instrumentation/api/servlet/AppServerBridge.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,35 +18,13 @@ public class AppServerBridge {
private static final ContextKey<AppServerBridge> 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 <code>false</code> 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 <code>true</code> 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;
}

/**
Expand Down Expand Up @@ -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
Expand All @@ -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));
}
}
}
2 changes: 1 addition & 1 deletion ...c/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v11_0/Jetty11Singletons.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion ...src/main/java/io/opentelemetry/javaagent/instrumentation/jetty/v8_0/Jetty8Singletons.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
2 changes: 1 addition & 1 deletion ...t/src/main/java/io/opentelemetry/javaagent/instrumentation/liberty/LibertySingletons.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
4 changes: 4 additions & 0 deletions instrumentation/servlet/servlet-3.0/javaagent/build.gradle.kts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<Test>().configureEach {
jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter")
}
7 changes: 7 additions & 0 deletions instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/AbstractServlet3Test.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -48,6 +49,7 @@ abstract class AbstractServlet3Test<SERVER, CONTEXT> extends HttpServerTest<SERV
addServlet(context, AUTH_REQUIRED.path, servlet)
addServlet(context, INDEXED_CHILD.path, servlet)
addServlet(context, CAPTURE_HEADERS.path, servlet)
addServlet(context, CAPTURE_PARAMETERS.path, servlet)
}

protected ServerEndpoint lastRequest
Expand All @@ -66,6 +68,11 @@ abstract class AbstractServlet3Test<SERVER, CONTEXT> extends HttpServerTest<SERV
]
}

@Override
boolean testCapturedRequestParameters() {
true
}

boolean errorEndpointUsesSendError() {
true
}
Expand Down
5 changes: 5 additions & 0 deletions instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServlet3Test.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import javax.servlet.http.HttpServletRequest

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
Expand Down Expand Up @@ -174,6 +175,7 @@ class JettyServlet3TestForward extends JettyDispatchTest {
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)
}
}
Expand Down Expand Up @@ -209,6 +211,7 @@ class JettyServlet3TestInclude extends JettyDispatchTest {
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_PARAMETERS.path, RequestDispatcherServlet.Include)
addServlet(context, "/dispatch" + INDEXED_CHILD.path, RequestDispatcherServlet.Include)
}
}
Expand Down Expand Up @@ -236,6 +239,7 @@ class JettyServlet3TestDispatchImmediate extends JettyDispatchTest {
addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + AUTH_REQUIRED.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive)
}
Expand Down Expand Up @@ -263,6 +267,7 @@ class JettyServlet3TestDispatchAsync extends JettyDispatchTest {
addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + AUTH_REQUIRED.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive)
}
Expand Down
3 changes: 3 additions & 0 deletions instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/JettyServletHandlerTest.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,9 @@ class JettyServletHandlerTest extends AbstractServlet3Test<Server, ServletHandle

@Override
String expectedServerSpanName(ServerEndpoint endpoint) {
if (endpoint == ServerEndpoint.CAPTURE_PARAMETERS) {
return "HTTP POST"
}
"HTTP GET"
}

Expand Down
14 changes: 14 additions & 0 deletions instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TestServlet3.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import javax.servlet.http.HttpServletResponse
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
Expand Down Expand Up @@ -54,6 +55,10 @@ class TestServlet3 {
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
Expand Down Expand Up @@ -104,6 +109,11 @@ class TestServlet3 {
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)
Expand Down Expand Up @@ -154,6 +164,10 @@ class TestServlet3 {
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
Expand Down
5 changes: 5 additions & 0 deletions instrumentation/servlet/servlet-3.0/javaagent/src/test/groovy/TomcatServlet3Test.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -351,6 +352,7 @@ class TomcatServlet3TestForward 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)
}
}
Expand Down Expand Up @@ -391,6 +393,7 @@ class TomcatServlet3TestInclude 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_PARAMETERS.path, RequestDispatcherServlet.Include)
addServlet(context, "/dispatch" + INDEXED_CHILD.path, RequestDispatcherServlet.Include)
}
}
Expand All @@ -417,6 +420,7 @@ class TomcatServlet3TestDispatchImmediate extends TomcatDispatchTest {
addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + AUTH_REQUIRED.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet3.DispatchImmediate)
addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive)
}
Expand All @@ -439,6 +443,7 @@ class TomcatServlet3TestDispatchAsync extends TomcatDispatchTest {
addServlet(context, "/dispatch" + REDIRECT.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + AUTH_REQUIRED.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + CAPTURE_HEADERS.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + CAPTURE_PARAMETERS.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch" + INDEXED_CHILD.path, TestServlet3.DispatchAsync)
addServlet(context, "/dispatch/recursive", TestServlet3.DispatchRecursive)
}
Expand Down
4 changes: 4 additions & 0 deletions instrumentation/servlet/servlet-5.0/javaagent/build.gradle.kts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,3 +21,7 @@ dependencies {
testLibrary("org.apache.tomcat.embed:tomcat-embed-core:10.0.0")
testLibrary("org.apache.tomcat.embed:tomcat-embed-jasper:10.0.0")
}

tasks.withType<Test>().configureEach {
jvmArgs("-Dotel.instrumentation.servlet.experimental.capture-request-parameters=test-parameter")
}
8 changes: 8 additions & 0 deletions ...c/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/Servlet5Accessor.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -95,6 +96,13 @@ public Iterable<String> getRequestHeaderNames(HttpServletRequest httpServletRequ
return Collections.list(httpServletRequest.getHeaderNames());
}

@Override
public List<String> 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();
Expand Down
8 changes: 8 additions & 0 deletions instrumentation/servlet/servlet-5.0/javaagent/src/test/groovy/AbstractServlet5Test.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -47,6 +49,7 @@ abstract class AbstractServlet5Test<SERVER, CONTEXT> extends HttpServerTest<SERV
addServlet(context, AUTH_REQUIRED.path, servlet)
addServlet(context, INDEXED_CHILD.path, servlet)
addServlet(context, CAPTURE_HEADERS.path, servlet)
addServlet(context, CAPTURE_PARAMETERS.path, servlet)
}

protected ServerEndpoint lastRequest
Expand All @@ -57,6 +60,11 @@ abstract class AbstractServlet5Test<SERVER, CONTEXT> extends HttpServerTest<SERV
super.request(uri, method)
}

@Override
boolean testCapturedRequestParameters() {
true
}

boolean errorEndpointUsesSendError() {
true
}
Expand Down
Loading

0 comments on commit 93f1823

Please sign in to comment.