From bd2cd4d719690d5d6db211760ad76b41a2d5d8fe Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:14:55 -0500 Subject: [PATCH 01/15] Add java-concurrent as a dependency to all instrumentations --- dd-java-agent/instrumentation/instrumentation.gradle | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dd-java-agent/instrumentation/instrumentation.gradle b/dd-java-agent/instrumentation/instrumentation.gradle index 12e443dbf87..8b5325ca61d 100644 --- a/dd-java-agent/instrumentation/instrumentation.gradle +++ b/dd-java-agent/instrumentation/instrumentation.gradle @@ -56,6 +56,8 @@ subprojects { Project subProj -> annotationProcessor deps.autoservice implementation deps.autoservice + // Always include concurrent instrumentation dependency to make sure that all instrumentations are tested with async parts instrumented + testCompile project(':dd-java-agent:instrumentation:java-concurrent') testCompile project(':dd-java-agent:testing') testAnnotationProcessor deps.autoservice testImplementation deps.autoservice From 4c00594afb1e283291717f09a831e698f53ad6a8 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:15:14 -0500 Subject: [PATCH 02/15] Fix couchbase ignores --- .../matcher/AdditionalLibraryIgnoresMatcher.java | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index b97dfcf6802..a0bb524de90 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -29,7 +29,6 @@ public boolean matches(final T target) { if (name.startsWith("com.beust.jcommander.") || name.startsWith("com.carrotsearch.hppc.") - || name.startsWith("com.couchbase.client.deps.") || name.startsWith("com.fasterxml.classmate.") || name.startsWith("com.fasterxml.jackson.") || name.startsWith("com.github.mustachejava.") @@ -189,6 +188,17 @@ public boolean matches(final T target) { return true; } + if (name.startsWith("com.couchbase.client.deps.")) { + // Couchbase library includes some packaged dependencies, unfortunately some of them are + // instrumented by java-concurrent instrumentation + if (name.startsWith("com.couchbase.client.deps.io.netty.") + || name.startsWith("com.couchbase.client.deps.org.LatencyUtils.") + || name.startsWith("com.couchbase.client.deps.com.lmax.disruptor.")) { + return false; + } + return true; + } + if (name.startsWith("com.google.cloud.") || name.startsWith("com.google.instrumentation.") || name.startsWith("com.google.j2objc.") From 694b7293754e9088d818131a5cbf14928498d3e3 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:32:52 -0500 Subject: [PATCH 03/15] Stop skipping cassandra core instrumentation Looks like it has lots of runnables sprinkled all over the place --- .../bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java | 7 ------- 1 file changed, 7 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index a0bb524de90..097d0d51d9b 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -181,13 +181,6 @@ public boolean matches(final T target) { return true; } - if (name.startsWith("com.datastax.driver.")) { - if (name.startsWith("com.datastax.driver.core.Cluster$")) { - return false; - } - return true; - } - if (name.startsWith("com.couchbase.client.deps.")) { // Couchbase library includes some packaged dependencies, unfortunately some of them are // instrumented by java-concurrent instrumentation From c11eb875bb375837d766686827d4e3a95e1affca Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:34:30 -0500 Subject: [PATCH 04/15] Stop ignoring org.springframework.scheduling.annotation It has some runnables and it is not very big --- .../bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java | 1 - 1 file changed, 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index 097d0d51d9b..c1aea7a4ef6 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -63,7 +63,6 @@ public boolean matches(final T target) { || name.startsWith("org.springframework.objenesis.") || name.startsWith("org.springframework.orm.") || name.startsWith("org.springframework.remoting.") - || name.startsWith("org.springframework.scheduling.annotation") || name.startsWith("org.springframework.scripting.") || name.startsWith("org.springframework.stereotype.") || name.startsWith("org.springframework.transaction.") From d88dbe363717cb871df9623ea2ec70e17ab54a20 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:45:59 -0500 Subject: [PATCH 05/15] Fix google http client ignore matchers --- .../bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index c1aea7a4ef6..bac5b060b5b 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -218,7 +218,7 @@ public boolean matches(final T target) { return true; } if (name.startsWith("com.google.api.")) { - if (name.equals("com.google.api.client.http.HttpRequest")) { + if (name.startsWith("com.google.api.client.http.HttpRequest")) { return false; } return true; From fc26e0aebceefd401a05d0231f953c1d9cb5efc7 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:47:05 -0500 Subject: [PATCH 06/15] Remove asm matchers - asm is already matched elsewhere --- .../bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index bac5b060b5b..c58ad3835b0 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -39,14 +39,12 @@ public boolean matches(final T target) { || name.startsWith("org.apache.lucene") || name.startsWith("org.apache.tartarus") || name.startsWith("org.json.simple") - || name.startsWith("org.objectweb.asm.") || name.startsWith("org.yaml.snakeyaml")) { return true; } if (name.startsWith("org.springframework.")) { if (name.startsWith("org.springframework.aop.") - || name.startsWith("org.springframework.asm.") || name.startsWith("org.springframework.cache.") || name.startsWith("org.springframework.dao.") || name.startsWith("org.springframework.ejb.") From cec897efd7e0a88ff086e46ed13b105b62672344 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:47:39 -0500 Subject: [PATCH 07/15] Add missng dots --- .../matcher/AdditionalLibraryIgnoresMatcher.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index c58ad3835b0..1c1a1cf9caf 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -33,13 +33,13 @@ public boolean matches(final T target) { || name.startsWith("com.fasterxml.jackson.") || name.startsWith("com.github.mustachejava.") || name.startsWith("com.jayway.jsonpath.") - || name.startsWith("com.lightbend.lagom") + || name.startsWith("com.lightbend.lagom.") || name.startsWith("javax.el.") || name.startsWith("net.sf.cglib.") - || name.startsWith("org.apache.lucene") - || name.startsWith("org.apache.tartarus") - || name.startsWith("org.json.simple") - || name.startsWith("org.yaml.snakeyaml")) { + || name.startsWith("org.apache.lucene.") + || name.startsWith("org.apache.tartarus.") + || name.startsWith("org.json.simple.") + || name.startsWith("org.yaml.snakeyaml.")) { return true; } From 367fe1df2a42f0d8d4e55ce83a5c517904ac74c1 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 07:58:39 -0500 Subject: [PATCH 08/15] Do not ignore org.springframework.util.concurrent. --- .../matcher/AdditionalLibraryIgnoresMatcher.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index 1c1a1cf9caf..cba19a0c690 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -65,7 +65,6 @@ public boolean matches(final T target) { || name.startsWith("org.springframework.stereotype.") || name.startsWith("org.springframework.transaction.") || name.startsWith("org.springframework.ui.") - || name.startsWith("org.springframework.util.") || name.startsWith("org.springframework.validation.")) { return true; } @@ -136,6 +135,13 @@ public boolean matches(final T target) { return true; } + if (name.startsWith("org.springframework.util.")) { + if (name.startsWith("org.springframework.util.concurrent.")) { + return false; + } + return true; + } + if (name.startsWith("org.springframework.web.")) { if (name.startsWith("org.springframework.web.servlet.") || name.startsWith("org.springframework.web.reactive.")) { From a5c258c5a3b68e78b0809dc4cb0020e38027a783 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 08:14:51 -0500 Subject: [PATCH 09/15] One more exception for h2 --- .../bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index cba19a0c690..f726dae05d1 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -234,7 +234,8 @@ public boolean matches(final T target) { || name.startsWith("org.h2.jdbcx.") // Some runnables that get instrumented || name.equals("org.h2.store.FileLock") - || name.equals("org.h2.engine.DatabaseCloser")) { + || name.equals("org.h2.engine.DatabaseCloser") + || name.equals("org.h2.engine.OnExitDatabaseCloser")) { return false; } return true; From ff9a1713268bde185c2cbb22cb40197cd64b2fb0 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 08:39:26 -0500 Subject: [PATCH 10/15] More spring exceptions --- .../matcher/AdditionalLibraryIgnoresMatcher.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index f726dae05d1..70e1fce5980 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -96,7 +96,10 @@ public boolean matches(final T target) { if (name.startsWith("org.springframework.boot.")) { // More runnables to deal with if (name.startsWith("org.springframework.boot.autoconfigure.BackgroundPreinitializer$") - || name.startsWith("org.springframework.boot.web.embedded.netty.NettyWebServer$")) { + || name.startsWith("org.springframework.boot.autoconfigure.condition.OnClassCondition$") + || name.startsWith("org.springframework.boot.web.embedded.netty.NettyWebServer$") + || name.startsWith( + "org.springframework.boot.context.embedded.tomcat.TomcatEmbeddedServletContainer$")) { return false; } return true; @@ -144,7 +147,8 @@ public boolean matches(final T target) { if (name.startsWith("org.springframework.web.")) { if (name.startsWith("org.springframework.web.servlet.") - || name.startsWith("org.springframework.web.reactive.")) { + || name.startsWith("org.springframework.web.reactive.") + || name.startsWith("org.springframework.web.context.request.async.")) { return false; } return true; From f99df54138050773c611dac216c3a742711f2750 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 08:58:46 -0500 Subject: [PATCH 11/15] Exceptions for hppc --- .../matcher/AdditionalLibraryIgnoresMatcher.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index 70e1fce5980..b13c61f820c 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -28,7 +28,6 @@ public boolean matches(final T target) { final String name = target.getActualName(); if (name.startsWith("com.beust.jcommander.") - || name.startsWith("com.carrotsearch.hppc.") || name.startsWith("com.fasterxml.classmate.") || name.startsWith("com.fasterxml.jackson.") || name.startsWith("com.github.mustachejava.") @@ -245,6 +244,13 @@ public boolean matches(final T target) { return true; } + if (name.startsWith("com.carrotsearch.hppc.")) { + if (name.startsWith("com.carrotsearch.hppc.HashOrderMixing$")) { + return false; + } + return true; + } + // kotlin, note we do not ignore kotlinx because we instrument coroutins code if (name.startsWith("kotlin.")) { return true; From a822bcda33438b175fceb145a333ce8d1ce3aa53 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 09:21:02 -0500 Subject: [PATCH 12/15] Split Runnable and Callable instrumentations Otherwise `Runnable` may define `call` method leading to instrumentation exception. See `org.h2.util.Task` as an example. --- ...tion.java => CallableInstrumentation.java} | 27 +------ .../concurrent/RunnableInstrumentation.java | 74 +++++++++++++++++++ 2 files changed, 78 insertions(+), 23 deletions(-) rename dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/{RunnableCallableInstrumentation.java => CallableInstrumentation.java} (69%) create mode 100644 dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableInstrumentation.java diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableCallableInstrumentation.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/CallableInstrumentation.java similarity index 69% rename from dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableCallableInstrumentation.java rename to dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/CallableInstrumentation.java index 4795056c849..bced21fffca 100644 --- a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableCallableInstrumentation.java +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/CallableInstrumentation.java @@ -24,15 +24,15 @@ /** Instrument {@link Runnable} and {@Callable} */ @Slf4j @AutoService(Instrumenter.class) -public final class RunnableCallableInstrumentation extends Instrumenter.Default { +public final class CallableInstrumentation extends Instrumenter.Default { - public RunnableCallableInstrumentation() { + public CallableInstrumentation() { super(AbstractExecutorInstrumentation.EXEC_NAME); } @Override public ElementMatcher typeMatcher() { - return implementsInterface(named(Runnable.class.getName()).or(named(Callable.class.getName()))); + return implementsInterface(named(Callable.class.getName())); } @Override @@ -45,7 +45,6 @@ public String[] helperClassNames() { @Override public Map contextStore() { final Map map = new HashMap<>(); - map.put(Runnable.class.getName(), State.class.getName()); map.put(Callable.class.getName(), State.class.getName()); return Collections.unmodifiableMap(map); } @@ -53,30 +52,12 @@ public Map contextStore() { @Override public Map, String> transformers() { final Map, String> transformers = new HashMap<>(); - transformers.put( - named("run").and(takesArguments(0)).and(isPublic()), - RunnableCallableInstrumentation.class.getName() + "$RunnableAdvice"); transformers.put( named("call").and(takesArguments(0)).and(isPublic()), - RunnableCallableInstrumentation.class.getName() + "$CallableAdvice"); + CallableInstrumentation.class.getName() + "$CallableAdvice"); return transformers; } - public static class RunnableAdvice { - - @Advice.OnMethodEnter(suppress = Throwable.class) - public static TraceScope enter(@Advice.This final Runnable thiz) { - final ContextStore contextStore = - InstrumentationContext.get(Runnable.class, State.class); - return AdviceUtils.startTaskScope(contextStore, thiz); - } - - @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) - public static void exit(@Advice.Enter final TraceScope scope) { - AdviceUtils.endTaskScope(scope); - } - } - public static class CallableAdvice { @Advice.OnMethodEnter(suppress = Throwable.class) diff --git a/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableInstrumentation.java b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableInstrumentation.java new file mode 100644 index 00000000000..2ae88a96f74 --- /dev/null +++ b/dd-java-agent/instrumentation/java-concurrent/src/main/java/datadog/trace/instrumentation/java/concurrent/RunnableInstrumentation.java @@ -0,0 +1,74 @@ +package datadog.trace.instrumentation.java.concurrent; + +import static datadog.trace.agent.tooling.bytebuddy.matcher.DDElementMatchers.implementsInterface; +import static net.bytebuddy.matcher.ElementMatchers.isPublic; +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +import com.google.auto.service.AutoService; +import datadog.trace.agent.tooling.Instrumenter; +import datadog.trace.bootstrap.ContextStore; +import datadog.trace.bootstrap.InstrumentationContext; +import datadog.trace.bootstrap.instrumentation.java.concurrent.State; +import datadog.trace.context.TraceScope; +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import lombok.extern.slf4j.Slf4j; +import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.method.MethodDescription; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; + +/** Instrument {@link Runnable} and {@Callable} */ +@Slf4j +@AutoService(Instrumenter.class) +public final class RunnableInstrumentation extends Instrumenter.Default { + + public RunnableInstrumentation() { + super(AbstractExecutorInstrumentation.EXEC_NAME); + } + + @Override + public ElementMatcher typeMatcher() { + return implementsInterface(named(Runnable.class.getName())); + } + + @Override + public String[] helperClassNames() { + return new String[] { + AdviceUtils.class.getName(), + }; + } + + @Override + public Map contextStore() { + final Map map = new HashMap<>(); + map.put(Runnable.class.getName(), State.class.getName()); + return Collections.unmodifiableMap(map); + } + + @Override + public Map, String> transformers() { + final Map, String> transformers = new HashMap<>(); + transformers.put( + named("run").and(takesArguments(0)).and(isPublic()), + RunnableInstrumentation.class.getName() + "$RunnableAdvice"); + return transformers; + } + + public static class RunnableAdvice { + + @Advice.OnMethodEnter(suppress = Throwable.class) + public static TraceScope enter(@Advice.This final Runnable thiz) { + final ContextStore contextStore = + InstrumentationContext.get(Runnable.class, State.class); + return AdviceUtils.startTaskScope(contextStore, thiz); + } + + @Advice.OnMethodExit(onThrowable = Throwable.class, suppress = Throwable.class) + public static void exit(@Advice.Enter final TraceScope scope) { + AdviceUtils.endTaskScope(scope); + } + } +} From f9d94dc5ba1c16ec9376c83be0a09d1b23549e81 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 09:22:19 -0500 Subject: [PATCH 13/15] Unignore `org.h2.util.Task` --- .../bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java | 1 + 1 file changed, 1 insertion(+) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index b13c61f820c..8938230d278 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -236,6 +236,7 @@ public boolean matches(final T target) { || name.startsWith("org.h2.jdbc.") || name.startsWith("org.h2.jdbcx.") // Some runnables that get instrumented + || name.equals("org.h2.util.Task") || name.equals("org.h2.store.FileLock") || name.equals("org.h2.engine.DatabaseCloser") || name.equals("org.h2.engine.OnExitDatabaseCloser")) { From c152952db4cc7029f78f70a3e62ac49a594426c5 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Thu, 5 Mar 2020 11:29:17 -0500 Subject: [PATCH 14/15] Do not enabled async propagation in grpc scopes Grpc lib uses executors to schedule to idle/delayed tasks. These tasks become part of the trace if async propagation is enabled causing trace to never finish. --- .../grpc/client/TracingClientInterceptor.java | 6 ------ .../grpc/server/TracingServerInterceptor.java | 14 -------------- 2 files changed, 20 deletions(-) diff --git a/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/client/TracingClientInterceptor.java b/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/client/TracingClientInterceptor.java index 5102dda936d..c99f0e50120 100644 --- a/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/client/TracingClientInterceptor.java +++ b/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/client/TracingClientInterceptor.java @@ -33,7 +33,6 @@ public ClientCall interceptCall( startSpan("grpc.client").setTag(DDTags.RESOURCE_NAME, method.getFullMethodName()); try (final AgentScope scope = activateSpan(span, false)) { DECORATE.afterStart(span); - scope.setAsyncPropagation(true); final ClientCall result; try { @@ -64,7 +63,6 @@ public void start(final Listener responseListener, final Metadata headers propagate().inject(span, headers, SETTER); try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); super.start(new TracingClientCallListener<>(span, responseListener), headers); } catch (final Throwable e) { DECORATE.onError(span, e); @@ -77,7 +75,6 @@ public void start(final Listener responseListener, final Metadata headers @Override public void sendMessage(final ReqT message) { try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); super.sendMessage(message); } catch (final Throwable e) { DECORATE.onError(span, e); @@ -104,7 +101,6 @@ public void onMessage(final RespT message) { .setTag("message.type", message.getClass().getName()); DECORATE.afterStart(messageSpan); final AgentScope scope = activateSpan(messageSpan, true); - scope.setAsyncPropagation(true); try { delegate().onMessage(message); } catch (final Throwable e) { @@ -121,7 +117,6 @@ public void onClose(final Status status, final Metadata trailers) { DECORATE.onClose(span, status); // Finishes span. try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().onClose(status, trailers); } catch (final Throwable e) { DECORATE.onError(span, e); @@ -135,7 +130,6 @@ public void onClose(final Status status, final Metadata trailers) { @Override public void onReady() { try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().onReady(); } catch (final Throwable e) { DECORATE.onError(span, e); diff --git a/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java b/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java index 57cb0e8cc7e..7335c733a2a 100644 --- a/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java +++ b/dd-java-agent/instrumentation/grpc-1.5/src/main/java/datadog/trace/instrumentation/grpc/server/TracingServerInterceptor.java @@ -37,7 +37,6 @@ public ServerCall.Listener interceptCall( DECORATE.afterStart(span); final AgentScope scope = activateSpan(span, false); - scope.setAsyncPropagation(true); final ServerCall.Listener result; try { @@ -53,7 +52,6 @@ public ServerCall.Listener interceptCall( span.finish(); throw e; } finally { - scope.setAsyncPropagation(false); scope.close(); } @@ -74,9 +72,7 @@ static final class TracingServerCall public void close(final Status status, final Metadata trailers) { DECORATE.onClose(span, status); try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().close(status, trailers); - scope.setAsyncPropagation(false); } catch (final Throwable e) { DECORATE.onError(span, e); throw e; @@ -100,7 +96,6 @@ public void onMessage(final ReqT message) { .setTag("message.type", message.getClass().getName()); DECORATE.afterStart(span); final AgentScope scope = activateSpan(span, true); - scope.setAsyncPropagation(true); try { delegate().onMessage(message); } catch (final Throwable e) { @@ -109,7 +104,6 @@ public void onMessage(final ReqT message) { this.span.finish(); throw e; } finally { - scope.setAsyncPropagation(false); DECORATE.beforeFinish(span); scope.close(); } @@ -118,9 +112,7 @@ public void onMessage(final ReqT message) { @Override public void onHalfClose() { try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().onHalfClose(); - scope.setAsyncPropagation(false); } catch (final Throwable e) { DECORATE.onError(span, e); DECORATE.beforeFinish(span); @@ -133,10 +125,8 @@ public void onHalfClose() { public void onCancel() { // Finishes span. try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().onCancel(); span.setTag("canceled", true); - scope.setAsyncPropagation(false); } catch (final Throwable e) { DECORATE.onError(span, e); throw e; @@ -150,9 +140,7 @@ public void onCancel() { public void onComplete() { // Finishes span. try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().onComplete(); - scope.setAsyncPropagation(false); } catch (final Throwable e) { DECORATE.onError(span, e); throw e; @@ -165,9 +153,7 @@ public void onComplete() { @Override public void onReady() { try (final AgentScope scope = activateSpan(span, false)) { - scope.setAsyncPropagation(true); delegate().onReady(); - scope.setAsyncPropagation(false); } catch (final Throwable e) { DECORATE.onError(span, e); DECORATE.beforeFinish(span); From 9ea57bf56039f0d8765bc6a3acde344ff448e019 Mon Sep 17 00:00:00 2001 From: Nikolay Martynov Date: Wed, 4 Mar 2020 14:34:36 -0500 Subject: [PATCH 15/15] More ignores --- .../AdditionalLibraryIgnoresMatcher.java | 80 ++++++++++++++++++- .../matcher/GlobalIgnoresMatcher.java | 8 -- .../log4j2/ThreadContextInstrumentation.java | 2 + 3 files changed, 81 insertions(+), 9 deletions(-) diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java index 8938230d278..7245b2d70a4 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/AdditionalLibraryIgnoresMatcher.java @@ -1,5 +1,6 @@ package datadog.trace.agent.tooling.bytebuddy.matcher; +import java.util.regex.Pattern; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -14,6 +15,9 @@ public class AdditionalLibraryIgnoresMatcher extends ElementMatcher.Junction.AbstractBase { + private static final Pattern COM_MCHANGE_PROXY = + Pattern.compile("com\\.mchange\\.v2\\.c3p0\\..*Proxy"); + public static Junction additionalLibraryIgnoresMatcher() { return new AdditionalLibraryIgnoresMatcher<>(); } @@ -33,11 +37,28 @@ public boolean matches(final T target) { || name.startsWith("com.github.mustachejava.") || name.startsWith("com.jayway.jsonpath.") || name.startsWith("com.lightbend.lagom.") + || name.startsWith("com.netflix.hystrix.") // test this + || name.startsWith("io.micrometer.") + || name.startsWith("io.github.classgraph.") || name.startsWith("javax.el.") + || name.startsWith("javax.crypto.") + || name.startsWith("javax.cache.") + || name.startsWith("javax.management.") + || name.startsWith("javax.persistence.") || name.startsWith("net.sf.cglib.") + || name.startsWith("nonapi.io.github.classgraph.") + || name.startsWith("org.apache.commons.") + || name.startsWith("org.apache.coyote.") + || name.startsWith("org.apache.el.") + || name.startsWith("org.apache.juli.") || name.startsWith("org.apache.lucene.") + || name.startsWith("org.apache.naming.") || name.startsWith("org.apache.tartarus.") + || name.startsWith("org.ehcache.") + || name.startsWith("org.hibernate.") // test this || name.startsWith("org.json.simple.") + || name.startsWith("org.msgpack.") + || name.startsWith("org.thymeleaf.") || name.startsWith("org.yaml.snakeyaml.")) { return true; } @@ -56,6 +77,7 @@ public boolean matches(final T target) { || name.startsWith("org.springframework.jmx.") || name.startsWith("org.springframework.jndi.") || name.startsWith("org.springframework.lang.") + || name.startsWith("org.springframework.mail.") || name.startsWith("org.springframework.messaging.") || name.startsWith("org.springframework.objenesis.") || name.startsWith("org.springframework.orm.") @@ -166,7 +188,10 @@ public boolean matches(final T target) { || name.startsWith("org.apache.xerces.") || name.startsWith("org.apache.xml.") || name.startsWith("org.apache.xpath.") - || name.startsWith("org.xml.")) { + || name.startsWith("org.xml.") + || name.startsWith("com.sun.org.apache.xerces.") + || name.startsWith("com.sun.org.apache.xalan.") + || name.startsWith("com.sun.xml.")) { return true; } @@ -252,11 +277,64 @@ public boolean matches(final T target) { return true; } + if (name.startsWith("okio.")) { + if (name.equals("okio.AsyncTimeout$Watchdog")) { + return false; + } + return true; + } + + if (name.startsWith("org.hsqldb.")) { + if (name.startsWith("org.hsqldb.jdbc.")) { + return false; + } + return true; + } + + if (name.startsWith("org.apache.catalina.")) { + if (name.startsWith("org.apache.catalina.connector.") + || name.startsWith("org.apache.catalina.core.") + || name.startsWith("org.apache.catalina.servlets.")) { + return false; + } + return true; + } + + if (name.startsWith("org.apache.logging.")) { + // We instrument single class in the whole library + // But unfortunately we also instrument some runnables + if (name.equals("org.apache.logging.log4j.ThreadContext") + || name.equals("org.apache.logging.log4j.core.util.DefaultShutdownCallbackRegistry")) { + return false; + } + return true; + } + + if (name.startsWith("org.apache.tomcat.")) { + if (name.startsWith("org.apache.tomcat.jdbc.")) { + return false; + } + return true; + } + + if (name.startsWith("io.grpc.")) { + // We instrument two specific classes + if (name.startsWith("io.grpc.internal.AbstractManagedChannelImplBuilder") + || name.startsWith("io.grpc.internal.AbstractServerImplBuilder")) { + return false; + } + return true; + } + // kotlin, note we do not ignore kotlinx because we instrument coroutins code if (name.startsWith("kotlin.")) { return true; } + if (COM_MCHANGE_PROXY.matcher(name).matches()) { + return true; + } + return false; } diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/GlobalIgnoresMatcher.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/GlobalIgnoresMatcher.java index eada68fd9c4..750846a6c71 100644 --- a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/GlobalIgnoresMatcher.java +++ b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/bytebuddy/matcher/GlobalIgnoresMatcher.java @@ -1,6 +1,5 @@ package datadog.trace.agent.tooling.bytebuddy.matcher; -import java.util.regex.Pattern; import net.bytebuddy.description.type.TypeDescription; import net.bytebuddy.matcher.ElementMatcher; @@ -23,9 +22,6 @@ public class GlobalIgnoresMatcher extends ElementMatcher.Junction.AbstractBase { - private static final Pattern COM_MCHANGE_PROXY = - Pattern.compile("com\\.mchange\\.v2\\.c3p0\\..*Proxy"); - public static ElementMatcher.Junction globalIgnoresMatcher() { return new GlobalIgnoresMatcher<>(); } @@ -144,10 +140,6 @@ public boolean matches(final T target) { return true; } - if (COM_MCHANGE_PROXY.matcher(name).matches()) { - return true; - } - if (additionalLibraryIgnoreMatcher.matches(target)) { return true; } diff --git a/dd-java-agent/instrumentation/log4j2/src/main/java/datadog/trace/instrumentation/log4j2/ThreadContextInstrumentation.java b/dd-java-agent/instrumentation/log4j2/src/main/java/datadog/trace/instrumentation/log4j2/ThreadContextInstrumentation.java index 88048bc1fad..27f2b85d1b2 100644 --- a/dd-java-agent/instrumentation/log4j2/src/main/java/datadog/trace/instrumentation/log4j2/ThreadContextInstrumentation.java +++ b/dd-java-agent/instrumentation/log4j2/src/main/java/datadog/trace/instrumentation/log4j2/ThreadContextInstrumentation.java @@ -29,6 +29,8 @@ protected boolean defaultEnabled() { return Config.get().isLogsInjectionEnabled(); } + // We instrument single class in the whole library, the rest is ignored by + // AdditionalLibraryIgnoresMatcher @Override public ElementMatcher typeMatcher() { return named("org.apache.logging.log4j.ThreadContext");