Fix Max captured frames for Exception Replay (#8856)

For recursive frames we were sending snapshot as long as the frame are
the same. frame capture was relying on the fact they were different.
Now we are limiting the snapshot sent to config parameter

Author: jpbempel

dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java

@@ -212,7 +212,8 @@ public static void startExceptionReplay() {
             configurationUpdater,
             classNameFilter,
             Duration.ofSeconds(config.getDebuggerExceptionCaptureInterval()),
-            config.getDebuggerMaxExceptionPerSecond());
+            config.getDebuggerMaxExceptionPerSecond(),
+            config.getDebuggerExceptionMaxCapturedFrames());
     DebuggerContext.initExceptionDebugger(exceptionDebugger);
     LOGGER.info("Started Exception Replay");
   }

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

@@ -38,28 +38,33 @@ public class DefaultExceptionDebugger implements DebuggerContext.ExceptionDebugg
   private final ConfigurationUpdater configurationUpdater;
   private final ClassNameFilter classNameFiltering;
   private final CircuitBreaker circuitBreaker;
+  private final int maxCapturedFrames;
 
   public DefaultExceptionDebugger(
       ConfigurationUpdater configurationUpdater,
       ClassNameFilter classNameFiltering,
       Duration captureInterval,
-      int maxExceptionPerSecond) {
+      int maxExceptionPerSecond,
+      int maxCapturedFrames) {
     this(
         new ExceptionProbeManager(classNameFiltering, captureInterval),
         configurationUpdater,
         classNameFiltering,
-        maxExceptionPerSecond);
+        maxExceptionPerSecond,
+        maxCapturedFrames);
   }
 
   DefaultExceptionDebugger(
       ExceptionProbeManager exceptionProbeManager,
       ConfigurationUpdater configurationUpdater,
       ClassNameFilter classNameFiltering,
-      int maxExceptionPerSecond) {
+      int maxExceptionPerSecond,
+      int maxCapturedFrames) {
     this.exceptionProbeManager = exceptionProbeManager;
     this.configurationUpdater = configurationUpdater;
     this.classNameFiltering = classNameFiltering;
     this.circuitBreaker = new CircuitBreaker(maxExceptionPerSecond, Duration.ofSeconds(1));
+    this.maxCapturedFrames = maxCapturedFrames;
   }
 
   @Override
@@ -91,7 +96,8 @@ public void handleException(Throwable t, AgentSpan span) {
         LOGGER.debug("Unable to find state for throwable: {}", innerMostException.toString());
         return;
       }
-      processSnapshotsAndSetTags(t, span, state, chainedExceptionsList, fingerprint);
+      processSnapshotsAndSetTags(
+          t, span, state, chainedExceptionsList, fingerprint, maxCapturedFrames);
       exceptionProbeManager.updateLastCapture(fingerprint);
     } else {
       // climb up the exception chain to find the first exception that has instrumented frames
@@ -128,7 +134,8 @@ private static void processSnapshotsAndSetTags(
       AgentSpan span,
       ThrowableState state,
       List<Throwable> chainedExceptions,
-      String fingerprint) {
+      String fingerprint,
+      int maxCapturedFrames) {
     if (span.getTag(DD_DEBUG_ERROR_EXCEPTION_ID) != null) {
       LOGGER.debug("Clear previous frame tags");
       // already set for this span, clear the frame tags
@@ -142,7 +149,8 @@ private static void processSnapshotsAndSetTags(
     }
     boolean snapshotAssigned = false;
     List<Snapshot> snapshots = state.getSnapshots();
-    for (int i = 0; i < snapshots.size(); i++) {
+    int maxSnapshotSize = Math.min(snapshots.size(), maxCapturedFrames);
+    for (int i = 0; i < maxSnapshotSize; i++) {
       Snapshot snapshot = snapshots.get(i);
       Throwable currentEx = chainedExceptions.get(snapshot.getChainedExceptionIdx());
       int[] mapping = createThrowableMapping(currentEx, t);

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

@@ -67,7 +67,7 @@ public void setUp() {
             new HashSet<>(singletonList("com.datadog.debugger.exception.ThirdPartyCode")));
     exceptionDebugger =
         new DefaultExceptionDebugger(
-            configurationUpdater, classNameFiltering, Duration.ofHours(1), 100);
+            configurationUpdater, classNameFiltering, Duration.ofHours(1), 100, 3);
     listener = new TestSnapshotListener(createConfig(), mock(ProbeStatusSink.class));
     DebuggerAgentHelper.injectSink(listener);
   }
@@ -275,7 +275,7 @@ public void nestedExceptionFullThirdParty() {
   public void filteringOutErrors() {
     ExceptionProbeManager manager = mock(ExceptionProbeManager.class);
     exceptionDebugger =
-        new DefaultExceptionDebugger(manager, configurationUpdater, classNameFiltering, 100);
+        new DefaultExceptionDebugger(manager, configurationUpdater, classNameFiltering, 100, 3);
     exceptionDebugger.handleException(new AssertionError("test"), mock(AgentSpan.class));
     verify(manager, times(0)).isAlreadyInstrumented(any());
   }

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

@@ -17,7 +17,6 @@
 import com.datadog.debugger.agent.ClassesToRetransformFinder;
 import com.datadog.debugger.agent.Configuration;
 import com.datadog.debugger.agent.ConfigurationUpdater;
-import com.datadog.debugger.agent.DebuggerAgent;
 import com.datadog.debugger.agent.DebuggerAgentHelper;
 import com.datadog.debugger.agent.DebuggerTransformer;
 import com.datadog.debugger.agent.JsonSnapshotSerializer;
@@ -99,7 +98,10 @@ public void before() {
     ProbeRateLimiter.setSamplerSupplier(rate -> rate < 101 ? probeSampler : globalSampler);
     ProbeRateLimiter.setGlobalSnapshotRate(1000);
     // to activate the call to DebuggerContext.handleException
-    DebuggerAgent.startExceptionReplay();
+    DebuggerContext.ProductConfigUpdater mockProductConfigUpdater =
+        mock(DebuggerContext.ProductConfigUpdater.class);
+    when(mockProductConfigUpdater.isExceptionReplayEnabled()).thenReturn(true);
+    DebuggerContext.initProductConfigUpdater(mockProductConfigUpdater);
     setFieldInConfig(Config.get(), "debuggerExceptionEnabled", true);
     setFieldInConfig(Config.get(), "dynamicInstrumentationClassFileDumpEnabled", true);
   }
@@ -224,7 +226,8 @@ public void recursive() throws Exception {
     callMethodFiboException(testClass); // generate snapshots
     Map<String, Set<String>> probeIdsByMethodName =
         extractProbeIdsByMethodName(exceptionProbeManager);
-    assertEquals(10, listener.snapshots.size());
+    // limited by Config::getDebuggerExceptionMaxCapturedFrames
+    assertEquals(3, listener.snapshots.size());
     Snapshot snapshot0 = listener.snapshots.get(0);
     assertProbeId(probeIdsByMethodName, "fiboException", snapshot0.getProbe().getId());
     assertEquals(
@@ -234,8 +237,6 @@ public void recursive() throws Exception {
     assertEquals("2", getValue(snapshot1.getCaptures().getReturn().getArguments().get("n")));
     Snapshot snapshot2 = listener.snapshots.get(2);
     assertEquals("3", getValue(snapshot2.getCaptures().getReturn().getArguments().get("n")));
-    Snapshot snapshot9 = listener.snapshots.get(9);
-    assertEquals("10", getValue(snapshot9.getCaptures().getReturn().getArguments().get("n")));
     // sampling happens only once ont he first snapshot then forced for coordinated sampling
     assertEquals(1, probeSampler.getCallCount());
     assertEquals(1, globalSampler.getCallCount());
@@ -382,7 +383,7 @@ private TestSnapshotListener setupExceptionDebugging(
     DebuggerContext.initValueSerializer(new JsonSnapshotSerializer());
     DefaultExceptionDebugger exceptionDebugger =
         new DefaultExceptionDebugger(
-            exceptionProbeManager, configurationUpdater, classNameFiltering, 100);
+            exceptionProbeManager, configurationUpdater, classNameFiltering, 100, 3);
     DebuggerContext.initExceptionDebugger(exceptionDebugger);
     configurationUpdater.accept(REMOTE_CONFIG, definitions);
     return listener;

dd-smoke-tests/debugger-integration-tests/src/main/java/datadog/smoketest/debugger/ServerDebuggerTestApplication.java

@@ -153,6 +153,8 @@ private static void runTracedMethod(String arg) {
       tracedMethodWithDeepException1(42, "foobar", 3.42, map, "var1", "var2", "var3");
     } else if ("lambdaOops".equals(arg)) {
       tracedMethodWithLambdaException(42, "foobar", 3.42, map, "var1", "var2", "var3");
+    } else if ("recursiveOops".equals(arg)) {
+      tracedMethodWithRecursiveException(42, "foobar", 3.42, map, "var1", "var2", "var3");
     } else {
       tracedMethod(42, "foobar", 3.42, map, "var1", "var2", "var3");
     }
@@ -239,6 +241,15 @@ private static void tracedMethodWithLambdaException(
     throw toRuntimeException("lambdaOops");
   }
 
+  private static void tracedMethodWithRecursiveException(
+      int argInt, String argStr, double argDouble, Map<String, String> argMap, String... argVar) {
+    if (argInt > 0) {
+      tracedMethodWithRecursiveException(argInt - 8, argStr, argDouble, argMap, argVar);
+    } else {
+      throw new RuntimeException("recursiveOops");
+    }
+  }
+
   private static RuntimeException toRuntimeException(String msg) {
     return toException(RuntimeException::new, msg);
   }

dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/ExceptionDebuggerIntegrationTest.java

@@ -237,6 +237,51 @@ void test5CapturedFrames() throws Exception {
         });
   }
 
+  @Test
+  @DisplayName("test3CapturedRecursiveFrames")
+  @DisabledIf(
+      value = "datadog.trace.api.Platform#isJ9",
+      disabledReason = "we cannot get local variable debug info")
+  void test3CapturedRecursiveFrames() throws Exception {
+    appUrl = startAppAndAndGetUrl();
+    execute(appUrl, TRACED_METHOD_NAME, "recursiveOops"); // instrumenting first exception
+    waitForInstrumentation(appUrl);
+    execute(appUrl, TRACED_METHOD_NAME, "recursiveOops"); // collecting snapshots and sending them
+    registerTraceListener(this::receiveExceptionReplayTrace);
+    registerSnapshotListener(this::receiveSnapshot);
+    processRequests(
+        () -> {
+          if (snapshotIdTags.isEmpty()) {
+            return false;
+          }
+          if (traceReceived
+              && snapshotReceived
+              && snapshots.containsKey(snapshotIdTags.get(0))
+              && snapshots.containsKey(snapshotIdTags.get(1))
+              && snapshots.containsKey(snapshotIdTags.get(2))) {
+            assertEquals(3, snapshotIdTags.size());
+            assertEquals(3, snapshots.size());
+            // snapshot 0
+            assertRecursiveSnapshot(snapshots.get(snapshotIdTags.get(0)));
+            // snapshot 1
+            assertRecursiveSnapshot(snapshots.get(snapshotIdTags.get(1)));
+            // snapshot 2
+            assertRecursiveSnapshot(snapshots.get(snapshotIdTags.get(2)));
+            return true;
+          }
+          return false;
+        });
+  }
+
+  private static void assertRecursiveSnapshot(Snapshot snapshot) {
+    assertNotNull(snapshot);
+    assertEquals(
+        "recursiveOops", snapshot.getCaptures().getReturn().getCapturedThrowable().getMessage());
+    assertEquals(
+        "datadog.smoketest.debugger.ServerDebuggerTestApplication.tracedMethodWithRecursiveException",
+        snapshot.getStack().get(0).getFunction());
+  }
+
   @Test
   @DisplayName("testLambdaHiddenFrames")
   @DisabledIf(value = "datadog.trace.api.Platform#isJ9", disabledReason = "HotSpot specific test")

dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/InProductEnablementIntegrationTest.java

@@ -45,7 +45,7 @@ void testDynamicInstrumentationEnablementWithLineProbe() throws Exception {
     LogProbe probe =
         LogProbe.builder()
             .probeId(LINE_PROBE_ID1)
-            .where("ServerDebuggerTestApplication.java", 301)
+            .where("ServerDebuggerTestApplication.java", 312)
             .build();
     setCurrentConfiguration(createConfig(probe));
     waitForFeatureStarted(appUrl, "Dynamic Instrumentation");