From 8d2f57b00177edd18f332a9e710f4ae61d4d7edd Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Tue, 25 Feb 2025 12:42:15 +0100 Subject: [PATCH 1/3] Fix Scalatest tracing for tests that are reported asynchronously --- .../scalatest/ScalatestInstrumentation.java | 28 ++++++++++++++++--- 1 file changed, 24 insertions(+), 4 deletions(-) diff --git a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java index b5ad3ee30cb..ed30b743e42 100644 --- a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java +++ b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java @@ -1,5 +1,6 @@ package datadog.trace.instrumentation.scalatest; +import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; @@ -7,13 +8,17 @@ import com.google.auto.service.AutoService; import datadog.trace.agent.tooling.Instrumenter; import datadog.trace.agent.tooling.InstrumenterModule; +import datadog.trace.bootstrap.CallDepthThreadLocalMap; import java.util.Set; import net.bytebuddy.asm.Advice; +import net.bytebuddy.description.type.TypeDescription; +import net.bytebuddy.matcher.ElementMatcher; +import org.scalatest.Reporter; import org.scalatest.events.Event; @AutoService(InstrumenterModule.class) public class ScalatestInstrumentation extends InstrumenterModule.CiVisibility - implements Instrumenter.ForSingleType, Instrumenter.HasMethodAdvice { + implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { public ScalatestInstrumentation() { super("ci-visibility", "scalatest"); @@ -25,8 +30,13 @@ public boolean isApplicable(Set enabledSystems) { } @Override - public String instrumentedType() { - return "org.scalatest.DispatchReporter"; + public String hierarchyMarkerType() { + return "org.scalatest.Reporter"; + } + + @Override + public ElementMatcher hierarchyMatcher() { + return implementsInterface(named(hierarchyMarkerType())); } @Override @@ -51,8 +61,13 @@ public void methodAdvice(MethodTransformer transformer) { public static class DispatchEventAdvice { @Advice.OnMethodEnter public static void onDispatchEvent(@Advice.Argument(value = 0) Event event) { + if (CallDepthThreadLocalMap.incrementCallDepth(Reporter.class) != 0) { + // nested call + return; + } + // Instead of registering our reporter using Scalatest's standard "-C" argument, - // we hook into internal dispatch reporter. + // we hook into internal reporter. // The reason is that Scalatest invokes registered reporters in a separate thread, // while we need to process events in the thread where they originate. // This is required because test span has to be active in the thread where @@ -61,5 +76,10 @@ public static void onDispatchEvent(@Advice.Argument(value = 0) Event event) { // could be properly associated with it. DatadogReporter.handle(event); } + + @Advice.OnMethodExit + public static void afterDispatchEvent() { + CallDepthThreadLocalMap.decrementCallDepth(Reporter.class); + } } } From f61d4e75f9137c949e4e121d410b5118e215c033 Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Tue, 25 Feb 2025 13:54:53 +0100 Subject: [PATCH 2/3] Fix duplicate events --- .../scalatest/ScalatestInstrumentation.java | 16 +++++----------- 1 file changed, 5 insertions(+), 11 deletions(-) diff --git a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java index ed30b743e42..d47500eb03f 100644 --- a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java +++ b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java @@ -1,6 +1,5 @@ package datadog.trace.instrumentation.scalatest; -import static datadog.trace.agent.tooling.bytebuddy.matcher.HierarchyMatchers.implementsInterface; import static datadog.trace.agent.tooling.bytebuddy.matcher.NameMatchers.named; import static net.bytebuddy.matcher.ElementMatchers.takesArgument; import static net.bytebuddy.matcher.ElementMatchers.takesArguments; @@ -11,14 +10,12 @@ import datadog.trace.bootstrap.CallDepthThreadLocalMap; import java.util.Set; import net.bytebuddy.asm.Advice; -import net.bytebuddy.description.type.TypeDescription; -import net.bytebuddy.matcher.ElementMatcher; import org.scalatest.Reporter; import org.scalatest.events.Event; @AutoService(InstrumenterModule.class) public class ScalatestInstrumentation extends InstrumenterModule.CiVisibility - implements Instrumenter.ForTypeHierarchy, Instrumenter.HasMethodAdvice { + implements Instrumenter.ForKnownTypes, Instrumenter.HasMethodAdvice { public ScalatestInstrumentation() { super("ci-visibility", "scalatest"); @@ -30,13 +27,10 @@ public boolean isApplicable(Set enabledSystems) { } @Override - public String hierarchyMarkerType() { - return "org.scalatest.Reporter"; - } - - @Override - public ElementMatcher hierarchyMatcher() { - return implementsInterface(named(hierarchyMarkerType())); + public String[] knownMatchingTypes() { + return new String[] { + "org.scalatest.DispatchReporter", "org.scalatest.tools.TestSortingReporter", + }; } @Override From 39af19185947181d3a766af1a1d3c6557429e5ee Mon Sep 17 00:00:00 2001 From: Nikita Tkachenko Date: Tue, 25 Feb 2025 14:10:56 +0100 Subject: [PATCH 3/3] Fix events duplication --- .../scalatest/ScalatestInstrumentation.java | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java index d47500eb03f..11919c9a080 100644 --- a/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java +++ b/dd-java-agent/instrumentation/scalatest/src/main/java/datadog/trace/instrumentation/scalatest/ScalatestInstrumentation.java @@ -50,6 +50,9 @@ public void methodAdvice(MethodTransformer transformer) { .and(takesArguments(1)) .and(takesArgument(0, named("org.scalatest.events.Event"))), ScalatestInstrumentation.class.getName() + "$DispatchEventAdvice"); + transformer.applyAdvice( + named("fireReadyEvents"), + ScalatestInstrumentation.class.getName() + "$SuppressAsyncEventsAdvice"); } public static class DispatchEventAdvice { @@ -76,4 +79,22 @@ public static void afterDispatchEvent() { CallDepthThreadLocalMap.decrementCallDepth(Reporter.class); } } + + /** + * {@link org.scalatest.tools.TestSortingReporter#fireReadyEvents} is triggered asynchronously. It + * fires some events that are then delegated to other reporters. We need to suppress them (by + * increasing the call depth so that {@link DispatchEventAdvice} is aborted) as the same events + * are reported earlier synchronously from {@link org.scalatest.tools.TestSortingReporter#apply} + */ + public static class SuppressAsyncEventsAdvice { + @Advice.OnMethodEnter + public static void onAsyncEventsTrigger() { + CallDepthThreadLocalMap.incrementCallDepth(Reporter.class); + } + + @Advice.OnMethodExit + public static void afterAsyncEventsTrigger() { + CallDepthThreadLocalMap.decrementCallDepth(Reporter.class); + } + } }