From a5c54c54f712794ba5806499d7903bfd7b8525cc Mon Sep 17 00:00:00 2001 From: Trask Stalnaker Date: Sat, 19 Oct 2019 11:57:47 -0700 Subject: [PATCH] Update servlet-3 to new agent api --- .../servlet3/AsyncContextInstrumentation.java | 16 +-- .../servlet3/FilterChain3Instrumentation.java | 1 - .../servlet3/HttpServlet3Instrumentation.java | 1 - .../HttpServletRequestExtractAdapter.java | 109 +++--------------- .../HttpServletRequestInjectAdapter.java | 24 ++-- .../servlet3/Servlet3Advice.java | 49 +++----- .../servlet3/Servlet3Decorator.java | 6 +- .../servlet3/TagSettingAsyncListener.java | 10 +- 8 files changed, 56 insertions(+), 160 deletions(-) diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java index 67c714240c..e9da26e9ba 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/AsyncContextInstrumentation.java @@ -2,6 +2,8 @@ package datadog.trace.instrumentation.servlet3; import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.propagate; +import static datadog.trace.instrumentation.servlet3.HttpServletRequestInjectAdapter.SETTER; import static java.util.Collections.singletonMap; import static net.bytebuddy.matcher.ElementMatchers.isInterface; import static net.bytebuddy.matcher.ElementMatchers.isMethod; @@ -12,9 +14,7 @@ import static net.bytebuddy.matcher.ElementMatchers.not; import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.bootstrap.CallDepthThreadLocalMap; -import io.opentracing.Span; -import io.opentracing.propagation.Format; -import io.opentracing.util.GlobalTracer; +import datadog.trace.instrumentation.api.AgentSpan; import java.util.Map; import javax.servlet.AsyncContext; import javax.servlet.ServletRequest; @@ -64,17 +64,13 @@ public final class AsyncContextInstrumentation extends Instrumenter.Default { final ServletRequest request = context.getRequest(); final Object spanAttr = request.getAttribute(DD_SPAN_ATTRIBUTE); - if (spanAttr instanceof Span) { + if (spanAttr instanceof AgentSpan) { request.removeAttribute(DD_SPAN_ATTRIBUTE); - final Span span = (Span) spanAttr; + final AgentSpan span = (AgentSpan) spanAttr; // Override propagation headers by injecting attributes from the current span // into the new request if (request instanceof HttpServletRequest) { - GlobalTracer.get() - .inject( - span.context(), - Format.Builtin.TEXT_MAP, - new HttpServletRequestInjectAdapter((HttpServletRequest) request)); + propagate().inject(span, (HttpServletRequest) request, SETTER); } final String path; if (args.length == 1 && args[0] instanceof String) { diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java index a559c7b155..cf22f3d02a 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/FilterChain3Instrumentation.java @@ -28,7 +28,6 @@ public final class FilterChain3Instrumentation extends Instrumenter.Default { "datadog.trace.agent.decorator.ServerDecorator", "datadog.trace.agent.decorator.HttpServerDecorator", packageName + ".HttpServletRequestExtractAdapter", - packageName + ".HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", packageName + ".Servlet3Decorator", packageName + ".TagSettingAsyncListener" }; diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java index f3e7967ed5..f4cb088ee7 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServlet3Instrumentation.java @@ -28,7 +28,6 @@ public final class HttpServlet3Instrumentation extends Instrumenter.Default { "datadog.trace.agent.decorator.ServerDecorator", "datadog.trace.agent.decorator.HttpServerDecorator", packageName + ".HttpServletRequestExtractAdapter", - packageName + ".HttpServletRequestExtractAdapter$MultivaluedMapFlatIterator", packageName + ".Servlet3Decorator", packageName + ".TagSettingAsyncListener", }; diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java index e50ad6f693..e8b1fff393 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestExtractAdapter.java @@ -1,111 +1,34 @@ package datadog.trace.instrumentation.servlet3; -import io.opentracing.propagation.TextMap; -import java.util.AbstractMap; +import datadog.trace.instrumentation.api.AgentPropagation; import java.util.ArrayList; import java.util.Collections; -import java.util.Enumeration; -import java.util.HashMap; -import java.util.Iterator; import java.util.List; -import java.util.Map; -import java.util.Set; import javax.servlet.http.HttpServletRequest; -/** - * Tracer extract adapter for {@link HttpServletRequest}. - * - * @author Pavol Loffay - */ -// FIXME: This code is duplicated in several places. Extract to a common dependency. -public class HttpServletRequestExtractAdapter implements TextMap { +public class HttpServletRequestExtractAdapter + implements AgentPropagation.Getter { - private final Map> headers; + public static final HttpServletRequestExtractAdapter GETTER = + new HttpServletRequestExtractAdapter(); - public HttpServletRequestExtractAdapter(final HttpServletRequest httpServletRequest) { - headers = servletHeadersToMultiMap(httpServletRequest); + @Override + public List keys(final HttpServletRequest carrier) { + final ArrayList keys = Collections.list(carrier.getHeaderNames()); + keys.addAll(Collections.list(carrier.getAttributeNames())); + return keys; } @Override - public Iterator> iterator() { - return new MultivaluedMapFlatIterator<>(headers.entrySet()); - } - - @Override - public void put(final String key, final String value) { - throw new UnsupportedOperationException("This class should be used only with Tracer.inject()!"); - } - - protected Map> servletHeadersToMultiMap( - final HttpServletRequest httpServletRequest) { - final Map> headersResult = new HashMap<>(); - - final Enumeration headerNamesIt = httpServletRequest.getHeaderNames(); - while (headerNamesIt.hasMoreElements()) { - final String headerName = headerNamesIt.nextElement(); - - final Enumeration valuesIt = httpServletRequest.getHeaders(headerName); - final List valuesList = new ArrayList<>(1); - while (valuesIt.hasMoreElements()) { - valuesList.add(valuesIt.nextElement()); - } - - headersResult.put(headerName, valuesList); - } - + public String get(final HttpServletRequest carrier, final String key) { /* * Read from the attributes and override the headers. - * This is used by HttpServletRequestInjectAdapter when a request is async-dispatched. + * This is used by HttpServletRequestSetter when a request is async-dispatched. */ - final Enumeration attributeNamesIt = httpServletRequest.getAttributeNames(); - while (attributeNamesIt.hasMoreElements()) { - final String attributeName = attributeNamesIt.nextElement(); - - final Object valuesIt = httpServletRequest.getAttribute(attributeName); - if (valuesIt instanceof String) { - headersResult.put(attributeName, Collections.singletonList((String) valuesIt)); - } - } - - return headersResult; - } - - public static final class MultivaluedMapFlatIterator implements Iterator> { - - private final Iterator>> mapIterator; - private Map.Entry> mapEntry; - private Iterator listIterator; - - public MultivaluedMapFlatIterator(final Set>> multiValuesEntrySet) { - mapIterator = multiValuesEntrySet.iterator(); - } - - @Override - public boolean hasNext() { - if (listIterator != null && listIterator.hasNext()) { - return true; - } - - return mapIterator.hasNext(); - } - - @Override - public Map.Entry next() { - if (mapEntry == null || (!listIterator.hasNext() && mapIterator.hasNext())) { - mapEntry = mapIterator.next(); - listIterator = mapEntry.getValue().iterator(); - } - - if (listIterator.hasNext()) { - return new AbstractMap.SimpleImmutableEntry<>(mapEntry.getKey(), listIterator.next()); - } else { - return new AbstractMap.SimpleImmutableEntry<>(mapEntry.getKey(), null); - } - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); + final Object attribute = carrier.getAttribute(key); + if (attribute instanceof String) { + return (String) attribute; } + return carrier.getHeader(key); } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java index 967243bbf2..c04aa0a144 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/HttpServletRequestInjectAdapter.java @@ -1,27 +1,17 @@ package datadog.trace.instrumentation.servlet3; -import io.opentracing.propagation.TextMap; -import java.util.Iterator; -import java.util.Map; +import datadog.trace.instrumentation.api.AgentPropagation; import javax.servlet.http.HttpServletRequest; /** Inject into request attributes since the request headers can't be modified. */ -public class HttpServletRequestInjectAdapter implements TextMap { +public class HttpServletRequestInjectAdapter + implements AgentPropagation.Setter { - private final HttpServletRequest httpServletRequest; - - public HttpServletRequestInjectAdapter(final HttpServletRequest httpServletRequest) { - this.httpServletRequest = httpServletRequest; - } + public static final HttpServletRequestInjectAdapter SETTER = + new HttpServletRequestInjectAdapter(); @Override - public Iterator> iterator() { - throw new UnsupportedOperationException( - "This class should be used only with Tracer.extract()!"); - } - - @Override - public void put(final String key, final String value) { - httpServletRequest.setAttribute(key, value); + public void set(final HttpServletRequest carrier, final String key, final String value) { + carrier.setAttribute(key, value); } } diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java index 7cc97a8a32..c8474f632d 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Advice.java @@ -1,16 +1,16 @@ package datadog.trace.instrumentation.servlet3; import static datadog.trace.agent.decorator.HttpServerDecorator.DD_SPAN_ATTRIBUTE; +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.propagate; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.servlet3.HttpServletRequestExtractAdapter.GETTER; import static datadog.trace.instrumentation.servlet3.Servlet3Decorator.DECORATE; import datadog.trace.api.DDTags; -import datadog.trace.context.TraceScope; -import io.opentracing.Scope; -import io.opentracing.Span; -import io.opentracing.SpanContext; -import io.opentracing.propagation.Format; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; import io.opentracing.tag.Tags; -import io.opentracing.util.GlobalTracer; import java.security.Principal; import java.util.concurrent.atomic.AtomicBoolean; import javax.servlet.ServletRequest; @@ -22,7 +22,7 @@ import net.bytebuddy.asm.Advice; public class Servlet3Advice { @Advice.OnMethodEnter(suppress = Throwable.class) - public static Scope startSpan( + public static AgentScope onEnter( @Advice.This final Object servlet, @Advice.Argument(0) final ServletRequest req) { final Object spanAttr = req.getAttribute(DD_SPAN_ATTRIBUTE); if (!(req instanceof HttpServletRequest) || spanAttr != null) { @@ -31,30 +31,19 @@ public class Servlet3Advice { } final HttpServletRequest httpServletRequest = (HttpServletRequest) req; - final SpanContext extractedContext = - GlobalTracer.get() - .extract( - Format.Builtin.HTTP_HEADERS, - new HttpServletRequestExtractAdapter(httpServletRequest)); + final AgentSpan.Context extractedContext = propagate().extract(httpServletRequest, GETTER); - final Scope scope = - GlobalTracer.get() - .buildSpan("servlet.request") - .ignoreActiveSpan() - .asChildOf(extractedContext) - .withTag("span.origin.type", servlet.getClass().getName()) - .startActive(false); - - final Span span = scope.span(); + final AgentSpan span = + startSpan("servlet.request", extractedContext) + .setTag("span.origin.type", servlet.getClass().getName()); DECORATE.afterStart(span); DECORATE.onConnection(span, httpServletRequest); DECORATE.onRequest(span, httpServletRequest); - if (scope instanceof TraceScope) { - ((TraceScope) scope).setAsyncPropagation(true); - } - req.setAttribute(DD_SPAN_ATTRIBUTE, span); + + final AgentScope scope = activateSpan(span, false); + scope.setAsyncPropagation(true); return scope; } @@ -62,14 +51,14 @@ public class Servlet3Advice { public static void stopSpan( @Advice.Argument(0) final ServletRequest request, @Advice.Argument(1) final ServletResponse response, - @Advice.Enter final Scope scope, + @Advice.Enter final AgentScope scope, @Advice.Thrown final Throwable throwable) { // Set user.principal regardless of who created this span. final Object spanAttr = request.getAttribute(DD_SPAN_ATTRIBUTE); - if (spanAttr instanceof Span && request instanceof HttpServletRequest) { + if (spanAttr instanceof AgentSpan && request instanceof HttpServletRequest) { final Principal principal = ((HttpServletRequest) request).getUserPrincipal(); if (principal != null) { - ((Span) spanAttr).setTag(DDTags.USER_NAME, principal.getName()); + ((AgentSpan) spanAttr).setTag(DDTags.USER_NAME, principal.getName()); } } @@ -77,13 +66,13 @@ public class Servlet3Advice { if (request instanceof HttpServletRequest && response instanceof HttpServletResponse) { final HttpServletRequest req = (HttpServletRequest) request; final HttpServletResponse resp = (HttpServletResponse) response; - final Span span = scope.span(); + final AgentSpan span = scope.span(); if (throwable != null) { DECORATE.onResponse(span, resp); if (resp.getStatus() == HttpServletResponse.SC_OK) { // exception is thrown in filter chain, but status code is incorrect - Tags.HTTP_STATUS.set(span, 500); + span.setTag(Tags.HTTP_STATUS.getKey(), 500); } DECORATE.onError(span, throwable); DECORATE.beforeFinish(span); diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java index 7ac849877b..1ce2a8873e 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/Servlet3Decorator.java @@ -1,7 +1,7 @@ package datadog.trace.instrumentation.servlet3; import datadog.trace.agent.decorator.HttpServerDecorator; -import io.opentracing.Span; +import datadog.trace.instrumentation.api.AgentSpan; import java.net.URI; import java.net.URISyntaxException; import javax.servlet.ServletException; @@ -53,7 +53,7 @@ public class Servlet3Decorator } @Override - public Span onRequest(final Span span, final HttpServletRequest request) { + public AgentSpan onRequest(final AgentSpan span, final HttpServletRequest request) { assert span != null; if (request != null) { span.setTag("servlet.context", request.getContextPath()); @@ -62,7 +62,7 @@ public class Servlet3Decorator } @Override - public Span onError(final Span span, final Throwable throwable) { + public AgentSpan onError(final AgentSpan span, final Throwable throwable) { if (throwable instanceof ServletException && throwable.getCause() != null) { super.onError(span, throwable.getCause()); } else { diff --git a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java index 3a070ae7c5..c278e36f5f 100644 --- a/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java +++ b/dd-java-agent/instrumentation/servlet-3/src/main/java/datadog/trace/instrumentation/servlet3/TagSettingAsyncListener.java @@ -2,7 +2,7 @@ package datadog.trace.instrumentation.servlet3; import static datadog.trace.instrumentation.servlet3.Servlet3Decorator.DECORATE; -import io.opentracing.Span; +import datadog.trace.instrumentation.api.AgentSpan; import io.opentracing.tag.Tags; import java.io.IOException; import java.util.concurrent.atomic.AtomicBoolean; @@ -12,9 +12,9 @@ import javax.servlet.http.HttpServletResponse; public class TagSettingAsyncListener implements AsyncListener { private final AtomicBoolean activated; - private final Span span; + private final AgentSpan span; - public TagSettingAsyncListener(final AtomicBoolean activated, final Span span) { + public TagSettingAsyncListener(final AtomicBoolean activated, final AgentSpan span) { this.activated = activated; this.span = span; } @@ -31,7 +31,7 @@ public class TagSettingAsyncListener implements AsyncListener { @Override public void onTimeout(final AsyncEvent event) throws IOException { if (activated.compareAndSet(false, true)) { - Tags.ERROR.set(span, Boolean.TRUE); + span.setError(true); span.setTag("timeout", event.getAsyncContext().getTimeout()); DECORATE.beforeFinish(span); span.finish(); @@ -45,7 +45,7 @@ public class TagSettingAsyncListener implements AsyncListener { if (((HttpServletResponse) event.getSuppliedResponse()).getStatus() == HttpServletResponse.SC_OK) { // exception is thrown in filter chain, but status code is incorrect - Tags.HTTP_STATUS.set(span, 500); + span.setTag(Tags.HTTP_STATUS.getKey(), 500); } DECORATE.onError(span, event.getThrowable()); DECORATE.beforeFinish(span);