diff --git a/dd-java-agent/instrumentation/servlet/request-2/request-2.gradle b/dd-java-agent/instrumentation/servlet/request-2/request-2.gradle index 926907ab91..1aaeec4796 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/request-2.gradle +++ b/dd-java-agent/instrumentation/servlet/request-2/request-2.gradle @@ -2,10 +2,10 @@ muzzle { pass { group = "javax.servlet" module = "servlet-api" - versions = "[2.3, 3.0)" + versions = "[2.2, 3.0)" assertInverse = true } - + fail { group = "javax.servlet" module = 'javax.servlet-api' @@ -24,7 +24,7 @@ testSets { } dependencies { - compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.3' + compileOnly group: 'javax.servlet', name: 'servlet-api', version: '2.2' testCompile(project(':dd-java-agent:testing')) { exclude group: 'org.eclipse.jetty', module: 'jetty-server' diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java index 09c9ed89a3..cdd1f02f81 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Advice.java @@ -28,8 +28,8 @@ public class Servlet2Advice { public static AgentScope onEnter( @Advice.This final Object servlet, @Advice.Argument(0) final ServletRequest request, - @Advice.Argument(value = 1, readOnly = false, typing = Assigner.Typing.DYNAMIC) - ServletResponse response) { + @Advice.Argument(value = 1, typing = Assigner.Typing.DYNAMIC) + final ServletResponse response) { final boolean hasServletTrace = request.getAttribute(DD_SPAN_ATTRIBUTE) instanceof AgentSpan; final boolean invalidRequest = !(request instanceof HttpServletRequest); @@ -45,7 +45,8 @@ public class Servlet2Advice { InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) .put((HttpServletResponse) response, httpServletRequest); - response = new StatusSavingHttpServletResponseWrapper((HttpServletResponse) response); + // Default value for checking for uncaught error later + InstrumentationContext.get(ServletResponse.class, Integer.class).put(response, 200); } final AgentSpan.Context extractedContext = propagate().extract(httpServletRequest, GETTER); @@ -89,10 +90,17 @@ public class Servlet2Advice { return; } final AgentSpan span = scope.span(); - DECORATE.onResponse(span, response); + + if (response instanceof HttpServletResponse) { + DECORATE.onResponse( + span, InstrumentationContext.get(ServletResponse.class, Integer.class).get(response)); + } else { + DECORATE.onResponse(span, null); + } + if (throwable != null) { - if (response instanceof StatusSavingHttpServletResponseWrapper - && ((StatusSavingHttpServletResponseWrapper) response).status + if (response instanceof HttpServletResponse + && InstrumentationContext.get(ServletResponse.class, Integer.class).get(response) == HttpServletResponse.SC_OK) { // exception was thrown but status code wasn't set span.setTag(Tags.HTTP_STATUS, 500); diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java index af9cc409d1..dc03c4112e 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Decorator.java @@ -4,11 +4,10 @@ import datadog.trace.bootstrap.instrumentation.api.AgentSpan; import datadog.trace.bootstrap.instrumentation.decorator.HttpServerDecorator; import java.net.URI; import java.net.URISyntaxException; -import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletRequest; public class Servlet2Decorator - extends HttpServerDecorator { + extends HttpServerDecorator { public static final Servlet2Decorator DECORATE = new Servlet2Decorator(); @Override @@ -50,13 +49,8 @@ public class Servlet2Decorator } @Override - protected Integer status(final ServletResponse httpServletResponse) { - if (httpServletResponse instanceof StatusSavingHttpServletResponseWrapper) { - return ((StatusSavingHttpServletResponseWrapper) httpServletResponse).status; - } else { - // HttpServletResponse doesn't have accessor for status code. - return null; - } + protected Integer status(final Integer status) { + return status; } @Override diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Instrumentation.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Instrumentation.java index 62cfbac86a..8ef804361e 100644 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2Instrumentation.java @@ -10,6 +10,7 @@ import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; +import java.util.HashMap; import java.util.Map; import net.bytebuddy.description.method.MethodDescription; import net.bytebuddy.description.type.TypeDescription; @@ -37,16 +38,17 @@ public final class Servlet2Instrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - packageName + ".Servlet2Decorator", - packageName + ".HttpServletRequestExtractAdapter", - packageName + ".StatusSavingHttpServletResponseWrapper", + packageName + ".Servlet2Decorator", packageName + ".HttpServletRequestExtractAdapter", }; } @Override public Map contextStore() { - return singletonMap( + final Map contextStores = new HashMap<>(); + contextStores.put( "javax.servlet.http.HttpServletResponse", "javax.servlet.http.HttpServletRequest"); + contextStores.put("javax.servlet.ServletResponse", Integer.class.getName()); + return contextStores; } /** diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseRedirectAdvice.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseRedirectAdvice.java new file mode 100644 index 0000000000..b02d52f7c8 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseRedirectAdvice.java @@ -0,0 +1,13 @@ +package datadog.trace.instrumentation.servlet2; + +import datadog.trace.bootstrap.InstrumentationContext; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletResponse; +import net.bytebuddy.asm.Advice; + +public class Servlet2ResponseRedirectAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter(@Advice.This final HttpServletResponse response) { + InstrumentationContext.get(ServletResponse.class, Integer.class).put(response, 302); + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusAdvice.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusAdvice.java new file mode 100644 index 0000000000..25b1a21b45 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusAdvice.java @@ -0,0 +1,14 @@ +package datadog.trace.instrumentation.servlet2; + +import datadog.trace.bootstrap.InstrumentationContext; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletResponse; +import net.bytebuddy.asm.Advice; + +public class Servlet2ResponseStatusAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static void onEnter( + @Advice.This final HttpServletResponse response, @Advice.Argument(0) final Integer status) { + InstrumentationContext.get(ServletResponse.class, Integer.class).put(response, status); + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusInstrumentation.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusInstrumentation.java new file mode 100644 index 0000000000..aafc44c478 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusInstrumentation.java @@ -0,0 +1,51 @@ +package datadog.trace.instrumentation.servlet2; + +import static datadog.trace.agent.tooling.ClassLoaderMatcher.hasClassesNamed; +import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.safeHasSuperType; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import java.util.HashMap; +import java.util.Map; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +@AutoService(Instrumenter.class) +public final class Servlet2ResponseStatusInstrumentation extends Instrumenter.Default { + public Servlet2ResponseStatusInstrumentation() { + super("servlet", "servlet-2"); + } + + // this is required to make sure servlet 2 instrumentation won't apply to servlet 3 + @Override + public ElementMatcher classLoaderMatcher() { + return not(hasClassesNamed("javax.servlet.AsyncEvent", "javax.servlet.AsyncListener")); + } + + @Override + public ElementMatcher typeMatcher() { + return safeHasSuperType(named("javax.servlet.http.HttpServletResponse")); + } + + @Override + public Map contextStore() { + return singletonMap("javax.servlet.ServletResponse", Integer.class.getName()); + } + + /** + * Unlike Servlet2Instrumentation it doesn't matter if the HttpServletResponseInstrumentation + * applies first + */ + @Override + public Map, String> transformers() { + final Map, String> transformers = new HashMap<>(); + transformers.put( + named("sendError").or(named("setStatus")), packageName + ".Servlet2ResponseStatusAdvice"); + transformers.put(named("sendRedirect"), packageName + ".Servlet2ResponseRedirectAdvice"); + return transformers; + } +} diff --git a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java deleted file mode 100644 index 971857ca59..0000000000 --- a/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java +++ /dev/null @@ -1,43 +0,0 @@ -package datadog.trace.instrumentation.servlet2; - -import java.io.IOException; -import javax.servlet.http.HttpServletResponse; -import javax.servlet.http.HttpServletResponseWrapper; - -public class StatusSavingHttpServletResponseWrapper extends HttpServletResponseWrapper { - public int status = 200; - - public StatusSavingHttpServletResponseWrapper(final HttpServletResponse response) { - super(response); - } - - @Override - public void sendError(final int status) throws IOException { - this.status = status; - super.sendError(status); - } - - @Override - public void sendError(final int status, final String message) throws IOException { - this.status = status; - super.sendError(status, message); - } - - @Override - public void sendRedirect(final String location) throws IOException { - status = 302; - super.sendRedirect(location); - } - - @Override - public void setStatus(final int status) { - this.status = status; - super.setStatus(status); - } - - @Override - public void setStatus(final int status, final String message) { - this.status = status; - super.setStatus(status, message); - } -}