Enable instrumentation of synthetic methods

bantonsson · bantonsson · commit c48213eb4fed · 2021-11-08T14:20:54.000+01:00
diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/AgentInstaller.java
@@ -5,6 +5,7 @@
 import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.nameStartsWith;
 import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.namedOneOf;
 import static net.bytebuddy.matcher.ElementMatchers.any;
+import static net.bytebuddy.matcher.ElementMatchers.isDefaultFinalizer;
 import static net.bytebuddy.matcher.ElementMatchers.none;
 
 import datadog.trace.agent.tooling.context.FieldBackedContextProvider;
@@ -20,12 +21,14 @@
 import java.util.ServiceLoader;
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArrayList;
+import net.bytebuddy.ByteBuddy;
 import net.bytebuddy.agent.builder.AgentBuilder;
 import net.bytebuddy.agent.builder.ResettableClassFileTransformer;
 import net.bytebuddy.description.type.TypeDefinition;
 import net.bytebuddy.description.type.TypeDescription;
 import net.bytebuddy.dynamic.DynamicType;
 import net.bytebuddy.matcher.ElementMatcher;
+import net.bytebuddy.matcher.LatentMatcher;
 import net.bytebuddy.utility.JavaModule;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -80,8 +83,12 @@ public static ResettableClassFileTransformer installBytebuddyAgent(
 
     FieldBackedContextProvider.resetContextMatchers();
 
+    // By default ByteBuddy will skip all methods that are synthetic or default finalizer
+    // but we need to instrument some synthetic methods in Scala, so change the ignore matcher
+    ByteBuddy byteBuddy =
+        new ByteBuddy().ignore(new LatentMatcher.Resolved<>(isDefaultFinalizer()));
     AgentBuilder.Ignored ignoredAgentBuilder =
-        new AgentBuilder.Default()
+        new AgentBuilder.Default(byteBuddy)
             .disableClassFormatChanges()
             .assureReadEdgeTo(INSTRUMENTATION, FieldBackedContextAccessor.class)
             .with(AgentTooling.transformerDecorator())
diff --git a/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java b/dd-java-agent/agent-tooling/src/main/java/datadog/trace/agent/tooling/Instrumenter.java
@@ -7,6 +7,7 @@
 import static java.util.Collections.singletonMap;
 import static net.bytebuddy.matcher.ElementMatchers.any;
 import static net.bytebuddy.matcher.ElementMatchers.isAnnotatedWith;
+import static net.bytebuddy.matcher.ElementMatchers.isSynthetic;
 import static net.bytebuddy.matcher.ElementMatchers.not;
 
 import datadog.trace.agent.tooling.bytebuddy.ExceptionHandlers;
@@ -160,7 +161,7 @@ public final AgentBuilder instrument(final AgentBuilder parentAgentBuilder) {
       if (transformer != null) {
         agentBuilder = agentBuilder.transform(transformer);
       }
-      AdviceBuilder adviceBuilder = new AdviceBuilder(agentBuilder);
+      AdviceBuilder adviceBuilder = new AdviceBuilder(agentBuilder, methodIgnoreMatcher());
       adviceTransformations(adviceBuilder);
       agentBuilder = adviceBuilder.agentBuilder;
       agentBuilder = contextProvider.additionalInstrumentation(agentBuilder);
@@ -202,9 +203,13 @@ private AgentBuilder.Identified.Extendable injectHelperClasses(
 
     private static class AdviceBuilder implements AdviceTransformation {
       AgentBuilder.Identified.Extendable agentBuilder;
+      final ElementMatcher<? super MethodDescription> ignoreMatcher;
 
-      public AdviceBuilder(AgentBuilder.Identified.Extendable agentBuilder) {
+      public AdviceBuilder(
+          AgentBuilder.Identified.Extendable agentBuilder,
+          ElementMatcher<? super MethodDescription> ignoreMatcher) {
         this.agentBuilder = agentBuilder;
+        this.ignoreMatcher = ignoreMatcher;
       }
 
       @Override
@@ -214,7 +219,7 @@ public void applyAdvice(ElementMatcher<? super MethodDescription> matcher, Strin
                 new AgentBuilder.Transformer.ForAdvice()
                     .include(Utils.getBootstrapProxy(), Utils.getAgentClassLoader())
                     .withExceptionHandler(ExceptionHandlers.defaultExceptionHandler())
-                    .advice(matcher, name));
+                    .advice(not(ignoreMatcher).and(matcher), name));
       }
     }
 
@@ -299,6 +304,14 @@ public AgentBuilder.Transformer transformer() {
       return null;
     }
 
+    /** @return A type matcher used to ignore some methods when applying transformation. */
+    public ElementMatcher<? super MethodDescription> methodIgnoreMatcher() {
+      // By default ByteBuddy will skip all methods that are synthetic at the top level, but since
+      // we need to instrument some synthetic methods in Scala and changed that, we make the default
+      // here to ignore synthetic methods to not change the behavior for unaware instrumentations
+      return isSynthetic();
+    }
+
     /**
      * Instrumenters should register each advice transformation by calling {@link
      * AdviceTransformation#applyAdvice(ElementMatcher, String)} one or more times.
diff --git a/dd-java-agent/testing/src/test/groovy/synthetic/SyntheticForkedTest.groovy b/dd-java-agent/testing/src/test/groovy/synthetic/SyntheticForkedTest.groovy
@@ -0,0 +1,39 @@
+package synthetic
+
+import datadog.trace.agent.test.AgentTestRunner
+
+abstract class SyntheticForkedTestBase extends AgentTestRunner {
+
+  def "test Synthetic methods"() {
+    expect:
+    SyntheticTestInstrumentation.Compute.result(i) == computed(i)
+
+    where:
+    i << [0, 1, 2, 3]
+  }
+
+  abstract int computed(int i)
+}
+
+class SyntheticForkedTest extends SyntheticForkedTestBase {
+
+  @Override
+  int computed(int i) {
+    return i * 2 + 1
+  }
+
+  @Override
+  protected void configurePreAgent() {
+    super.configurePreAgent()
+
+    System.setProperty("synthetic.test.enabled", "true")
+  }
+}
+
+class SyntheticDisabledForkedTest extends SyntheticForkedTestBase {
+
+  @Override
+  int computed(int i) {
+    return i + 1
+  }
+}
diff --git a/dd-java-agent/testing/src/test/java/synthetic/SyntheticTestInstrumentation.java b/dd-java-agent/testing/src/test/java/synthetic/SyntheticTestInstrumentation.java
@@ -0,0 +1,60 @@
+package synthetic;
+
+import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named;
+import static net.bytebuddy.matcher.ElementMatchers.none;
+
+import com.google.auto.service.AutoService;
+import datadog.trace.agent.tooling.Instrumenter;
+import net.bytebuddy.asm.Advice;
+import net.bytebuddy.description.method.MethodDescription;
+import net.bytebuddy.description.type.TypeDescription;
+import net.bytebuddy.matcher.ElementMatcher;
+
+@AutoService(Instrumenter.class)
+public class SyntheticTestInstrumentation extends Instrumenter.Tracing {
+
+  public SyntheticTestInstrumentation() {
+    super("synthetic-test");
+  }
+
+  @Override
+  public ElementMatcher<? super TypeDescription> typeMatcher() {
+    return named(getClass().getName() + "$WithSynthetic");
+  }
+
+  @Override
+  public ElementMatcher<? super MethodDescription> methodIgnoreMatcher() {
+    if (Boolean.getBoolean("synthetic.test.enabled")) {
+      return none();
+    } else {
+      return super.methodIgnoreMatcher();
+    }
+  }
+
+  @Override
+  public void adviceTransformations(AdviceTransformation transformation) {
+    transformation.applyAdvice(named("access$000"), getClass().getName() + "$AccessAdvice");
+  }
+
+  public static class Compute {
+    public static Integer result(Integer i) {
+      WithSynthetic p = new WithSynthetic(i);
+      return p.secret + 1;
+    }
+  }
+
+  private static final class WithSynthetic {
+    private Integer secret;
+
+    public WithSynthetic(Integer secret) {
+      this.secret = secret;
+    }
+  }
+
+  public static final class AccessAdvice {
+    @Advice.OnMethodExit(suppress = Throwable.class)
+    public static void exit(@Advice.Return(readOnly = false) Integer i) {
+      i = i * 2;
+    }
+  }
+}
