Enforce continuation diagnostics in suite fixtures

.agents/skills/fix-continuation-leakage/SKILL.md

@@ -69,11 +69,12 @@ set -o pipefail
    separately from trace-count or arrival-order assertions; fixing a leak may expose an unrelated
    flaky assertion.
 
-## Fixture setup failures
+## Fixture failures
 
-Automatic recording may start after `setupSpec()` or equivalent fixture initialization. For an
-initialization error or a trace wait inside setup, temporarily record around that setup block and
-remove the diagnostic scaffolding after finding the owner.
+Automatic recording covers Spock `setupSpec()` / `cleanupSpec()` and JUnit `@BeforeAll` /
+`@AfterAll`, in addition to per-test setup and cleanup. The failure output identifies whether the
+problem belongs to suite setup, one test, or suite cleanup. Code that runs before the
+instrumentation-test harness initializes the tracer remains outside this window.
 
 Apply process-wide configuration before starting servers, actor systems, executors, or other
 long-lived fixtures. Use a forked test or recreate the fixture when its static state cannot be

dd-java-agent/instrumentation-testing/build.gradle

@@ -23,6 +23,7 @@ dependencies {
   testImplementation group: 'cglib', name: 'cglib', version: '3.2.5'
   // test instrumenting java 1.1 bytecode
   testImplementation group: 'net.sf.jt400', name: 'jt400', version: '6.1'
+  testImplementation libs.bundles.mockito
 
   // We have autoservices defined in test subtree, looks like we need this to be able to properly rebuild this
   testAnnotationProcessor libs.autoservice.processor

dd-java-agent/instrumentation-testing/src/main/groovy/datadog/trace/agent/test/InstrumentationSpecification.groovy

@@ -34,6 +34,7 @@ import datadog.metrics.impl.MonitoringImpl
 import datadog.trace.agent.test.asserts.ListWriterAssert
 import datadog.trace.agent.test.asserts.TagsAssert
 import datadog.trace.agent.test.scopediag.ScopeDiagnostics
+import datadog.trace.agent.test.scopediag.ScopeDiagnosticsSpockSupport
 import datadog.trace.agent.test.scopediag.TrackScopeContinuations
 import datadog.trace.agent.test.datastreams.MockFeaturesDiscovery
 import datadog.trace.agent.test.datastreams.RecordingDatastreamsPayloadWriter
@@ -113,7 +114,8 @@ import spock.lang.Shared
 @SuppressWarnings('UnnecessaryDotClass')
 @ExtendWith(TestClassShadowingExtension.class)
 @ExtendWith(TooManyInvocationsErrorHandler.class)
-abstract class InstrumentationSpecification extends DDSpecification implements AgentBuilder.Listener {
+@TrackScopeContinuations
+abstract class InstrumentationSpecification extends DDSpecification implements AgentBuilder.Listener, ScopeDiagnosticsSpockSupport {
   private static final long TIMEOUT_MILLIS = TimeUnit.SECONDS.toMillis(20)
 
   protected static final Instrumentation INSTRUMENTATION = ByteBuddyAgent.getInstrumentation()
@@ -189,6 +191,9 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
   @Shared
   boolean isLatestDepTest = Boolean.getBoolean('test.dd.latestDepTest')
 
+  @Shared
+  boolean scopeDiagnosticsSuiteSetupPending
+
   @SuppressWarnings('PropertyName')
   @Shared
   TraceConfig MOCK_DSM_TRACE_CONFIG = new TraceConfig() {
@@ -427,6 +432,11 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
 
     // check for instrumentation issues during installation
     assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors()
+
+    if (scopeDiagnosticsSuiteEnabled()) {
+      ScopeDiagnostics.startRecording()
+      scopeDiagnosticsSuiteSetupPending = true
+    }
   }
 
   protected String idGenerationStrategyName() {
@@ -442,6 +452,18 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
   }
 
   void setup() {
+    if (scopeDiagnosticsSuiteEnabled()) {
+      if (scopeDiagnosticsSuiteSetupPending) {
+        scopeDiagnosticsSuiteSetupPending = false
+        def suiteSetupFailure = reportScopeDiagnosticsForPhase(scopeDiagClassConfig(), "suite setup")
+        if (suiteSetupFailure != null) {
+          throw suiteSetupFailure
+        }
+      } else {
+        ScopeDiagnostics.reset()
+      }
+    }
+
     InstrumentationErrors.resetErrors() // reset for each test
 
     configureLoggingLevels()
@@ -492,51 +514,57 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
   }
 
   void cleanup() {
-    if (isTestAgentEnabled()) {
-      // save Datadog environment to DDAgentWriter header
-      addEnvironmentVariablesToHeaders(TEST_AGENT_API)
-
-      // write ListWriter traces to the AgentWriter at cleanup so trace-processing changes occur after span assertions
-      def traces = TEST_WRITER.toArray()
-      for (trace in traces) {
-        TEST_AGENT_WRITER.write(trace as List<DDSpan>)
+    try {
+      if (isTestAgentEnabled()) {
+        // save Datadog environment to DDAgentWriter header
+        addEnvironmentVariablesToHeaders(TEST_AGENT_API)
+
+        // write ListWriter traces to the AgentWriter at cleanup so trace-processing changes occur after span assertions
+        def traces = TEST_WRITER.toArray()
+        for (trace in traces) {
+          TEST_AGENT_WRITER.write(trace as List<DDSpan>)
+        }
+        TEST_AGENT_WRITER.flush()
       }
-      TEST_AGENT_WRITER.flush()
-    }
-    TEST_TRACER.flush()
+      TEST_TRACER.flush()
 
-    def scopeDiagnosticsFailure = reportScopeDiagnostics()
+      def scopeDiagnosticsFailure = reportScopeDiagnostics()
 
-    try {
-      def util = new MockUtil()
-      util.detachMock(STATS_D_CLIENT)
+      try {
+        def util = new MockUtil()
+        util.detachMock(STATS_D_CLIENT)
 
-      ActiveSubsystems.APPSEC_ACTIVE = originalAppSecRuntimeValue
+        ActiveSubsystems.APPSEC_ACTIVE = originalAppSecRuntimeValue
 
-      if (Config.get().isDebuggerCodeOriginEnabled()) {
-        injectSysConfig(CODE_ORIGIN_FOR_SPANS_ENABLED, "false", true)
-        rebuildConfig()
-      }
+        if (Config.get().isDebuggerCodeOriginEnabled()) {
+          injectSysConfig(CODE_ORIGIN_FOR_SPANS_ENABLED, "false", true)
+          rebuildConfig()
+        }
 
-      try {
-        if (enabledFinishTimingChecks()) {
-          doCheckRepeatedFinish()
+        try {
+          if (enabledFinishTimingChecks()) {
+            doCheckRepeatedFinish()
+          }
+        } finally {
+          spanFinishLocations.clear()
+          originalToTrackingSpan.clear()
         }
-      } finally {
-        spanFinishLocations.clear()
-        originalToTrackingSpan.clear()
-      }
 
-      // check for instrumentation issues while running each test
-      assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors()
-    } catch (Throwable cleanupFailure) {
+        // check for instrumentation issues while running each test
+        assert InstrumentationErrors.noErrors(): InstrumentationErrors.describeErrors()
+      } catch (Throwable cleanupFailure) {
+        if (scopeDiagnosticsFailure != null) {
+          cleanupFailure.addSuppressed(scopeDiagnosticsFailure)
+        }
+        throw cleanupFailure
+      }
       if (scopeDiagnosticsFailure != null) {
-        cleanupFailure.addSuppressed(scopeDiagnosticsFailure)
+        throw scopeDiagnosticsFailure
+      }
+    } finally {
+      if (scopeDiagnosticsSuiteEnabled()) {
+        ScopeDiagnostics.startRecording()
       }
-      throw cleanupFailure
-    }
-    if (scopeDiagnosticsFailure != null) {
-      throw scopeDiagnosticsFailure
     }
   }
 
@@ -549,12 +577,23 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
     return ann
   }
 
+  private TrackScopeContinuations scopeDiagClassConfig() {
+    return this.class.getAnnotation(TrackScopeContinuations)
+  }
+
+  private boolean scopeDiagnosticsSuiteEnabled() {
+    return ScopeDiagnostics.isEnabled(scopeDiagClassConfig())
+  }
+
   private boolean scopeDiagnosticsEnabled() {
     return ScopeDiagnostics.isEnabled(scopeDiagConfig())
   }
 
   private Throwable reportScopeDiagnostics() {
-    def config = scopeDiagConfig()
+    return reportScopeDiagnosticsForPhase(scopeDiagConfig(), null)
+  }
+
+  private Throwable reportScopeDiagnosticsForPhase(TrackScopeContinuations config, String phase) {
     if (!ScopeDiagnostics.isEnabled(config)) {
       return null
     }
@@ -563,6 +602,9 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
       ScopeDiagnostics.stop()
       def report = ScopeDiagnostics.report()
       if (report.hasFindings()) {
+        if (phase != null) {
+          println("Scope diagnostics for ${phase}:")
+        }
         println(report.renderTimeline())
       }
       ScopeDiagnostics.assertNoLeaks(report)
@@ -574,6 +616,23 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
     }
   }
 
+  @Override
+  void onSuiteSetupFailure() {
+    if (!scopeDiagnosticsSuiteSetupPending) {
+      return
+    }
+    scopeDiagnosticsSuiteSetupPending = false
+    def diagnosticFailure = reportScopeDiagnosticsForPhase(scopeDiagClassConfig(), "suite setup")
+    try {
+      if (diagnosticFailure != null) {
+        throw diagnosticFailure
+      }
+    } finally {
+      // A pending setup window implies suite diagnostics are enabled.
+      ScopeDiagnostics.startRecording()
+    }
+  }
+
   private void doCheckRepeatedFinish() {
     for (Map.Entry<DDSpan, List<Exception>> entry: this.spanFinishLocations.entrySet()) {
       if (entry.value.size() == 1) {
@@ -602,20 +661,31 @@ abstract class InstrumentationSpecification extends DDSpecification implements A
   protected void cleanupAfterAgent() {}
 
   void cleanupSpec() {
-    TEST_TRACER?.close()
-    TEST_AGENT_WRITER?.close()
+    def scopeDiagnosticsFailure = reportScopeDiagnosticsForPhase(scopeDiagClassConfig(), "suite cleanup")
+    try {
+      TEST_TRACER?.close()
+      TEST_AGENT_WRITER?.close()
 
-    if (null != activeTransformer) {
-      INSTRUMENTATION.removeTransformer(activeTransformer)
-      activeTransformer = null
-    }
+      if (null != activeTransformer) {
+        INSTRUMENTATION.removeTransformer(activeTransformer)
+        activeTransformer = null
+      }
 
-    cleanupAfterAgent()
+      cleanupAfterAgent()
 
-    // All cleanup should happen before these assertion.  If not, a failing assertion may prevent cleanup
-    assert TRANSFORMED_CLASSES_TYPES.findAll {
-      GlobalIgnores.isAdditionallyIgnored(it.getActualName())
-    }.isEmpty(): "Transformed classes match global libraries ignore matcher"
+      // All cleanup should happen before these assertion.  If not, a failing assertion may prevent cleanup
+      assert TRANSFORMED_CLASSES_TYPES.findAll {
+        GlobalIgnores.isAdditionallyIgnored(it.getActualName())
+      }.isEmpty(): "Transformed classes match global libraries ignore matcher"
+    } catch (Throwable cleanupFailure) {
+      if (scopeDiagnosticsFailure != null) {
+        cleanupFailure.addSuppressed(scopeDiagnosticsFailure)
+      }
+      throw cleanupFailure
+    }
+    if (scopeDiagnosticsFailure != null) {
+      throw scopeDiagnosticsFailure
+    }
   }
 
   boolean useStrictTraceWrites() {

dd-java-agent/instrumentation-testing/src/main/java/datadog/trace/agent/test/AbstractInstrumentationTest.java

@@ -7,7 +7,7 @@
 import datadog.instrument.classinject.ClassInjector;
 import datadog.trace.agent.test.assertions.TraceAssertions;
 import datadog.trace.agent.test.assertions.TraceMatcher;
-import datadog.trace.agent.test.scopediag.ScopeDiagnosticsExtension;
+import datadog.trace.agent.test.scopediag.TrackScopeContinuations;
 import datadog.trace.agent.tooling.AgentInstaller;
 import datadog.trace.agent.tooling.InstrumenterModule;
 import datadog.trace.agent.tooling.TracerInstaller;
@@ -56,11 +56,11 @@
  * </ul>
  */
 @WithConfig(key = "detailed.instrumentation.errors", value = "true")
+@TrackScopeContinuations
 @ExtendWith({
   TestClassShadowingExtension.class,
   AllowContextTestingExtension.class,
-  LegacyContextTestingExtension.class,
-  ScopeDiagnosticsExtension.class
+  LegacyContextTestingExtension.class
 })
 public abstract class AbstractInstrumentationTest {
   static final Instrumentation INSTRUMENTATION = ByteBuddyAgent.getInstrumentation();