open-telemetry
/
opentelemetry-java-instrumentation
mirror of https://github.com/open-telemetry/opentelemetry-java-instrumentation.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Remove RequestDispatcherAdvice (#2942)

This commit is contained in:
Trask Stalnaker 2021-05-12 21:37:48 -07:00 committed by GitHub
parent 02ac89c549
commit f413e5a3aa
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
8 changed files with 4 additions and 573 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/JakartaServletInstrumentationModule.java
View File

@ -9,7 +9,6 @@ import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.async.AsyncContextInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.service.ServletAndFilterInstrumentation;
import java.util.Arrays;
@ -34,9 +33,7 @@ public class JakartaServletInstrumentationModule extends InstrumentationModule {
adviceClassName(".service.JakartaServletInitAdvice"),
adviceClassName(".service.JakartaServletFilterInitAdvice")),
new HttpServletResponseInstrumentation(
BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")),
new RequestDispatcherInstrumentation(
BASE_PACKAGE, adviceClassName(".dispatcher.RequestDispatcherAdvice")));
BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")));
}
private static String adviceClassName(String suffix) {

62
instrumentation/servlet/servlet-5.0/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/v5_0/dispatcher/RequestDispatcherAdvice.java
View File

@ -1,62 +0,0 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet.v5_0.dispatcher;
import static io.opentelemetry.instrumentation.api.tracer.HttpServerTracer.CONTEXT_ATTRIBUTE;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherAdviceHelper;
import jakarta.servlet.RequestDispatcher;
import jakarta.servlet.ServletRequest;
import java.lang.reflect.Method;
import net.bytebuddy.asm.Advice;
public class RequestDispatcherAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void start(
@Advice.Origin Method method,
@Advice.This RequestDispatcher dispatcher,
@Advice.Local("otelRequestContext") Context requestContext,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope,
@Advice.Argument(0) ServletRequest request) {
Context currentContext = Java8BytecodeBridge.currentContext();
Object requestContextAttr = request.getAttribute(CONTEXT_ATTRIBUTE);
requestContext = requestContextAttr instanceof Context ? (Context) requestContextAttr : null;
context = RequestDispatcherAdviceHelper.getStartParentContext(currentContext, requestContext);
if (context == null) {
return;
}
// this tells the dispatched servlet to use the current span as the parent for its work
request.setAttribute(CONTEXT_ATTRIBUTE, context);
// TODO (trask) do we need this, since doing manual propagation above?
scope = context.makeCurrent();
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void stop(
@Advice.Argument(0) ServletRequest request,
@Advice.Local("otelRequestContext") Context requestContext,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope,
@Advice.Thrown Throwable throwable) {
scope.close();
if (requestContext != null) {
// restore the original request context
request.setAttribute(CONTEXT_ATTRIBUTE, requestContext);
}
}
}

52
instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherAdviceHelper.java
View File

@ -1,52 +0,0 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher;
import io.opentelemetry.api.trace.Span;
import io.opentelemetry.api.trace.SpanContext;
import io.opentelemetry.context.Context;
import io.opentelemetry.instrumentation.api.tracer.HttpServerTracer;
import org.checkerframework.checker.nullness.qual.Nullable;
public class RequestDispatcherAdviceHelper {
/**
* Determines if the advice for {@link RequestDispatcherInstrumentation} should create a new span
* and provides the context in which that span should be created.
*
* @param requestContext Value of the {@link HttpServerTracer#CONTEXT_ATTRIBUTE} attribute of the
* servlet request.
* @return The context in which the advice should create the dispatcher span in. Returns <code>
* null</code> in case a new span should not be created.
*/
// TODO (trask) do we need to guard against context leak here?
// this could be simplified by always using currentContext, only falling back to requestContext
// if currentContext does not have a valid span
public static @Nullable Context getStartParentContext(
Context currentContext, @Nullable Context requestContext) {
Span currentSpan = Span.fromContext(currentContext);
SpanContext currentSpanContext = currentSpan.getSpanContext();
if (!currentSpanContext.isValid()) {
return requestContext;
}
if (requestContext == null) {
return currentContext;
}
// at this point: currentContext has a valid span and requestContext is not null
Span requestSpan = Span.fromContext(requestContext);
if (requestSpan.getSpanContext().getTraceId().equals(currentSpanContext.getTraceId())) {
// currentContext is part of the same trace, so return it
return currentContext;
}
// currentContext is part of a different trace, so lets ignore it.
// This can happen with the way Tomcat does error handling.
return requestContext;
}
}

52
instrumentation/servlet/servlet-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/common/dispatcher/RequestDispatcherInstrumentation.java
View File

@ -1,52 +0,0 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher;
import static io.opentelemetry.javaagent.extension.matcher.AgentElementMatchers.implementsInterface;
import static io.opentelemetry.javaagent.extension.matcher.ClassLoaderMatcher.hasClassesNamed;
import static io.opentelemetry.javaagent.extension.matcher.NameMatchers.namedOneOf;
import static java.util.Collections.singletonMap;
import static net.bytebuddy.matcher.ElementMatchers.isPublic;
import static net.bytebuddy.matcher.ElementMatchers.named;
import static net.bytebuddy.matcher.ElementMatchers.takesArgument;
import static net.bytebuddy.matcher.ElementMatchers.takesArguments;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import java.util.Map;
import net.bytebuddy.description.method.MethodDescription;
import net.bytebuddy.description.type.TypeDescription;
import net.bytebuddy.matcher.ElementMatcher;
public class RequestDispatcherInstrumentation implements TypeInstrumentation {
private final String basePackageName;
private final String adviceClassName;
public RequestDispatcherInstrumentation(String basePackageName, String adviceClassName) {
this.basePackageName = basePackageName;
this.adviceClassName = adviceClassName;
}
@Override
public ElementMatcher<ClassLoader> classLoaderOptimization() {
return hasClassesNamed(basePackageName + ".RequestDispatcher");
}
@Override
public ElementMatcher<TypeDescription> typeMatcher() {
return implementsInterface(named(basePackageName + ".RequestDispatcher"));
}
@Override
public Map<? extends ElementMatcher<? super MethodDescription>, String> transformers() {
return singletonMap(
namedOneOf("forward", "include")
.and(takesArguments(2))
.and(takesArgument(0, named(basePackageName + ".ServletRequest")))
.and(takesArgument(1, named(basePackageName + ".ServletResponse")))
.and(isPublic()),
adviceClassName);
}
}

9
instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/JavaxServletInstrumentationModule.java
View File

@ -8,9 +8,8 @@ package io.opentelemetry.javaagent.instrumentation.servlet.javax;
import com.google.auto.service.AutoService;
import io.opentelemetry.javaagent.extension.instrumentation.InstrumentationModule;
import io.opentelemetry.javaagent.extension.instrumentation.TypeInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherInstrumentation;
import io.opentelemetry.javaagent.instrumentation.servlet.common.response.HttpServletResponseInstrumentation;
import java.util.Arrays;
import java.util.Collections;
import java.util.List;
@AutoService(InstrumentationModule.class)
@ -23,11 +22,9 @@ public class JavaxServletInstrumentationModule extends InstrumentationModule {
@Override
public List<TypeInstrumentation> typeInstrumentations() {
return Arrays.asList(
return Collections.singletonList(
new HttpServletResponseInstrumentation(
BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")),
new RequestDispatcherInstrumentation(
BASE_PACKAGE, adviceClassName(".dispatcher.RequestDispatcherAdvice")));
BASE_PACKAGE, adviceClassName(".response.ResponseSendAdvice")));
}
private static String adviceClassName(String suffix) {

60
instrumentation/servlet/servlet-javax-common/javaagent/src/main/java/io/opentelemetry/javaagent/instrumentation/servlet/javax/dispatcher/RequestDispatcherAdvice.java
View File

@ -1,60 +0,0 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
package io.opentelemetry.javaagent.instrumentation.servlet.javax.dispatcher;
import static io.opentelemetry.instrumentation.api.tracer.HttpServerTracer.CONTEXT_ATTRIBUTE;
import io.opentelemetry.context.Context;
import io.opentelemetry.context.Scope;
import io.opentelemetry.javaagent.instrumentation.api.Java8BytecodeBridge;
import io.opentelemetry.javaagent.instrumentation.servlet.common.dispatcher.RequestDispatcherAdviceHelper;
import java.lang.reflect.Method;
import javax.servlet.ServletRequest;
import net.bytebuddy.asm.Advice;
public class RequestDispatcherAdvice {
@Advice.OnMethodEnter(suppress = Throwable.class)
public static void start(
@Advice.Origin Method method,
@Advice.Local("otelRequestContext") Context requestContext,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope,
@Advice.Argument(0) ServletRequest request) {
Context currentContext = Java8BytecodeBridge.currentContext();
Object requestContextAttr = request.getAttribute(CONTEXT_ATTRIBUTE);
requestContext = requestContextAttr instanceof Context ? (Context) requestContextAttr : null;
context = RequestDispatcherAdviceHelper.getStartParentContext(currentContext, requestContext);
if (context == null) {
return;
}
// this tells the dispatched servlet to use the current span as the parent for its work
request.setAttribute(CONTEXT_ATTRIBUTE, context);
// TODO (trask) do we need this, since doing manual propagation above?
scope = context.makeCurrent();
}
@Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
public static void stop(
@Advice.Argument(0) ServletRequest request,
@Advice.Local("otelRequestContext") Context requestContext,
@Advice.Local("otelContext") Context context,
@Advice.Local("otelScope") Scope scope,
@Advice.Thrown Throwable throwable) {
scope.close();
if (requestContext != null) {
// restore the original request context
request.setAttribute(CONTEXT_ATTRIBUTE, requestContext);
}
}
}

128
instrumentation/servlet/servlet-javax-common/javaagent/src/test/groovy/RequestDispatcherTest.groovy
View File

@ -1,128 +0,0 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
import static io.opentelemetry.instrumentation.test.utils.TraceUtils.basicSpan
import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace
import io.opentelemetry.context.Context
import io.opentelemetry.instrumentation.test.AgentInstrumentationSpecification
import javax.servlet.ServletException
import javax.servlet.http.HttpServletRequest
import javax.servlet.http.HttpServletResponse
class RequestDispatcherTest extends AgentInstrumentationSpecification {
def request = Mock(HttpServletRequest)
def response = Mock(HttpServletResponse)
def dispatcher = new RequestDispatcherUtils(request, response)
def "test dispatch no-parent"() {
when:
dispatcher.forward("")
dispatcher.include("")
then:
assertTraces(2) {
traces.sort(orderByRootSpanName("forward-child", "include-child"))
trace(0, 1) {
basicSpan(it, 0, "forward-child")
}
trace(1, 1) {
basicSpan(it, 0, "include-child")
}
}
}
def "test dispatcher #method with parent"() {
when:
runUnderTrace("parent") {
dispatcher."$method"(target)
}
then:
assertTraces(1) {
trace(0, 2) {
basicSpan(it, 0, "parent")
basicSpan(it, 1, "$operation-child", span(0))
}
}
where:
operation | method
"forward" | "forward"
"forward" | "forwardNamed"
"include" | "include"
"include" | "includeNamed"
target = "test-$method"
}
def "test dispatcher #method with parent from request attribute"() {
setup:
Context context
runUnderTrace("parent") {
context = Context.current()
}
when:
runUnderTrace("notParent") {
context.makeCurrent().withCloseable {
dispatcher."$method"(target)
}
}
then:
assertTraces(2) {
traces.sort(orderByRootSpanName("parent", "notParent"))
trace(0, 2) {
basicSpan(it, 0, "parent")
basicSpan(it, 1, "$operation-child", span(0))
}
trace(1, 1) {
basicSpan(it, 0, "notParent")
}
}
where:
operation | method
"forward" | "forward"
"forward" | "forwardNamed"
"include" | "include"
"include" | "includeNamed"
target = "test-$method"
}
def "test dispatcher #method exception"() {
setup:
def ex = new ServletException("some error")
def dispatcher = new RequestDispatcherUtils(request, response, ex)
when:
runUnderTrace("parent") {
dispatcher."$method"(target)
}
then:
def th = thrown(ServletException)
th == ex
assertTraces(1) {
trace(0, 2) {
basicSpan(it, 0, "parent", null, ex)
basicSpan(it, 1, "$operation-child", span(0))
}
}
where:
operation | method
"forward" | "forward"
"forward" | "forwardNamed"
"include" | "include"
"include" | "includeNamed"
target = "test-$method"
}
}

209
instrumentation/servlet/servlet-javax-common/javaagent/src/test/java/RequestDispatcherUtils.java
View File

@ -1,209 +0,0 @@
/*
* Copyright The OpenTelemetry Authors
* SPDX-License-Identifier: Apache-2.0
*/
import static io.opentelemetry.instrumentation.test.utils.TraceUtils.runUnderTrace;
import java.io.IOException;
import java.io.InputStream;
import java.net.URL;
import java.util.Enumeration;
import java.util.Set;
import java.util.concurrent.Callable;
import javax.servlet.RequestDispatcher;
import javax.servlet.Servlet;
import javax.servlet.ServletContext;
import javax.servlet.ServletException;
import javax.servlet.ServletRequest;
import javax.servlet.ServletResponse;
public class RequestDispatcherUtils {
private final ServletRequest req;
private final ServletResponse resp;
private final ServletException toThrow;
public RequestDispatcherUtils(ServletRequest req, ServletResponse resp) {
this.req = req;
this.resp = resp;
toThrow = null;
}
public RequestDispatcherUtils(
ServletRequest req, ServletResponse resp, ServletException toThrow) {
this.req = req;
this.resp = resp;
this.toThrow = toThrow;
}
/* RequestDispatcher can't be visible to groovy otherwise things break, so everything is
* encapsulated in here where groovy doesn't need to access it.
*/
void forward(String target) throws ServletException, IOException {
new TestContext().getRequestDispatcher(target).forward(req, resp);
}
void include(String target) throws ServletException, IOException {
new TestContext().getRequestDispatcher(target).include(req, resp);
}
void forwardNamed(String target) throws ServletException, IOException {
new TestContext().getNamedDispatcher(target).forward(req, resp);
}
void includeNamed(String target) throws ServletException, IOException {
new TestContext().getNamedDispatcher(target).include(req, resp);
}
class TestContext implements ServletContext {
@Override
public String getContextPath() {
return null;
}
@Override
public ServletContext getContext(String s) {
return null;
}
@Override
public int getMajorVersion() {
return 0;
}
@Override
public int getMinorVersion() {
return 0;
}
@Override
public String getMimeType(String s) {
return null;
}
@Override
public Set getResourcePaths(String s) {
return null;
}
@Override
public URL getResource(String s) {
return null;
}
@Override
public InputStream getResourceAsStream(String s) {
return null;
}
@Override
public RequestDispatcher getRequestDispatcher(String s) {
return new TestDispatcher();
}
@Override
public RequestDispatcher getNamedDispatcher(String s) {
return new TestDispatcher();
}
@Override
public Servlet getServlet(String s) {
return null;
}
@Override
public Enumeration getServlets() {
return null;
}
@Override
public Enumeration getServletNames() {
return null;
}
@Override
public void log(String s) {}
@Override
public void log(Exception e, String s) {}
@Override
public void log(String s, Throwable throwable) {}
@Override
public String getRealPath(String s) {
return null;
}
@Override
public String getServerInfo() {
return null;
}
@Override
public String getInitParameter(String s) {
return null;
}
@Override
public Enumeration getInitParameterNames() {
return null;
}
@Override
public Object getAttribute(String s) {
return null;
}
@Override
public Enumeration getAttributeNames() {
return null;
}
@Override
public void setAttribute(String s, Object o) {}
@Override
public void removeAttribute(String s) {}
@Override
public String getServletContextName() {
return null;
}
}
class TestDispatcher implements RequestDispatcher {
@Override
public void forward(ServletRequest servletRequest, ServletResponse servletResponse)
throws ServletException {
runUnderTrace(
"forward-child",
new Callable<Object>() {
@Override
public Object call() {
return null;
}
});
if (toThrow != null) {
throw toThrow;
}
}
@Override
public void include(ServletRequest servletRequest, ServletResponse servletResponse)
throws ServletException {
runUnderTrace(
"include-child",
new Callable<Object>() {
@Override
public Object call() {
return null;
}
});
if (toThrow != null) {
throw toThrow;
}
}
}
}