Reduce tracer thread count by combining scheduled executors into a single executor.

Pulls out utility classes for reuse by other projects.

This also meant the dependency had to be bundled with dd-trace-ot since it isn't published as a separate dependency.

dd-java-agent/agent-bootstrap/agent-bootstrap.gradle

@@ -5,6 +5,10 @@ plugins {
 
 apply from: "${rootDir}/gradle/java.gradle"
 
+// FIXME: Improve test coverage.
+minimumBranchCoverage = 0.0
+minimumInstructionCoverage = 0.0
+
 dependencies {
   compile project(':dd-trace-api')
   compile deps.opentracing

dd-java-agent/agent-tooling/agent-tooling.gradle

@@ -23,7 +23,6 @@ dependencies {
 
   testCompile deps.opentracing
   testCompile project(':dd-java-agent:testing')
-  testCompile project(':utils:gc-utils')
 
   instrumentationMuzzle sourceSets.main.output
   instrumentationMuzzle configurations.compile

dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Cleaner.java

@@ -1,70 +1,33 @@
 package datadog.trace.agent.tooling;
 
+import static datadog.common.exec.SharedExecutors.isTaskSchedulerShutdown;
+import static datadog.common.exec.SharedExecutors.scheduleTaskAtFixedRate;
+
 import java.lang.ref.WeakReference;
 import java.util.concurrent.RejectedExecutionException;
-import java.util.concurrent.ScheduledExecutorService;
 import java.util.concurrent.ScheduledFuture;
-import java.util.concurrent.ScheduledThreadPoolExecutor;
-import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import lombok.extern.slf4j.Slf4j;
 
 @Slf4j
 class Cleaner {
-  private static final long SHUTDOWN_WAIT_SECONDS = 5;
-
-  private static final ThreadFactory THREAD_FACTORY =
-      new ThreadFactory() {
-        @Override
-        public Thread newThread(final Runnable r) {
-          final Thread thread = new Thread(r, "dd-cleaner");
-          thread.setDaemon(true);
-          thread.setPriority(Thread.MIN_PRIORITY);
-          thread.setContextClassLoader(null);
-          return thread;
-        }
-      };
-
-  private final ScheduledThreadPoolExecutor cleanerService;
-  private final Thread shutdownCallback;
-
-  Cleaner() {
-    cleanerService = new ScheduledThreadPoolExecutor(1, THREAD_FACTORY);
-    cleanerService.setRemoveOnCancelPolicy(true);
-    shutdownCallback = new ShutdownCallback(cleanerService);
-    try {
-      Runtime.getRuntime().addShutdownHook(shutdownCallback);
-    } catch (final IllegalStateException ex) {
-      // The JVM is already shutting down.
-    }
-  }
 
   <T> void scheduleCleaning(
       final T target, final Adapter<T> adapter, final long frequency, final TimeUnit unit) {
     final CleanupRunnable<T> command = new CleanupRunnable<>(target, adapter);
-    if (cleanerService.isShutdown()) {
-      log.warn("Cleaning scheduled but cleaner is shutdown. Target won't be cleaned {}", target);
+    if (isTaskSchedulerShutdown()) {
+      log.warn(
+          "Cleaning scheduled but task scheduler is shutdown. Target won't be cleaned {}", target);
     } else {
       try {
         // Schedule job and save future to allow job to be canceled if target is GC'd.
-        command.setFuture(cleanerService.scheduleAtFixedRate(command, frequency, frequency, unit));
+        command.setFuture(scheduleTaskAtFixedRate(command, frequency, frequency, unit));
       } catch (final RejectedExecutionException e) {
         log.warn("Cleaning task rejected. Target won't be cleaned {}", target);
       }
     }
   }
 
-  private void stop() {
-    cleanerService.shutdownNow();
-    Runtime.getRuntime().removeShutdownHook(shutdownCallback);
-  }
-
-  @Override
-  public void finalize() {
-    // Do we really want this?
-    stop();
-  }
-
   public interface Adapter<T> {
     void clean(T target);
   }
@@ -93,23 +56,4 @@ class Cleaner {
       this.future = future;
     }
   }
-
-  private static final class ShutdownCallback extends Thread {
-
-    private final ScheduledExecutorService executorService;
-
-    private ShutdownCallback(final ScheduledExecutorService executorService) {
-      this.executorService = executorService;
-    }
-
-    @Override
-    public void run() {
-      try {
-        executorService.shutdownNow();
-        executorService.awaitTermination(SHUTDOWN_WAIT_SECONDS, TimeUnit.SECONDS);
-      } catch (final InterruptedException e) {
-        // Don't bother waiting then...
-      }
-    }
-  }
 }

dd-java-agent/agent-tooling/src/test/groovy/datadog/trace/agent/tooling/CleanerTest.groovy

@@ -8,6 +8,7 @@ import java.lang.ref.WeakReference
 import java.util.concurrent.CountDownLatch
 import java.util.concurrent.atomic.AtomicInteger
 
+import static datadog.common.exec.SharedExecutors.isTaskSchedulerShutdown
 import static java.util.concurrent.TimeUnit.MILLISECONDS
 
 class CleanerTest extends DDSpecification {
@@ -15,10 +16,6 @@ class CleanerTest extends DDSpecification {
   @Subject
   def cleaner = new Cleaner()
 
-  def cleanup() {
-    cleaner.stop()
-  }
-
   def "test scheduling"() {
     setup:
     def latch = new CountDownLatch(2)
@@ -31,7 +28,7 @@ class CleanerTest extends DDSpecification {
     }
 
     expect:
-    !cleaner.cleanerService.isShutdown()
+    !isTaskSchedulerShutdown()
 
     when:
     cleaner.scheduleCleaning(target, action, 10, MILLISECONDS)
@@ -52,7 +49,7 @@ class CleanerTest extends DDSpecification {
     }
 
     expect:
-    !cleaner.cleanerService.isShutdown()
+    !isTaskSchedulerShutdown()
 
     when:
     cleaner.scheduleCleaning(target.get(), action, 10, MILLISECONDS)
@@ -76,7 +73,7 @@ class CleanerTest extends DDSpecification {
     }
 
     expect:
-    !cleaner.cleanerService.isShutdown()
+    !isTaskSchedulerShutdown()
 
     when:
     cleaner.scheduleCleaning(null, action, 10, MILLISECONDS)
@@ -85,16 +82,4 @@ class CleanerTest extends DDSpecification {
     then:
     callCount.get() == 0
   }
-
-  def "test shutdown"() {
-    expect:
-    !cleaner.cleanerService.isShutdown()
-
-    when:
-    cleaner.stop()
-
-    then:
-    cleaner.cleanerService.awaitTermination(50, MILLISECONDS)
-    cleaner.cleanerService.isTerminated()
-  }
 }

dd-java-agent/dd-java-agent.gradle

@@ -103,7 +103,7 @@ dependencies {
   testCompile project(':dd-java-agent:agent-bootstrap')
   testCompile project(':dd-trace-api')
   testCompile project(':dd-trace-ot')
-  testCompile project(':utils:gc-utils')
+  testCompile project(':utils:test-utils')
 
   testCompile deps.opentracingMock
   testCompile deps.testLogging

dd-java-agent/testing/testing.gradle

@@ -31,7 +31,6 @@ dependencies {
 
   compile deps.groovy
 
-  testCompile project(':utils:gc-utils')
   testCompile project(':utils:test-utils')
   testCompile project(':dd-java-agent:instrumentation:trace-annotation')
 

dd-trace-ot/dd-trace-ot.gradle

@@ -1,4 +1,5 @@
 plugins {
+  id "com.github.johnrengelman.shadow" version "5.2.0"
   id "me.champeau.gradle.jmh" version "0.5.0"
 }
 
@@ -32,6 +33,8 @@ dependencies {
   implementation deps.autoservice
 
   compile project(':dd-trace-api')
+  compile project(':utils:thread-utils')
+
   compile deps.opentracing
   compile group: 'io.opentracing.contrib', name: 'opentracing-tracerresolver', version: '0.1.0'
 
@@ -49,7 +52,6 @@ dependencies {
   testImplementation deps.autoservice
 
   testCompile project(":dd-java-agent:testing")
-  testCompile project(':utils:gc-utils')
   testCompile group: 'com.github.stefanbirkner', name: 'system-rules', version: '1.17.1'
   testCompile group: 'org.msgpack', name: 'jackson-dataformat-msgpack', version: '0.8.20'
 
@@ -82,6 +84,23 @@ dependencies {
 test.finalizedBy ot31CompatabilityTest
 test.finalizedBy ot33CompatabilityTest
 
+jar {
+  archiveClassifier = 'unbundled'
+}
+
+shadowJar {
+  archiveClassifier = ''
+
+  dependencies {
+    include(project(':utils:thread-utils'))
+  }
+}
+
+// We don't want bundled dependencies to show up in the pom.
+modifyPom {
+  dependencies.removeAll { it.artifactId == "thread-utils" }
+}
+
 jmh {
 //  include = [".*URLAsResourceNameBenchmark"]
 //  include = ['some regular expression'] // include pattern (regular expression) for benchmarks to be executed

dd-trace-ot/src/main/java/datadog/opentracing/DDTracer.java

@@ -839,6 +839,7 @@ public class DDTracer implements io.opentracing.Tracer, Closeable, datadog.trace
     private final WeakReference<DDTracer> reference;
 
     private ShutdownHook(final DDTracer tracer) {
+      super("dd-tracer-shutdown-hook");
       reference = new WeakReference<>(tracer);
     }
 

dd-trace-ot/src/main/java/datadog/opentracing/PendingTrace.java

@@ -1,5 +1,7 @@
 package datadog.opentracing;
 
+import static datadog.common.exec.SharedExecutors.scheduleTaskAtFixedRate;
+
 import datadog.opentracing.scopemanager.ContinuableScope;
 import datadog.trace.common.util.Clock;
 import java.io.Closeable;
@@ -15,9 +17,6 @@ import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.ConcurrentHashMap;
 import java.util.concurrent.ConcurrentLinkedDeque;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.concurrent.atomic.AtomicInteger;
@@ -253,6 +252,7 @@ public class PendingTrace extends ConcurrentLinkedDeque<DDSpan> {
       expireReference();
     }
     if (count > 0) {
+      // TODO attempt to flatten and report if top level spans are finished. (for accurate metrics)
       log.debug(
           "trace {} : {} unfinished spans garbage collected. Trace will not report.",
           traceId,
@@ -302,24 +302,12 @@ public class PendingTrace extends ConcurrentLinkedDeque<DDSpan> {
 
   private static class SpanCleaner implements Runnable, Closeable {
     private static final long CLEAN_FREQUENCY = 1;
-    private static final ThreadFactory FACTORY =
-        new ThreadFactory() {
-          @Override
-          public Thread newThread(final Runnable r) {
-            final Thread thread = new Thread(r, "dd-span-cleaner");
-            thread.setDaemon(true);
-            return thread;
-          }
-        };
-
-    private final ScheduledExecutorService executorService =
-        Executors.newScheduledThreadPool(1, FACTORY);
 
     private final Set<PendingTrace> pendingTraces =
         Collections.newSetFromMap(new ConcurrentHashMap<PendingTrace, Boolean>());
 
     public SpanCleaner() {
-      executorService.scheduleAtFixedRate(this, 0, CLEAN_FREQUENCY, TimeUnit.SECONDS);
+      scheduleTaskAtFixedRate(this, 0, CLEAN_FREQUENCY, TimeUnit.SECONDS);
     }
 
     @Override
@@ -331,13 +319,6 @@ public class PendingTrace extends ConcurrentLinkedDeque<DDSpan> {
 
     @Override
     public void close() {
-      executorService.shutdownNow();
-      try {
-        executorService.awaitTermination(500, TimeUnit.MILLISECONDS);
-      } catch (final InterruptedException e) {
-        log.info("Writer properly closed and async writer interrupted.");
-      }
-
       // Make sure that whatever was left over gets cleaned up
       run();
     }

dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/AbstractDisruptor.java

@@ -4,9 +4,9 @@ import com.lmax.disruptor.EventHandler;
 import com.lmax.disruptor.SleepingWaitStrategy;
 import com.lmax.disruptor.dsl.Disruptor;
 import com.lmax.disruptor.dsl.ProducerType;
+import datadog.common.exec.DaemonThreadFactory;
 import java.io.Closeable;
 import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import lombok.extern.slf4j.Slf4j;
 
@@ -33,7 +33,7 @@ abstract class AbstractDisruptor<T> implements Closeable {
     disruptor.handleEventsWith(handler);
   }
 
-  protected abstract ThreadFactory getThreadFactory();
+  protected abstract DaemonThreadFactory getThreadFactory();
 
   public void start() {
     disruptor.start();

dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/BatchWritingDisruptor.java

@@ -1,14 +1,14 @@
 package datadog.trace.common.writer.ddagent;
 
+import static datadog.common.exec.DaemonThreadFactory.TRACE_WRITER;
+import static datadog.common.exec.SharedExecutors.scheduleTaskAtFixedRate;
+
 import com.lmax.disruptor.EventHandler;
-import datadog.trace.common.util.DaemonThreadFactory;
+import datadog.common.exec.DaemonThreadFactory;
 import datadog.trace.common.writer.DDAgentWriter;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.concurrent.CountDownLatch;
-import java.util.concurrent.Executors;
-import java.util.concurrent.ScheduledExecutorService;
-import java.util.concurrent.ThreadFactory;
 import java.util.concurrent.TimeUnit;
 import lombok.extern.slf4j.Slf4j;
 
@@ -21,10 +21,6 @@ import lombok.extern.slf4j.Slf4j;
 public class BatchWritingDisruptor extends AbstractDisruptor<byte[]> {
   private static final int FLUSH_PAYLOAD_BYTES = 5_000_000; // 5 MB
 
-  // TODO: move executor to tracer for sharing with other tasks.
-  private final ScheduledExecutorService heartbeatExecutor =
-      Executors.newScheduledThreadPool(1, new DaemonThreadFactory("dd-trace-heartbeat"));
-
   private final DisruptorEvent.HeartbeatTranslator<byte[]> heartbeatTranslator =
       new DisruptorEvent.HeartbeatTranslator();
 
@@ -48,13 +44,13 @@ public class BatchWritingDisruptor extends AbstractDisruptor<byte[]> {
               }
             }
           };
-      heartbeatExecutor.scheduleAtFixedRate(heartbeat, 100, 100, TimeUnit.MILLISECONDS);
+      scheduleTaskAtFixedRate(heartbeat, 100, 100, TimeUnit.MILLISECONDS);
     }
   }
 
   @Override
-  protected ThreadFactory getThreadFactory() {
-    return new DaemonThreadFactory("dd-trace-writer");
+  protected DaemonThreadFactory getThreadFactory() {
+    return TRACE_WRITER;
   }
 
   @Override

dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/DDAgentApi.java

@@ -1,5 +1,6 @@
 package datadog.trace.common.writer.ddagent;
 
+import static datadog.common.exec.SharedExecutors.taskScheduler;
 import static datadog.trace.common.serialization.MsgpackFormatWriter.MSGPACK_WRITER;
 
 import com.squareup.moshi.JsonAdapter;
@@ -17,6 +18,7 @@ import java.util.List;
 import java.util.Map;
 import java.util.concurrent.TimeUnit;
 import lombok.extern.slf4j.Slf4j;
+import okhttp3.Dispatcher;
 import okhttp3.HttpUrl;
 import okhttp3.MediaType;
 import okhttp3.OkHttpClient;
@@ -263,6 +265,9 @@ public class DDAgentApi {
         .connectTimeout(HTTP_TIMEOUT, TimeUnit.SECONDS)
         .writeTimeout(HTTP_TIMEOUT, TimeUnit.SECONDS)
         .readTimeout(HTTP_TIMEOUT, TimeUnit.SECONDS)
+
+        // We don't do async so this shouldn't matter, but just to be safe...
+        .dispatcher(new Dispatcher(taskScheduler()))
         .build();
   }
 

dd-trace-ot/src/main/java/datadog/trace/common/writer/ddagent/TraceProcessingDisruptor.java

@@ -1,11 +1,12 @@
 package datadog.trace.common.writer.ddagent;
 
+import static datadog.common.exec.DaemonThreadFactory.TRACE_PROCESSOR;
+
 import com.lmax.disruptor.EventHandler;
+import datadog.common.exec.DaemonThreadFactory;
 import datadog.opentracing.DDSpan;
-import datadog.trace.common.util.DaemonThreadFactory;
 import datadog.trace.common.writer.DDAgentWriter;
 import java.util.List;
-import java.util.concurrent.ThreadFactory;
 import lombok.extern.slf4j.Slf4j;
 
 /**
@@ -29,8 +30,8 @@ public class TraceProcessingDisruptor extends AbstractDisruptor<List<DDSpan>> {
   }
 
   @Override
-  protected ThreadFactory getThreadFactory() {
-    return new DaemonThreadFactory("dd-trace-processor");
+  protected DaemonThreadFactory getThreadFactory() {
+    return TRACE_PROCESSOR;
   }
 
   @Override

gradle/java.gradle

@@ -6,8 +6,12 @@ apply plugin: 'groovy'
 apply from: "$rootDir/gradle/checkstyle.gradle"
 apply from: "$rootDir/gradle/codenarc.gradle"
 
-def applyCodeCoverage = !(project.plugins.hasPlugin('com.github.johnrengelman.shadow')
-  || project.path.startsWith(":dd-java-agent:instrumentation:"))
+def applyCodeCoverage = !(
+  project.path.startsWith(":dd-smoke-tests") ||
+    project.path == ":dd-java-agent" ||
+    project.path == ":dd-java-agent:load-generator" ||
+    project.path.startsWith(":dd-java-agent:benchmark") ||
+    project.path.startsWith(":dd-java-agent:instrumentation"))
 
 if (applyCodeCoverage) {
   apply from: "$rootDir/gradle/jacoco.gradle"

settings.gradle

@@ -31,8 +31,8 @@ include ':dd-java-agent:load-generator'
 
 // misc
 include ':dd-java-agent:testing'
-include ':utils:gc-utils'
 include ':utils:test-utils'
+include ':utils:thread-utils'
 
 // smoke tests
 include ':dd-smoke-tests:cli'

utils/thread-utils/src/main/java/datadog/common/exec/DaemonThreadFactory.java

@@ -1,9 +1,15 @@
-package datadog.trace.common.util;
+package datadog.common.exec;
 
 import java.util.concurrent.ThreadFactory;
 
 /** A {@link ThreadFactory} implementation that starts all {@link Thread} as daemons. */
 public final class DaemonThreadFactory implements ThreadFactory {
+  public static final DaemonThreadFactory TRACE_PROCESSOR =
+      new DaemonThreadFactory("dd-trace-processor");
+  public static final DaemonThreadFactory TRACE_WRITER = new DaemonThreadFactory("dd-trace-writer");
+  public static final DaemonThreadFactory TASK_SCHEDULER =
+      new DaemonThreadFactory("dd-task-scheduler");
+
   private final String threadName;
 
   /**

utils/thread-utils/src/main/java/datadog/common/exec/SharedExecutors.java

@@ -0,0 +1,56 @@
+package datadog.common.exec;
+
+import static datadog.common.exec.DaemonThreadFactory.TASK_SCHEDULER;
+
+import java.util.concurrent.Executors;
+import java.util.concurrent.ScheduledExecutorService;
+import java.util.concurrent.ScheduledFuture;
+import java.util.concurrent.TimeUnit;
+
+public final class SharedExecutors {
+  private static final long SHUTDOWN_WAIT_SECONDS = 5;
+
+  private static final ScheduledExecutorService TASK_SCHEDULER_EXECUTOR_SERVICE =
+      Executors.newSingleThreadScheduledExecutor(TASK_SCHEDULER);
+
+  static {
+    try {
+      Runtime.getRuntime().addShutdownHook(new ShutdownCallback(TASK_SCHEDULER_EXECUTOR_SERVICE));
+    } catch (final IllegalStateException ex) {
+      // The JVM is already shutting down.
+    }
+  }
+
+  public static ScheduledExecutorService taskScheduler() {
+    return TASK_SCHEDULER_EXECUTOR_SERVICE;
+  }
+
+  public static ScheduledFuture<?> scheduleTaskAtFixedRate(
+      final Runnable command, final long initialDelay, final long period, final TimeUnit unit) {
+    return TASK_SCHEDULER_EXECUTOR_SERVICE.scheduleAtFixedRate(command, initialDelay, period, unit);
+  }
+
+  public static boolean isTaskSchedulerShutdown() {
+    return TASK_SCHEDULER_EXECUTOR_SERVICE.isShutdown();
+  }
+
+  private static final class ShutdownCallback extends Thread {
+
+    private final ScheduledExecutorService executorService;
+
+    private ShutdownCallback(final ScheduledExecutorService executorService) {
+      super("dd-exec-shutdown-hook");
+      this.executorService = executorService;
+    }
+
+    @Override
+    public void run() {
+      try {
+        executorService.shutdown();
+        executorService.awaitTermination(SHUTDOWN_WAIT_SECONDS, TimeUnit.SECONDS);
+      } catch (final InterruptedException e) {
+        // Don't bother waiting then...
+      }
+    }
+  }
+}