From 94750a83c9b164d9b787c2358209e12f9392ae18 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 20 Apr 2020 13:48:23 -0400 Subject: [PATCH 1/3] Rework servlet 2 to not wrap HttpServletResponse objects --- .../servlet2/Servlet2Advice.java | 16 +++--- .../servlet2/Servlet2Decorator.java | 10 ++-- .../servlet2/Servlet2Instrumentation.java | 4 +- .../Servlet2ResponseRedirectAdvice.java | 15 ++++++ .../Servlet2ResponseStatusAdvice.java | 16 ++++++ ...Servlet2ResponseStatusInstrumentation.java | 52 +++++++++++++++++++ ...tatusSavingHttpServletResponseWrapper.java | 43 --------------- 7 files changed, 95 insertions(+), 61 deletions(-) create mode 100644 dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseRedirectAdvice.java create mode 100644 dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusAdvice.java create mode 100644 dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusInstrumentation.java delete mode 100644 dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/StatusSavingHttpServletResponseWrapper.java 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..0486659ff9 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); @@ -44,8 +44,7 @@ public class Servlet2Advice { // For use by HttpServletResponseInstrumentation: InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) .put((HttpServletResponse) response, httpServletRequest); - - response = new StatusSavingHttpServletResponseWrapper((HttpServletResponse) response); + httpServletRequest.setAttribute("dd.http-status", HttpServletResponse.SC_OK); } final AgentSpan.Context extractedContext = propagate().extract(httpServletRequest, GETTER); @@ -73,7 +72,6 @@ public class Servlet2Advice { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Argument(0) final ServletRequest request, - @Advice.Argument(1) final ServletResponse response, @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { // Set user.principal regardless of who created this span. @@ -89,11 +87,11 @@ public class Servlet2Advice { return; } final AgentSpan span = scope.span(); - DECORATE.onResponse(span, response); + if (request instanceof HttpServletRequest) { + DECORATE.onResponse(span, (HttpServletRequest) request); + } if (throwable != null) { - if (response instanceof StatusSavingHttpServletResponseWrapper - && ((StatusSavingHttpServletResponseWrapper) response).status - == HttpServletResponse.SC_OK) { + if ((Integer) request.getAttribute("dd.http-status") == 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..2256d84767 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,11 +49,10 @@ public class Servlet2Decorator } @Override - protected Integer status(final ServletResponse httpServletResponse) { - if (httpServletResponse instanceof StatusSavingHttpServletResponseWrapper) { - return ((StatusSavingHttpServletResponseWrapper) httpServletResponse).status; + protected Integer status(final HttpServletRequest httpServletRequest) { + if (httpServletRequest != null) { + return (Integer) httpServletRequest.getAttribute("dd.http-status"); } else { - // HttpServletResponse doesn't have accessor for status code. return null; } } 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..4d970e5a27 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 @@ -37,9 +37,7 @@ public final class Servlet2Instrumentation extends Instrumenter.Default { @Override public String[] helperClassNames() { return new String[] { - packageName + ".Servlet2Decorator", - packageName + ".HttpServletRequestExtractAdapter", - packageName + ".StatusSavingHttpServletResponseWrapper", + packageName + ".Servlet2Decorator", packageName + ".HttpServletRequestExtractAdapter", }; } 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..c546ae888f --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseRedirectAdvice.java @@ -0,0 +1,15 @@ +package datadog.trace.instrumentation.servlet2; + +import datadog.trace.bootstrap.InstrumentationContext; +import javax.servlet.http.HttpServletRequest; +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(HttpServletResponse.class, HttpServletRequest.class) + .get(response) + .setAttribute("status-code", 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..d834974bc6 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusAdvice.java @@ -0,0 +1,16 @@ +package datadog.trace.instrumentation.servlet2; + +import datadog.trace.bootstrap.InstrumentationContext; +import javax.servlet.http.HttpServletRequest; +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(HttpServletResponse.class, HttpServletRequest.class) + .get(response) + .setAttribute("dd.http-status", 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..57322ec212 --- /dev/null +++ b/dd-java-agent/instrumentation/servlet/request-2/src/main/java/datadog/trace/instrumentation/servlet2/Servlet2ResponseStatusInstrumentation.java @@ -0,0 +1,52 @@ +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.http.HttpServletResponse", "javax.servlet.http.HttpServletRequest"); + } + + /** + * 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); - } -} From dc43f3f54c5b0ed2f074d2052549f87047bd3b7a Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 20 Apr 2020 14:57:34 -0400 Subject: [PATCH 2/3] We don't depend on a 2.3 type anymore --- .../instrumentation/servlet/request-2/request-2.gradle | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) 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' From aec2fad9db6f78775f7e10ed17d96b87bcd7ce67 Mon Sep 17 00:00:00 2001 From: Brian Devins-Suresh Date: Mon, 20 Apr 2020 18:19:42 -0400 Subject: [PATCH 3/3] Use the parent interface --- .../servlet2/Servlet2Advice.java | 18 ++++++++++++++---- .../servlet2/Servlet2Decorator.java | 10 +++------- .../servlet2/Servlet2Instrumentation.java | 6 +++++- .../Servlet2ResponseRedirectAdvice.java | 6 ++---- .../servlet2/Servlet2ResponseStatusAdvice.java | 6 ++---- .../Servlet2ResponseStatusInstrumentation.java | 3 +-- 6 files changed, 27 insertions(+), 22 deletions(-) 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 0486659ff9..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 @@ -44,7 +44,9 @@ public class Servlet2Advice { // For use by HttpServletResponseInstrumentation: InstrumentationContext.get(HttpServletResponse.class, HttpServletRequest.class) .put((HttpServletResponse) response, httpServletRequest); - httpServletRequest.setAttribute("dd.http-status", HttpServletResponse.SC_OK); + + // 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); @@ -72,6 +74,7 @@ public class Servlet2Advice { @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) public static void stopSpan( @Advice.Argument(0) final ServletRequest request, + @Advice.Argument(1) final ServletResponse response, @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { // Set user.principal regardless of who created this span. @@ -87,11 +90,18 @@ public class Servlet2Advice { return; } final AgentSpan span = scope.span(); - if (request instanceof HttpServletRequest) { - DECORATE.onResponse(span, (HttpServletRequest) request); + + if (response instanceof HttpServletResponse) { + DECORATE.onResponse( + span, InstrumentationContext.get(ServletResponse.class, Integer.class).get(response)); + } else { + DECORATE.onResponse(span, null); } + if (throwable != null) { - if ((Integer) request.getAttribute("dd.http-status") == HttpServletResponse.SC_OK) { + 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 2256d84767..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 @@ -7,7 +7,7 @@ import java.net.URISyntaxException; import javax.servlet.http.HttpServletRequest; public class Servlet2Decorator - extends HttpServerDecorator { + extends HttpServerDecorator { public static final Servlet2Decorator DECORATE = new Servlet2Decorator(); @Override @@ -49,12 +49,8 @@ public class Servlet2Decorator } @Override - protected Integer status(final HttpServletRequest httpServletRequest) { - if (httpServletRequest != null) { - return (Integer) httpServletRequest.getAttribute("dd.http-status"); - } else { - 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 4d970e5a27..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; @@ -43,8 +44,11 @@ public final class Servlet2Instrumentation extends Instrumenter.Default { @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 index c546ae888f..b02d52f7c8 100644 --- 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 @@ -1,15 +1,13 @@ package datadog.trace.instrumentation.servlet2; import datadog.trace.bootstrap.InstrumentationContext; -import javax.servlet.http.HttpServletRequest; +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(HttpServletResponse.class, HttpServletRequest.class) - .get(response) - .setAttribute("status-code", 302); + 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 index d834974bc6..25b1a21b45 100644 --- 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 @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.servlet2; import datadog.trace.bootstrap.InstrumentationContext; -import javax.servlet.http.HttpServletRequest; +import javax.servlet.ServletResponse; import javax.servlet.http.HttpServletResponse; import net.bytebuddy.asm.Advice; @@ -9,8 +9,6 @@ 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(HttpServletResponse.class, HttpServletRequest.class) - .get(response) - .setAttribute("dd.http-status", 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 index 57322ec212..aafc44c478 100644 --- 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 @@ -33,8 +33,7 @@ public final class Servlet2ResponseStatusInstrumentation extends Instrumenter.De @Override public Map contextStore() { - return singletonMap( - "javax.servlet.http.HttpServletResponse", "javax.servlet.http.HttpServletRequest"); + return singletonMap("javax.servlet.ServletResponse", Integer.class.getName()); } /**