Merge pull request #952 from DataDog/mar-kolya/do-not-trace-datadog-cl

Do not trace failed class loads via Datadog Classloader

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/DatadogClassLoader.java

@@ -1,20 +1,8 @@
 package datadog.trace.bootstrap;
 
-import java.io.ByteArrayInputStream;
-import java.io.ByteArrayOutputStream;
-import java.io.IOException;
-import java.io.InputStream;
 import java.net.MalformedURLException;
 import java.net.URL;
 import java.net.URLClassLoader;
-import java.net.URLConnection;
-import java.net.URLStreamHandler;
-import java.nio.file.NoSuchFileException;
-import java.security.Permission;
-import java.util.HashMap;
-import java.util.Map;
-import java.util.jar.JarEntry;
-import java.util.jar.JarInputStream;
 import lombok.extern.slf4j.Slf4j;
 
 /**
@@ -116,99 +104,4 @@ public class DatadogClassLoader extends URLClassLoader {
       throw new ClassNotFoundException(name);
     }
   }
-
-  protected static class InternalJarURLHandler extends URLStreamHandler {
-    private final Map<String, byte[]> filenameToBytes = new HashMap<>();
-
-    public InternalJarURLHandler(
-        final String internalJarFileName, final ClassLoader classloaderForJarResource) {
-
-      // "/" is used as the default url of the jar
-      // This is called by the SecureClassLoader trying to obtain permissions
-      filenameToBytes.put("/", new byte[] {});
-
-      final InputStream jarStream =
-          classloaderForJarResource.getResourceAsStream(internalJarFileName);
-
-      if (jarStream != null) {
-        try (final JarInputStream inputStream = new JarInputStream(jarStream)) {
-          JarEntry entry = inputStream.getNextJarEntry();
-
-          while (entry != null) {
-            filenameToBytes.put("/" + entry.getName(), getBytes(inputStream));
-
-            entry = inputStream.getNextJarEntry();
-          }
-
-        } catch (final IOException e) {
-          log.error("Unable to read internal jar", e);
-        }
-      } else {
-        log.error("Internal jar not found");
-      }
-    }
-
-    @Override
-    protected URLConnection openConnection(final URL url) throws IOException {
-      final byte[] bytes = filenameToBytes.get(url.getFile());
-
-      if (bytes == null) {
-        throw new NoSuchFileException(url.getFile(), null, url.getFile() + " not in internal jar");
-      }
-
-      return new InternalJarURLConnection(url, bytes);
-    }
-  }
-
-  protected static class InternalJarURLConnection extends URLConnection {
-    private final byte[] bytes;
-
-    private InternalJarURLConnection(final URL url, final byte[] bytes) {
-      super(url);
-      this.bytes = bytes;
-    }
-
-    @Override
-    public void connect() throws IOException {
-      connected = true;
-    }
-
-    @Override
-    public InputStream getInputStream() throws IOException {
-      return new ByteArrayInputStream(bytes);
-    }
-
-    @Override
-    public Permission getPermission() {
-      // No permissions needed because all classes are in memory
-      return null;
-    }
-  }
-
-  /**
-   * Standard "copy InputStream to byte[]" implementation using a ByteArrayOutputStream
-   *
-   * <p>IOUtils.toByteArray() or Java 9's InputStream.readAllBytes() could be replacements if they
-   * were available
-   *
-   * <p>This can be optimized using the JarEntry's size(), but its not always available
-   *
-   * @param inputStream
-   * @return a byte[] from the inputstream
-   * @throws IOException
-   */
-  private static byte[] getBytes(final InputStream inputStream) throws IOException {
-    final byte[] buffer = new byte[4096];
-
-    int bytesRead = inputStream.read(buffer, 0, buffer.length);
-    try (final ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
-      while (bytesRead != -1) {
-        outputStream.write(buffer, 0, bytesRead);
-
-        bytesRead = inputStream.read(buffer, 0, buffer.length);
-      }
-
-      return outputStream.toByteArray();
-    }
-  }
 }

dd-java-agent/agent-bootstrap/src/main/java/datadog/trace/bootstrap/InternalJarURLHandler.java

@@ -0,0 +1,111 @@
+package datadog.trace.bootstrap;
+
+import java.io.ByteArrayInputStream;
+import java.io.ByteArrayOutputStream;
+import java.io.IOException;
+import java.io.InputStream;
+import java.net.URL;
+import java.net.URLConnection;
+import java.net.URLStreamHandler;
+import java.nio.file.NoSuchFileException;
+import java.security.Permission;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.jar.JarEntry;
+import java.util.jar.JarInputStream;
+import lombok.extern.slf4j.Slf4j;
+
+@Slf4j
+public class InternalJarURLHandler extends URLStreamHandler {
+  private final Map<String, byte[]> filenameToBytes = new HashMap<>();
+
+  InternalJarURLHandler(
+      final String internalJarFileName, final ClassLoader classloaderForJarResource) {
+
+    // "/" is used as the default url of the jar
+    // This is called by the SecureClassLoader trying to obtain permissions
+    filenameToBytes.put("/", new byte[] {});
+
+    final InputStream jarStream =
+        classloaderForJarResource.getResourceAsStream(internalJarFileName);
+
+    if (jarStream != null) {
+      try (final JarInputStream inputStream = new JarInputStream(jarStream)) {
+        JarEntry entry = inputStream.getNextJarEntry();
+
+        while (entry != null) {
+          filenameToBytes.put("/" + entry.getName(), getBytes(inputStream));
+
+          entry = inputStream.getNextJarEntry();
+        }
+
+      } catch (final IOException e) {
+        log.error("Unable to read internal jar", e);
+      }
+    } else {
+      log.error("Internal jar not found");
+    }
+  }
+
+  @Override
+  protected URLConnection openConnection(final URL url) throws IOException {
+    final byte[] bytes = filenameToBytes.get(url.getFile());
+
+    if (bytes == null) {
+      throw new NoSuchFileException(url.getFile(), null, url.getFile() + " not in internal jar");
+    }
+
+    return new InternalJarURLConnection(url, bytes);
+  }
+
+  /**
+   * Standard "copy InputStream to byte[]" implementation using a ByteArrayOutputStream
+   *
+   * <p>IOUtils.toByteArray() or Java 9's InputStream.readAllBytes() could be replacements if they
+   * were available
+   *
+   * <p>This can be optimized using the JarEntry's size(), but its not always available
+   *
+   * @param inputStream stream to read
+   * @return a byte[] from the inputstream
+   */
+  private static byte[] getBytes(final InputStream inputStream) throws IOException {
+    final byte[] buffer = new byte[4096];
+
+    int bytesRead = inputStream.read(buffer, 0, buffer.length);
+    try (final ByteArrayOutputStream outputStream = new ByteArrayOutputStream()) {
+      while (bytesRead != -1) {
+        outputStream.write(buffer, 0, bytesRead);
+
+        bytesRead = inputStream.read(buffer, 0, buffer.length);
+      }
+
+      return outputStream.toByteArray();
+    }
+  }
+
+  private static class InternalJarURLConnection extends URLConnection {
+    private final byte[] bytes;
+
+    private InternalJarURLConnection(final URL url, final byte[] bytes) {
+      super(url);
+      this.bytes = bytes;
+    }
+
+    @Override
+    public void connect() throws IOException {
+      connected = true;
+    }
+
+    @Override
+    public InputStream getInputStream() throws IOException {
+      return new ByteArrayInputStream(bytes);
+    }
+
+    @Override
+    public Permission getPermission() {
+      // No permissions needed because all classes are in memory
+      return null;
+    }
+  }
+}

dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/HttpUrlConnectionInstrumentation.java

@@ -80,19 +80,6 @@ public class HttpUrlConnectionInstrumentation extends Instrumenter.Default {
       final HttpUrlState state = contextStore.putIfAbsent(thiz, HttpUrlState.FACTORY);
 
       synchronized (state) {
-        /*
-         * AgentWriter uses HttpURLConnection to report to the trace-agent. We don't want to trace
-         * those requests. Check after the connected test above because getRequestProperty will
-         * throw an exception if already connected.
-         */
-        final boolean isTraceRequest =
-            Thread.currentThread().getName().equals("dd-agent-writer")
-                || (!connected && thiz.getRequestProperty("Datadog-Meta-Lang") != null);
-        if (isTraceRequest) {
-          state.finish();
-          return null;
-        }
-
         final int callDepth = CallDepthThreadLocalMap.incrementCallDepth(HttpURLConnection.class);
         if (callDepth > 0) {
           return null;

dd-java-agent/instrumentation/http-url-connection/src/main/java/datadog/trace/instrumentation/http_url_connection/UrlInstrumentation.java

@@ -12,11 +12,13 @@ import datadog.trace.agent.tooling.Instrumenter;
 import datadog.trace.api.Config;
 import datadog.trace.api.DDSpanTypes;
 import datadog.trace.api.DDTags;
+import datadog.trace.bootstrap.InternalJarURLHandler;
 import io.opentracing.Scope;
 import io.opentracing.Span;
 import io.opentracing.tag.Tags;
 import io.opentracing.util.GlobalTracer;
 import java.net.URL;
+import java.net.URLStreamHandler;
 import java.util.Collections;
 import java.util.Map;
 import net.bytebuddy.asm.Advice;
@@ -49,10 +51,14 @@ public class UrlInstrumentation extends Instrumenter.Default {
 
     @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class)
     public static void errorSpan(
-        @Advice.This final URL url, @Advice.Thrown final Throwable throwable) {
+        @Advice.This final URL url,
+        @Advice.Thrown final Throwable throwable,
+        @Advice.FieldValue("handler") final URLStreamHandler handler) {
       if (throwable != null) {
-        final boolean isTraceRequest = Thread.currentThread().getName().equals("dd-agent-writer");
+        // Various agent components end up calling `openConnection` indirectly
-        if (isTraceRequest) {
+        // when loading classes. Avoid tracing these calls.
+        final boolean disableTracing = handler instanceof InternalJarURLHandler;
+        if (disableTracing) {
           return;
         }
 

dd-smoke-tests/cli/src/test/groovy/datadog/smoketest/CliApplicationSmokeTest.groovy

@@ -6,7 +6,7 @@ import java.util.concurrent.TimeUnit
 
 class CliApplicationSmokeTest extends AbstractSmokeTest {
   // Estimate for the amount of time instrumentation, plus request, plus some extra
-  private static final int TIMEOUT_SECS = 15
+  private static final int TIMEOUT_SECS = 30
 
   @Override
   ProcessBuilder createProcessBuilder() {
@@ -20,6 +20,8 @@ class CliApplicationSmokeTest extends AbstractSmokeTest {
     processBuilder.directory(new File(buildDirectory))
   }
 
+  // TODO: once java7 support is dropped use waitFor() with timeout call added in java8
+  // instead of timeout on test
   @Timeout(value = TIMEOUT_SECS, unit = TimeUnit.SECONDS)
   def "Cli application process ends before timeout"() {
     expect:

dd-trace/src/main/java/datadog/trace/tracer/writer/AgentClient.java

@@ -20,6 +20,8 @@ import lombok.Getter;
 import lombok.extern.slf4j.Slf4j;
 import org.msgpack.jackson.dataformat.MessagePackFactory;
 
+// FIXME: this has to be migrated to okhttp before we can use this.
+// Otherwise http requests coming from here will get traced.
 @Slf4j
 class AgentClient {