From 4e58643bd07105b6fee12ca4afdf4be91fd1567f Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Wed, 22 Jan 2020 18:09:10 -0500 Subject: [PATCH 1/5] Initial finatra instrumentation --- .../instrumentation/finatra/finatra.gradle | 42 ++++++ .../finatra/FinatraDecorator.java | 51 +++++++ .../finatra/FinatraInstrumentation.java | 142 ++++++++++++++++++ .../ExecutorInstrumentationUtils.java | 20 ++- settings.gradle | 1 + 5 files changed, 251 insertions(+), 5 deletions(-) create mode 100644 dd-java-agent/instrumentation/finatra/finatra.gradle create mode 100644 dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java create mode 100644 dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java diff --git a/dd-java-agent/instrumentation/finatra/finatra.gradle b/dd-java-agent/instrumentation/finatra/finatra.gradle new file mode 100644 index 0000000000..f85086a2c6 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra/finatra.gradle @@ -0,0 +1,42 @@ +// Set properties before any plugins get loaded +ext { + minJavaVersionForTests = JavaVersion.VERSION_1_8 +} + +apply from: "${rootDir}/gradle/java.gradle" +apply from: "${rootDir}/gradle/test-with-scala.gradle" + +// apply plugin: 'org.unbroken-dome.test-sets' +//testSets { +// +// latestDepTest { +// dirName = 'test' +// } +//} + +muzzle { + +} + +dependencies { + compileOnly group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' + compileOnly group: 'com.twitter', name: 'finatra-http_2.11', version: '19.1.0' + + testCompile project(':dd-java-agent:instrumentation:netty-4.1') + testCompile project(':dd-java-agent:instrumentation:java-concurrent') + testCompile project(':dd-java-agent:instrumentation:trace-annotation') + + testCompile group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' + + testCompile(group: 'com.fasterxml.jackson.module', name: 'jackson-module-scala_2.11', version: '2.10.2') { + force = true + } + +// latestDepTestCompile project(':dd-java-agent:instrumentation:java-concurrent') + // latestDepTestCompile project(':dd-java-agent:instrumentation:trace-annotation') +} + +//compileLatestDepTestGroovy { +// classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) +// dependsOn compileLatestDepTestScala +//} diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java new file mode 100644 index 0000000000..3d8cd1d1df --- /dev/null +++ b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java @@ -0,0 +1,51 @@ +package datadog.trace.instrumentation.finatra; + +import com.twitter.finagle.http.Request; +import com.twitter.finagle.http.Response; +import datadog.trace.agent.decorator.HttpServerDecorator; +import java.net.URI; +import java.net.URISyntaxException; + +public class FinatraDecorator extends HttpServerDecorator { + public static final FinatraDecorator DECORATE = new FinatraDecorator(); + + @Override + protected String component() { + return "finatra"; + } + + @Override + protected String method(final Request request) { + return request.method().name(); + } + + @Override + protected URI url(final Request request) throws URISyntaxException { + return URI.create(request.uri()); + } + + @Override + protected String peerHostname(final Request request) { + return request.remoteHost(); + } + + @Override + protected String peerHostIP(final Request request) { + return request.remoteAddress().getHostAddress(); + } + + @Override + protected Integer peerPort(final Request request) { + return request.remotePort(); + } + + @Override + protected Integer status(final Response response) { + return response.statusCode(); + } + + @Override + protected String[] instrumentationNames() { + return new String[] {"finatra"}; + } +} diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java new file mode 100644 index 0000000000..2911bd58bf --- /dev/null +++ b/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -0,0 +1,142 @@ +package datadog.trace.instrumentation.finatra; + +import static datadog.trace.agent.tooling.ByteBuddyElementMatchers.safeHasSuperType; +import static datadog.trace.instrumentation.api.AgentTracer.activateSpan; +import static datadog.trace.instrumentation.api.AgentTracer.activeSpan; +import static datadog.trace.instrumentation.api.AgentTracer.startSpan; +import static datadog.trace.instrumentation.finatra.FinatraDecorator.DECORATE; +import static java.util.Collections.singletonMap; +import static net.bytebuddy.matcher.ElementMatchers.isInterface; +import static net.bytebuddy.matcher.ElementMatchers.isMethod; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.not; +import static net.bytebuddy.matcher.ElementMatchers.takesArgument; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import com.google.auto.service.AutoService; +import com.twitter.finagle.http.Request; +import com.twitter.finagle.http.Response; +import com.twitter.util.Future; +import com.twitter.util.FutureEventListener; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.api.DDTags; +import datadog.trace.instrumentation.api.AgentScope; +import datadog.trace.instrumentation.api.AgentSpan; +import datadog.trace.instrumentation.api.Tags; +import java.lang.reflect.Method; +import java.util.Map; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import scala.Some; + +@AutoService(Instrumenter.class) +public class FinatraInstrumentation extends Instrumenter.Default { + public FinatraInstrumentation() { + super("finatra"); + } + + @Override + public String[] helperClassNames() { + return new String[] { + "datadog.trace.agent.decorator.BaseDecorator", + "datadog.trace.agent.decorator.ServerDecorator", + "datadog.trace.agent.decorator.HttpServerDecorator", + packageName + ".FinatraDecorator", + FinatraInstrumentation.class.getName() + "$Listener" + }; + } + + @Override + public ElementMatcher typeMatcher() { + return not(isInterface()) + .and(safeHasSuperType(named("com.twitter.finatra.http.internal.routing.Route"))); + } + + @Override + public Map, String> transformers() { + return singletonMap( + isMethod() + .and(named("handleMatch")) + .and(takesArguments(2)) + .and(takesArgument(0, named("com.twitter.finagle.http.Request"))), + FinatraInstrumentation.class.getName() + "$RouteAdvice"); + } + + public static class RouteAdvice { + @Advice.OnMethodEnter(suppress = Throwable.class) + public static AgentScope nameSpan( + @Advice.Argument(0) final Request request, + @Advice.FieldValue("path") final String path, + @Advice.FieldValue("clazz") final Class clazz, + @Advice.Origin final Method method) { + + final AgentSpan parent = activeSpan(); + + final AgentSpan span = startSpan("finatra.request"); + DECORATE.afterStart(span); + DECORATE.onConnection(span, request); + DECORATE.onRequest(span, request); + + if (parent != null && "netty.request".equals(parent.getSpanName())) { + parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); + parent.setTag(Tags.COMPONENT, "finatra"); + parent.setSpanName("finatra.request"); + + span.setSpanName("finatra.controller"); + span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); + } + + final AgentScope scope = activateSpan(span, false); + scope.setAsyncPropagation(true); + return scope; + } + + @Advice.OnMethodExit(onThrowable = Throwable.class) + public static void setupCallback( + @Advice.Enter final AgentScope scope, + @Advice.Thrown final Throwable throwable, + @Advice.Return final Some> responseOption) { + + if (scope == null) { + return; + } + + final AgentSpan span = scope.span(); + if (throwable != null) { + DECORATE.onError(span, throwable); + DECORATE.beforeFinish(span); + span.finish(); + scope.close(); + return; + } + + responseOption.get().addEventListener(new Listener(scope)); + } + } + + public static class Listener implements FutureEventListener { + private final AgentScope scope; + + public Listener(final AgentScope scope) { + this.scope = scope; + } + + @Override + public void onSuccess(final Response response) { + DECORATE.onResponse(scope.span(), response); + DECORATE.beforeFinish(scope.span()); + scope.span().finish(); + scope.close(); + } + + @Override + public void onFailure(final Throwable cause) { + DECORATE.onError(scope.span(), cause); + DECORATE.beforeFinish(scope.span()); + scope.span().finish(); + scope.close(); + } + } +} diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java index a7ca0eaf54..6967415e8d 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java @@ -44,12 +44,22 @@ public class ExecutorInstrumentationUtils { */ public static State setupState( final ContextStore contextStore, final T task, final TraceScope scope) { + final State state = contextStore.putIfAbsent(task, State.FACTORY); - final TraceScope.Continuation continuation = scope.capture(); - if (state.setContinuation(continuation)) { - log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); - } else { - continuation.close(false); + + // Don't instrument the executor's own runnables. These runnables may never return until + // netty shuts down. Any created continuations will be open until that time preventing traces + // from being reported + if (!task.getClass() + .getName() + .startsWith("io.netty.util.concurrent.SingleThreadEventExecutor$")) { + + final TraceScope.Continuation continuation = scope.capture(); + if (state.setContinuation(continuation)) { + log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); + } else { + continuation.close(false); + } } return state; } diff --git a/settings.gradle b/settings.gradle index 8a982d5967..0914d6bd70 100644 --- a/settings.gradle +++ b/settings.gradle @@ -60,6 +60,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-2' include ':dd-java-agent:instrumentation:elasticsearch:transport-5' include ':dd-java-agent:instrumentation:elasticsearch:transport-5.3' include ':dd-java-agent:instrumentation:elasticsearch:transport-6' +include ':dd-java-agent:instrumentation:finatra' include ':dd-java-agent:instrumentation:glassfish' include ':dd-java-agent:instrumentation:google-http-client' include ':dd-java-agent:instrumentation:grizzly-2' From 68e52497d65503f70c5b0fe311fd2a54a51b2c22 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 23 Jan 2020 10:51:38 -0500 Subject: [PATCH 2/5] Move to versioned folder name --- .../finatra-2.9/finatra-2.9.gradle | 55 +++++++++++++++++++ .../finatra/FinatraDecorator.java | 0 .../finatra/FinatraInstrumentation.java | 0 .../instrumentation/finatra/finatra.gradle | 42 -------------- settings.gradle | 2 +- 5 files changed, 56 insertions(+), 43 deletions(-) create mode 100644 dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle rename dd-java-agent/instrumentation/{finatra => finatra-2.9}/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java (100%) rename dd-java-agent/instrumentation/{finatra => finatra-2.9}/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java (100%) delete mode 100644 dd-java-agent/instrumentation/finatra/finatra.gradle diff --git a/dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle b/dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle new file mode 100644 index 0000000000..b856b7d3ee --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/finatra-2.9.gradle @@ -0,0 +1,55 @@ +// Set properties before any plugins get loaded +ext { + minJavaVersionForTests = JavaVersion.VERSION_1_8 +} + +apply from: "${rootDir}/gradle/java.gradle" +apply from: "${rootDir}/gradle/test-with-scala.gradle" + +apply plugin: 'org.unbroken-dome.test-sets' + +testSets { + + latestDepTest { + dirName = 'test' + } +} + +muzzle { + // There are some weird library issues below 2.9 so can't assert inverse + pass { + group = 'com.twitter' + module = 'finatra-http_2.11' + versions = '[2.9.0,]' + } + + pass { + group = 'com.twitter' + module = 'finatra-http_2.12' + versions = '[2.9.0,]' + } +} + +dependencies { + compileOnly group: 'com.twitter', name: 'finatra-http_2.11', version: '2.9.0' + + testCompile project(':dd-java-agent:instrumentation:netty-4.1') + testCompile project(':dd-java-agent:instrumentation:java-concurrent') + + testCompile group: 'com.twitter', name: 'finatra-http_2.11', version: '19.12.0' + testCompile(group: 'com.fasterxml.jackson.core', name: 'jackson-databind', version: '2.9.10') { + force = true + } + + // Required for older versions of finatra on JDKs >= 11 + testCompile group: 'com.sun.activation', name: 'javax.activation', version: '1.2.0' + + latestDepTestCompile project(':dd-java-agent:instrumentation:netty-4.1') + latestDepTestCompile project(':dd-java-agent:instrumentation:java-concurrent') + latestDepTestCompile group: 'com.twitter', name: 'finatra-http_2.11', version: '+' +} + +compileLatestDepTestGroovy { + classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) + dependsOn compileLatestDepTestScala +} diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java similarity index 100% rename from dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java rename to dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraDecorator.java diff --git a/dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java similarity index 100% rename from dd-java-agent/instrumentation/finatra/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java rename to dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java diff --git a/dd-java-agent/instrumentation/finatra/finatra.gradle b/dd-java-agent/instrumentation/finatra/finatra.gradle deleted file mode 100644 index f85086a2c6..0000000000 --- a/dd-java-agent/instrumentation/finatra/finatra.gradle +++ /dev/null @@ -1,42 +0,0 @@ -// Set properties before any plugins get loaded -ext { - minJavaVersionForTests = JavaVersion.VERSION_1_8 -} - -apply from: "${rootDir}/gradle/java.gradle" -apply from: "${rootDir}/gradle/test-with-scala.gradle" - -// apply plugin: 'org.unbroken-dome.test-sets' -//testSets { -// -// latestDepTest { -// dirName = 'test' -// } -//} - -muzzle { - -} - -dependencies { - compileOnly group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' - compileOnly group: 'com.twitter', name: 'finatra-http_2.11', version: '19.1.0' - - testCompile project(':dd-java-agent:instrumentation:netty-4.1') - testCompile project(':dd-java-agent:instrumentation:java-concurrent') - testCompile project(':dd-java-agent:instrumentation:trace-annotation') - - testCompile group: 'com.twitter', name: 'finagle-http_2.11', version: '19.1.0' - - testCompile(group: 'com.fasterxml.jackson.module', name: 'jackson-module-scala_2.11', version: '2.10.2') { - force = true - } - -// latestDepTestCompile project(':dd-java-agent:instrumentation:java-concurrent') - // latestDepTestCompile project(':dd-java-agent:instrumentation:trace-annotation') -} - -//compileLatestDepTestGroovy { -// classpath = classpath.plus(files(compileLatestDepTestScala.destinationDir)) -// dependsOn compileLatestDepTestScala -//} diff --git a/settings.gradle b/settings.gradle index 0914d6bd70..d649c059f2 100644 --- a/settings.gradle +++ b/settings.gradle @@ -60,7 +60,7 @@ include ':dd-java-agent:instrumentation:elasticsearch:transport-2' include ':dd-java-agent:instrumentation:elasticsearch:transport-5' include ':dd-java-agent:instrumentation:elasticsearch:transport-5.3' include ':dd-java-agent:instrumentation:elasticsearch:transport-6' -include ':dd-java-agent:instrumentation:finatra' +include ':dd-java-agent:instrumentation:finatra-2.9' include ':dd-java-agent:instrumentation:glassfish' include ':dd-java-agent:instrumentation:google-http-client' include ':dd-java-agent:instrumentation:grizzly-2' From 8ff985afdbaf3fdf8ddf1f6d42396cf70130625a Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 23 Jan 2020 12:04:56 -0500 Subject: [PATCH 3/5] Cleaner way to skip netty executor's tasks --- .../ExecutorInstrumentationUtils.java | 38 +++++++++++-------- 1 file changed, 22 insertions(+), 16 deletions(-) diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java index 6967415e8d..c61aab869c 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/ExecutorInstrumentationUtils.java @@ -26,11 +26,24 @@ public class ExecutorInstrumentationUtils { * @return true iff given task object should be wrapped */ public static boolean shouldAttachStateToTask(final Object task, final Executor executor) { + if (task == null) { + return false; + } + final TraceScope scope = activeScope(); - return (scope != null + final Class enclosingClass = task.getClass().getEnclosingClass(); + + return scope != null && scope.isAsyncPropagating() - && task != null - && !ExecutorInstrumentationUtils.isExecutorDisabledForThisTask(executor, task)); + && !ExecutorInstrumentationUtils.isExecutorDisabledForThisTask(executor, task) + + // Don't instrument the executor's own runnables. These runnables may never return until + // netty shuts down. Any created continuations will be open until that time preventing + // traces from being reported + && (enclosingClass == null + || !enclosingClass + .getName() + .equals("io.netty.util.concurrent.SingleThreadEventExecutor")); } /** @@ -47,20 +60,13 @@ public class ExecutorInstrumentationUtils { final State state = contextStore.putIfAbsent(task, State.FACTORY); - // Don't instrument the executor's own runnables. These runnables may never return until - // netty shuts down. Any created continuations will be open until that time preventing traces - // from being reported - if (!task.getClass() - .getName() - .startsWith("io.netty.util.concurrent.SingleThreadEventExecutor$")) { - - final TraceScope.Continuation continuation = scope.capture(); - if (state.setContinuation(continuation)) { - log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); - } else { - continuation.close(false); - } + final TraceScope.Continuation continuation = scope.capture(); + if (state.setContinuation(continuation)) { + log.debug("created continuation {} from scope {}, state: {}", continuation, scope, state); + } else { + continuation.close(false); } + return state; } From 37a279069b87af184930d70209077a03b23b50f5 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 30 Jan 2020 15:23:26 -0500 Subject: [PATCH 4/5] Add server tests --- .../finatra/FinatraInstrumentation.java | 13 ++- .../src/test/groovy/FinatraServerTest.groovy | 91 +++++++++++++++++++ .../NettyServerTestInstrumentation.java | 20 ++++ .../src/test/scala/FinatraController.scala | 56 ++++++++++++ .../src/test/scala/FinatraServer.scala | 13 +++ .../ResponseSettingExceptionMapper.scala | 15 +++ 6 files changed, 205 insertions(+), 3 deletions(-) create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala create mode 100644 dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java index 2911bd58bf..3c426fbdde 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java +++ b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -19,6 +19,7 @@ import com.twitter.finagle.http.Response; import com.twitter.util.Future; import com.twitter.util.FutureEventListener; import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.api.Config; import datadog.trace.api.DDTags; import datadog.trace.instrumentation.api.AgentScope; import datadog.trace.instrumentation.api.AgentSpan; @@ -76,8 +77,6 @@ public class FinatraInstrumentation extends Instrumenter.Default { final AgentSpan span = startSpan("finatra.request"); DECORATE.afterStart(span); - DECORATE.onConnection(span, request); - DECORATE.onRequest(span, request); if (parent != null && "netty.request".equals(parent.getSpanName())) { parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); @@ -86,6 +85,9 @@ public class FinatraInstrumentation extends Instrumenter.Default { span.setSpanName("finatra.controller"); span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); + } else { + DECORATE.onConnection(span, request); + DECORATE.onRequest(span, request); } final AgentScope scope = activateSpan(span, false); @@ -125,7 +127,12 @@ public class FinatraInstrumentation extends Instrumenter.Default { @Override public void onSuccess(final Response response) { - DECORATE.onResponse(scope.span(), response); + if ("finatra.request".equals(scope.span().getSpanName())) { + DECORATE.onResponse(scope.span(), response); + } else if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { + scope.span().setError(true); + } + DECORATE.beforeFinish(scope.span()); scope.span().finish(); scope.close(); diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy new file mode 100644 index 0000000000..da891d5fc7 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/FinatraServerTest.groovy @@ -0,0 +1,91 @@ +import com.twitter.finatra.http.HttpServer +import com.twitter.util.Await +import com.twitter.util.Closable +import com.twitter.util.Duration +import datadog.opentracing.DDSpan +import datadog.trace.agent.test.asserts.TraceAssert +import datadog.trace.agent.test.base.HttpServerTest +import datadog.trace.api.DDSpanTypes +import datadog.trace.instrumentation.api.Tags +import datadog.trace.instrumentation.finatra.FinatraDecorator + +import java.util.concurrent.TimeoutException + +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.ERROR +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.EXCEPTION +import static datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint.SUCCESS + +class FinatraServerTest extends HttpServerTest { + private static final Duration TIMEOUT = Duration.fromSeconds(5) + private static final long STARTUP_TIMEOUT = 20 * 1000 + + static closeAndWait(Closable closable) { + if (closable != null) { + Await.ready(closable.close(), TIMEOUT) + } + } + + @Override + HttpServer startServer(int port) { + HttpServer testServer = new FinatraServer() + + // Starting the server is blocking so start it in a separate thread + Thread startupThread = new Thread({ + testServer.main("-admin.port=:0", "-http.port=:" + port) + }) + startupThread.setDaemon(true) + startupThread.start() + + long startupDeadline = System.currentTimeMillis() + STARTUP_TIMEOUT + while (!testServer.started()) { + if (System.currentTimeMillis() > startupDeadline) { + throw new TimeoutException("Timed out waiting for server startup") + } + } + + return testServer + } + + @Override + boolean hasHandlerSpan() { + return true + } + + @Override + void stopServer(HttpServer httpServer) { + Await.ready(httpServer.close(), TIMEOUT) + } + + @Override + FinatraDecorator decorator() { + return FinatraDecorator.DECORATE + } + + @Override + String expectedOperationName() { + return "finatra.request" + } + + void handlerSpan(TraceAssert trace, int index, Object parent, ServerEndpoint endpoint = SUCCESS) { + def errorEndpoint = endpoint == EXCEPTION || endpoint == ERROR + trace.span(index) { + serviceName expectedServiceName() + operationName "finatra.controller" + resourceName "FinatraController" + spanType DDSpanTypes.HTTP_SERVER + errored errorEndpoint + childOf(parent as DDSpan) + tags { + "$Tags.COMPONENT" FinatraDecorator.DECORATE.component() + "$Tags.SPAN_KIND" Tags.SPAN_KIND_SERVER + + // Finatra doesn't propagate the stack trace or exception to the instrumentation + // so the normal errorTags() method can't be used + if (errorEndpoint) { + "$Tags.ERROR" true + } + defaultTags() + } + } + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java new file mode 100644 index 0000000000..224b08f4af --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/groovy/NettyServerTestInstrumentation.java @@ -0,0 +1,20 @@ +import static net.bytebuddy.matcher.ElementMatchers.named; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.test.base.HttpServerTestAdvice; +import datadog.trace.agent.tooling.Instrumenter; +import net.bytebuddy.agent.builder.AgentBuilder; + +@AutoService(Instrumenter.class) +public class NettyServerTestInstrumentation implements Instrumenter { + + @Override + public AgentBuilder instrument(final AgentBuilder agentBuilder) { + return agentBuilder + .type(named("io.netty.handler.codec.ByteToMessageDecoder")) + .transform( + new AgentBuilder.Transformer.ForAdvice() + .advice( + named("channelRead"), HttpServerTestAdvice.ServerEntryAdvice.class.getName())); + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala new file mode 100644 index 0000000000..6a5503c8a4 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraController.scala @@ -0,0 +1,56 @@ +import com.twitter.finagle.http.{Request, Response} +import com.twitter.finatra.http.Controller +import com.twitter.util.Future +import datadog.trace.agent.test.base.HttpServerTest.ServerEndpoint._ +import datadog.trace.agent.test.base.HttpServerTest.controller +import groovy.lang.Closure + +class FinatraController extends Controller { + any(SUCCESS.getPath) { request: Request => + controller(SUCCESS, new Closure[Response](null) { + override def call(): Response = { + response.ok(SUCCESS.getBody) + } + }) + } + + any(ERROR.getPath) { request: Request => + controller(ERROR, new Closure[Response](null) { + override def call(): Response = { + response.internalServerError(ERROR.getBody) + } + }) + } + + any(NOT_FOUND.getPath) { request: Request => + controller(NOT_FOUND, new Closure[Response](null) { + override def call(): Response = { + response.notFound(NOT_FOUND.getBody) + } + }) + } + + any(QUERY_PARAM.getPath) { request: Request => + controller(QUERY_PARAM, new Closure[Response](null) { + override def call(): Response = { + response.ok(QUERY_PARAM.getBody) + } + }) + } + + any(EXCEPTION.getPath) { request: Request => + controller(EXCEPTION, new Closure[Future[Response]](null) { + override def call(): Future[Response] = { + throw new Exception(EXCEPTION.getBody) + } + }) + } + + any(REDIRECT.getPath) { request: Request => + controller(REDIRECT, new Closure[Response](null) { + override def call(): Response = { + response.found.location(REDIRECT.getBody) + } + }) + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala new file mode 100644 index 0000000000..0bc035e017 --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/FinatraServer.scala @@ -0,0 +1,13 @@ +import com.twitter.finagle.http.Request +import com.twitter.finatra.http.HttpServer +import com.twitter.finatra.http.filters.ExceptionMappingFilter +import com.twitter.finatra.http.routing.HttpRouter + +class FinatraServer extends HttpServer { + override protected def configureHttp(router: HttpRouter): Unit = { + router + .filter[ExceptionMappingFilter[Request]] + .add[FinatraController] + .exceptionMapper[ResponseSettingExceptionMapper] + } +} diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala new file mode 100644 index 0000000000..de5a31928d --- /dev/null +++ b/dd-java-agent/instrumentation/finatra-2.9/src/test/scala/ResponseSettingExceptionMapper.scala @@ -0,0 +1,15 @@ + + +import com.twitter.finagle.http.{Request, Response} +import com.twitter.finatra.http.exceptions.ExceptionMapper +import com.twitter.finatra.http.response.ResponseBuilder +import javax.inject.{Inject, Singleton} + +@Singleton +class ResponseSettingExceptionMapper @Inject()(response: ResponseBuilder) + extends ExceptionMapper[Exception] { + + override def toResponse(request: Request, exception: Exception): Response = { + response.internalServerError(exception.getMessage) + } +} From ed12af6994f43ff2496a6ad3bfddd5b66ab6ce51 Mon Sep 17 00:00:00 2001 From: Laplie Anderson Date: Thu, 30 Jan 2020 18:21:49 -0500 Subject: [PATCH 5/5] Assume parent span is netty --- .../finatra/FinatraInstrumentation.java | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java index 3c426fbdde..19269c0d16 100644 --- a/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java +++ b/dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java @@ -73,22 +73,15 @@ public class FinatraInstrumentation extends Instrumenter.Default { @Advice.FieldValue("clazz") final Class clazz, @Advice.Origin final Method method) { + // Update the parent "netty.request" final AgentSpan parent = activeSpan(); + parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); + parent.setTag(Tags.COMPONENT, "finatra"); + parent.setSpanName("finatra.request"); - final AgentSpan span = startSpan("finatra.request"); + final AgentSpan span = startSpan("finatra.controller"); DECORATE.afterStart(span); - - if (parent != null && "netty.request".equals(parent.getSpanName())) { - parent.setTag(DDTags.RESOURCE_NAME, request.method().name() + " " + path); - parent.setTag(Tags.COMPONENT, "finatra"); - parent.setSpanName("finatra.request"); - - span.setSpanName("finatra.controller"); - span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); - } else { - DECORATE.onConnection(span, request); - DECORATE.onRequest(span, request); - } + span.setTag(DDTags.RESOURCE_NAME, DECORATE.spanNameForClass(clazz)); final AgentScope scope = activateSpan(span, false); scope.setAsyncPropagation(true); @@ -127,9 +120,8 @@ public class FinatraInstrumentation extends Instrumenter.Default { @Override public void onSuccess(final Response response) { - if ("finatra.request".equals(scope.span().getSpanName())) { - DECORATE.onResponse(scope.span(), response); - } else if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { + // Don't use DECORATE.onResponse because this is the controller span + if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) { scope.span().setError(true); }