Merge pull request #6829 from DataDog/jpbempel/async-instrumentation

Make Exception debugging instrumentation async

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/DefaultExceptionDebugger.java

@@ -11,6 +11,7 @@
 import com.datadog.debugger.util.ExceptionHelper;
 import datadog.trace.bootstrap.debugger.DebuggerContext;
 import datadog.trace.bootstrap.instrumentation.api.AgentSpan;
+import datadog.trace.util.AgentTaskScheduler;
 import java.util.List;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -66,11 +67,15 @@ public void handleException(Throwable t, AgentSpan span) {
     } else {
       exceptionProbeManager.createProbesForException(
           fingerprint, innerMostException.getStackTrace());
-      // TODO make it async
-      configurationUpdater.accept(EXCEPTION, exceptionProbeManager.getProbes());
+      AgentTaskScheduler.INSTANCE.execute(() -> applyExceptionConfiguration(fingerprint));
     }
   }
 
+  private void applyExceptionConfiguration(String fingerprint) {
+    configurationUpdater.accept(EXCEPTION, exceptionProbeManager.getProbes());
+    exceptionProbeManager.addFingerprint(fingerprint);
+  }
+
   private static void processSnapshotsAndSetTags(
       Throwable t, AgentSpan span, ThrowableState state, Throwable innerMostException) {
     if (span.getTag(DD_DEBUG_ERROR_EXCEPTION_ID) != null) {

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java

@@ -56,6 +56,9 @@ public void createProbesForException(String fingerprint, StackTraceElement[] sta
       ExceptionProbe probe = createMethodProbe(this, where);
       probes.putIfAbsent(probe.getId(), probe);
     }
+  }
+
+  void addFingerprint(String fingerprint) {
     fingerprints.add(fingerprint);
   }
 
@@ -78,10 +81,6 @@ public boolean shouldCaptureException(String fingerprint) {
     return fingerprints.contains(fingerprint);
   }
 
-  boolean containsFingerprint(String fingerprint) {
-    return fingerprints.contains(fingerprint);
-  }
-
   public void addSnapshot(Snapshot snapshot) {
     Throwable throwable = snapshot.getCaptures().getReturn().getCapturedThrowable().getThrowable();
     throwable = ExceptionHelper.getInnerMostThrowable(throwable);

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/ExceptionHelper.java

@@ -73,7 +73,6 @@ public static Throwable getInnerMostThrowable(Throwable t) {
 
   public static StackTraceElement[] flattenStackTrace(Throwable t) {
     List<StackTraceElement> result = new ArrayList<>();
-    result.add(null); // add a stack frame representing the exception message
     result.addAll(Arrays.asList(t.getStackTrace()));
     if (t.getCause() != null) {
       internalFlattenStackTrace(t.getCause(), t.getStackTrace(), result);
@@ -90,13 +89,9 @@ private static void internalFlattenStackTrace(
       m--;
       n--;
     }
-    elements.add(null); // add a stack frame representing the exception message
     for (int i = 0; i <= m; i++) {
       elements.add(trace[i]);
     }
-    if (trace.length - 1 - m > 0) {
-      elements.add(null); // add a stack frame representing number of common frames ("... n more")
-    }
     if (t.getCause() != null) {
       internalFlattenStackTrace(t.getCause(), trace, elements);
     }

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/DefaultExceptionDebuggerTest.java

@@ -1,6 +1,7 @@
 package com.datadog.debugger.exception;
 
 import static com.datadog.debugger.exception.DefaultExceptionDebugger.SNAPSHOT_ID_TAG_FMT;
+import static com.datadog.debugger.util.TestHelper.assertWithTimeout;
 import static java.util.Collections.emptyList;
 import static java.util.stream.Collectors.toMap;
 import static org.junit.jupiter.api.Assertions.*;
@@ -30,6 +31,7 @@
 import java.io.PrintWriter;
 import java.io.StringReader;
 import java.io.StringWriter;
+import java.time.Duration;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -61,8 +63,10 @@ public void simpleException() {
     String fingerprint = Fingerprinter.fingerprint(exception, classNameFiltering);
     AgentSpan span = mock(AgentSpan.class);
     exceptionDebugger.handleException(exception, span);
+    assertWithTimeout(
+        () -> exceptionDebugger.getExceptionProbeManager().isAlreadyInstrumented(fingerprint),
+        Duration.ofSeconds(30));
     exceptionDebugger.handleException(exception, span);
-    assertTrue(exceptionDebugger.getExceptionProbeManager().isAlreadyInstrumented(fingerprint));
     verify(configurationUpdater).accept(eq(ConfigurationAcceptor.Source.EXCEPTION), any());
   }
 
@@ -95,7 +99,7 @@ public void nestedException() {
         listener.snapshots.stream().collect(toMap(Snapshot::getId, Function.identity()));
     List<String> lines = parseStackTrace(exception);
     int expectedFrameIndex =
-        findLine(
+        findFrameIndex(
             lines,
             "com.datadog.debugger.exception.DefaultExceptionDebuggerTest.createNestException");
     assertSnapshot(
@@ -105,7 +109,7 @@ public void nestedException() {
         "com.datadog.debugger.exception.DefaultExceptionDebuggerTest",
         "createNestException");
     expectedFrameIndex =
-        findLine(
+        findFrameIndex(
             lines, "com.datadog.debugger.exception.DefaultExceptionDebuggerTest.nestedException");
     assertSnapshot(
         tags,
@@ -114,7 +118,7 @@ public void nestedException() {
         "com.datadog.debugger.exception.DefaultExceptionDebuggerTest",
         "nestedException");
     expectedFrameIndex =
-        findLine(
+        findFrameIndex(
             lines,
             "com.datadog.debugger.exception.DefaultExceptionDebuggerTest.createTest1Exception");
     assertSnapshot(
@@ -125,7 +129,7 @@ public void nestedException() {
         "createTest1Exception");
   }
 
-  private static int findLine(List<String> lines, String str) {
+  private static int findFrameIndex(List<String> lines, String str) {
     for (int i = 0; i < lines.size(); i++) {
       if (lines.get(i).contains(str)) {
         return i;
@@ -143,7 +147,9 @@ private static List<String> parseStackTrace(RuntimeException exception) {
     try {
       String line;
       while ((line = reader.readLine()) != null) {
-        results.add(line);
+        if (line.startsWith("\tat ")) {
+          results.add(line);
+        }
       }
     } catch (IOException e) {
       e.printStackTrace();

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeInstrumentationTest.java

@@ -4,6 +4,7 @@
 import static com.datadog.debugger.exception.DefaultExceptionDebugger.DD_DEBUG_ERROR_EXCEPTION_ID;
 import static com.datadog.debugger.exception.DefaultExceptionDebugger.ERROR_DEBUG_INFO_CAPTURED;
 import static com.datadog.debugger.exception.DefaultExceptionDebugger.SNAPSHOT_ID_TAG_FMT;
+import static com.datadog.debugger.util.TestHelper.assertWithTimeout;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.mockito.Mockito.mock;
@@ -31,6 +32,7 @@
 import datadog.trace.core.CoreTracer;
 import java.lang.instrument.ClassFileTransformer;
 import java.lang.instrument.Instrumentation;
+import java.time.Duration;
 import java.util.Arrays;
 import java.util.Map;
 import java.util.Set;
@@ -77,7 +79,10 @@ public void onlyInstrument() throws Exception {
         setupExceptionDebugging(config, exceptionProbeManager, classNameFiltering);
     final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20";
     Class<?> testClass = compileAndLoadClass(CLASS_NAME);
-    callMethodThrowingRuntimeException(testClass); // instrument exception stacktrace
+    String fingerprint =
+        callMethodThrowingRuntimeException(testClass); // instrument exception stacktrace
+    assertWithTimeout(
+        () -> exceptionProbeManager.isAlreadyInstrumented(fingerprint), Duration.ofSeconds(30));
     assertEquals(2, exceptionProbeManager.getProbes().size());
     callMethodNoException(testClass);
     assertEquals(0, listener.snapshots.size());
@@ -91,7 +96,10 @@ public void instrumentAndCaptureSnapshots() throws Exception {
         setupExceptionDebugging(config, exceptionProbeManager, classNameFiltering);
     final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20";
     Class<?> testClass = compileAndLoadClass(CLASS_NAME);
-    callMethodThrowingRuntimeException(testClass); // instrument exception stacktrace
+    String fingerprint =
+        callMethodThrowingRuntimeException(testClass); // instrument exception stacktrace
+    assertWithTimeout(
+        () -> exceptionProbeManager.isAlreadyInstrumented(fingerprint), Duration.ofSeconds(30));
     assertEquals(2, exceptionProbeManager.getProbes().size());
     callMethodThrowingRuntimeException(testClass); // generate snapshots
     Map<String, Set<String>> probeIdsByMethodName =
@@ -107,7 +115,7 @@ public void instrumentAndCaptureSnapshots() throws Exception {
     MutableSpan span = traceInterceptor.getFirstSpan();
     assertEquals(snapshot0.getExceptionId(), span.getTags().get(DD_DEBUG_ERROR_EXCEPTION_ID));
     assertEquals(Boolean.TRUE, span.getTags().get(ERROR_DEBUG_INFO_CAPTURED));
-    assertEquals(snapshot0.getId(), span.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 1)));
+    assertEquals(snapshot0.getId(), span.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 0)));
   }
 
   @Test
@@ -119,10 +127,14 @@ public void differentExceptionsSameStack() throws Exception {
     final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20";
     Class<?> testClass = compileAndLoadClass(CLASS_NAME);
     // instrument RuntimeException stacktrace
-    callMethodThrowingRuntimeException(testClass);
+    String fingerprint0 = callMethodThrowingRuntimeException(testClass);
+    assertWithTimeout(
+        () -> exceptionProbeManager.isAlreadyInstrumented(fingerprint0), Duration.ofSeconds(30));
     assertEquals(2, exceptionProbeManager.getProbes().size());
     // instrument IllegalArgumentException stacktrace
-    callMethodThrowingIllegalArgException(testClass);
+    String fingerprint1 = callMethodThrowingIllegalArgException(testClass);
+    assertWithTimeout(
+        () -> exceptionProbeManager.isAlreadyInstrumented(fingerprint1), Duration.ofSeconds(30));
     assertEquals(4, exceptionProbeManager.getProbes().size());
     Map<String, Set<String>> probeIdsByMethodName =
         extractProbeIdsByMethodName(exceptionProbeManager);
@@ -142,11 +154,11 @@ public void differentExceptionsSameStack() throws Exception {
     MutableSpan span0 = traceInterceptor.getAllTraces().get(0).get(0);
     assertEquals(snapshot0.getExceptionId(), span0.getTags().get(DD_DEBUG_ERROR_EXCEPTION_ID));
     assertEquals(Boolean.TRUE, span0.getTags().get(ERROR_DEBUG_INFO_CAPTURED));
-    assertEquals(snapshot0.getId(), span0.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 1)));
+    assertEquals(snapshot0.getId(), span0.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 0)));
     MutableSpan span1 = traceInterceptor.getAllTraces().get(1).get(0);
     assertEquals(snapshot1.getExceptionId(), span1.getTags().get(DD_DEBUG_ERROR_EXCEPTION_ID));
     assertEquals(Boolean.TRUE, span1.getTags().get(ERROR_DEBUG_INFO_CAPTURED));
-    assertEquals(snapshot1.getId(), span1.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 1)));
+    assertEquals(snapshot1.getId(), span1.getTags().get(String.format(SNAPSHOT_ID_TAG_FMT, 0)));
   }
 
   private static void assertExceptionMsg(String expectedMsg, Snapshot snapshot) {
@@ -160,22 +172,26 @@ private static void assertProbeId(
     assertTrue(probeIdsByMethodName.get(methodName).contains(id));
   }
 
-  private static void callMethodThrowingRuntimeException(Class<?> testClass) {
+  private String callMethodThrowingRuntimeException(Class<?> testClass) {
     try {
       Reflect.on(testClass).call("main", "exception").get();
       Assertions.fail("should not reach this code");
     } catch (RuntimeException ex) {
       assertEquals("oops", ex.getCause().getCause().getMessage());
+      return Fingerprinter.fingerprint(ex, classNameFiltering);
     }
+    return null;
   }
 
-  private static void callMethodThrowingIllegalArgException(Class<?> testClass) {
+  private String callMethodThrowingIllegalArgException(Class<?> testClass) {
     try {
       Reflect.on(testClass).call("main", "illegal").get();
       Assertions.fail("should not reach this code");
     } catch (RuntimeException ex) {
       assertEquals("illegal argument", ex.getCause().getCause().getMessage());
+      return Fingerprinter.fingerprint(ex, classNameFiltering);
     }
+    return null;
   }
 
   private static void callMethodNoException(Class<?> testClass) {

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeManagerTest.java

@@ -18,7 +18,7 @@ public void instrumentStackTrace() {
     RuntimeException exception = new RuntimeException("test");
     String fingerprint = Fingerprinter.fingerprint(exception, classNameFiltering);
     exceptionProbeManager.createProbesForException(fingerprint, exception.getStackTrace());
-    assertTrue(exceptionProbeManager.isAlreadyInstrumented(fingerprint));
+    assertFalse(exceptionProbeManager.getProbes().isEmpty());
   }
 
   @Test

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/util/ExceptionHelperTest.java

@@ -104,9 +104,9 @@ public void flattenStackTrace() {
             },
             simpleException);
     StackTraceElement[] stack = ExceptionHelper.flattenStackTrace(simpleException);
-    assertEquals(2, stack.length); // message + frame
+    assertEquals(1, stack.length);
     stack = ExceptionHelper.flattenStackTrace(nestedException);
-    assertEquals(4, stack.length); // message + frame + message + frame
+    assertEquals(2, stack.length);
   }
 
   @Test

dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/util/TestHelper.java

@@ -1,7 +1,11 @@
 package com.datadog.debugger.util;
 
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
 import datadog.trace.api.Config;
 import java.lang.reflect.Field;
+import java.time.Duration;
+import java.util.function.BooleanSupplier;
 
 public class TestHelper {
 
@@ -14,4 +18,17 @@ public static void setFieldInConfig(Config config, String fieldName, Object valu
       e.printStackTrace();
     }
   }
+
+  public static void assertWithTimeout(BooleanSupplier predicate, Duration timeout) {
+    Duration sleepTime = Duration.ofMillis(10);
+    long count = timeout.toMillis() / sleepTime.toMillis();
+    while (count-- > 0 && !predicate.getAsBoolean()) {
+      try {
+        Thread.sleep(sleepTime.toMillis());
+      } catch (InterruptedException e) {
+        throw new RuntimeException(e);
+      }
+    }
+    assertTrue(predicate.getAsBoolean());
+  }
 }

dd-trace-api/src/main/java/datadog/trace/api/config/DebuggerConfig.java

@@ -29,8 +29,7 @@ public final class DebuggerConfig {
   public static final String DEBUGGER_SYMBOL_FORCE_UPLOAD = "internal.force.symbol.database.upload";
   public static final String DEBUGGER_SYMBOL_INCLUDES = "symbol.database.includes";
   public static final String DEBUGGER_SYMBOL_FLUSH_THRESHOLD = "symbol.database.flush.threshold";
-  public static final String DEBUGGER_EXCEPTION_ENABLED =
-      "dynamic.instrumentation.exception.enabled";
+  public static final String DEBUGGER_EXCEPTION_ENABLED = "exception.debugging.enabled";
 
   private DebuggerConfig() {}
 }

dd-trace-core/src/main/java/datadog/trace/core/StatusLogger.java

@@ -116,6 +116,8 @@ public void toJson(JsonWriter writer, Config config) throws IOException {
     writer.value(config.isRemoteConfigEnabled());
     writer.name("debugger_enabled");
     writer.value(config.isDebuggerEnabled());
+    writer.name("debugger_exception_enabled");
+    writer.value(config.isDebuggerExceptionEnabled());
     writer.name("appsec_enabled");
     writer.value(config.getAppSecActivation().toString());
     writer.name("appsec_rules_file_path");