Merge pull request #1188 from DataDog/landerson/finatra

Finatra Instrumentation

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
+}

dd-java-agent/instrumentation/finatra-2.9/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<Request, Request, Response> {
+  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"};
+  }
+}

dd-java-agent/instrumentation/finatra-2.9/src/main/java/datadog/trace/instrumentation/finatra/FinatraInstrumentation.java

@@ -0,0 +1,141 @@
+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.Config;
+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<? super TypeDescription> typeMatcher() {
+    return not(isInterface())
+        .and(safeHasSuperType(named("com.twitter.finatra.http.internal.routing.Route")));
+  }
+
+  @Override
+  public Map<? extends ElementMatcher<? super MethodDescription>, 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) {
+
+      // 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.controller");
+      DECORATE.afterStart(span);
+      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<Future<Response>> 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<Response> {
+    private final AgentScope scope;
+
+    public Listener(final AgentScope scope) {
+      this.scope = scope;
+    }
+
+    @Override
+    public void onSuccess(final Response response) {
+      // Don't use DECORATE.onResponse because this is the controller span
+      if (Config.get().getHttpServerErrorStatuses().contains(DECORATE.status(response))) {
+        scope.span().setError(true);
+      }
+
+      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();
+    }
+  }
+}

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<HttpServer, FinatraDecorator> {
+  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()
+      }
+    }
+  }
+}

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()));
+  }
+}

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)
+      }
+    })
+  }
+}

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]
+  }
+}

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)
+  }
+}

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"));
   }
 
   /**
@@ -44,13 +57,16 @@ public class ExecutorInstrumentationUtils {
    */
   public static <T> State setupState(
       final ContextStore<T, State> 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);
     }
+
     return state;
   }
 

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-2.9'
 include ':dd-java-agent:instrumentation:glassfish'
 include ':dd-java-agent:instrumentation:google-http-client'
 include ':dd-java-agent:instrumentation:grizzly-2'