From 2ca125edc09dcf300f8065bac469ed02374e1d3f Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 21 May 2025 07:21:50 +0200 Subject: [PATCH] Fix Max captured frames for Exception Replay 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 --- .../datadog/debugger/agent/DebuggerAgent.java | 3 +- .../exception/DefaultExceptionDebugger.java | 20 ++++++--- .../DefaultExceptionDebuggerTest.java | 4 +- .../ExceptionProbeInstrumentationTest.java | 13 +++--- .../ServerDebuggerTestApplication.java | 11 +++++ .../ExceptionDebuggerIntegrationTest.java | 45 +++++++++++++++++++ .../InProductEnablementIntegrationTest.java | 2 +- 7 files changed, 82 insertions(+), 16 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java index bc59cc47438..dbeb9738565 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerAgent.java +++ b/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"); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/DefaultExceptionDebugger.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/DefaultExceptionDebugger.java index 2d4b2dd5fe5..981f52208b8 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/DefaultExceptionDebugger.java +++ b/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 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 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); diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/DefaultExceptionDebuggerTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/DefaultExceptionDebuggerTest.java index aaabb300cc0..a399fee533b 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/DefaultExceptionDebuggerTest.java +++ b/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()); } diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeInstrumentationTest.java index 060b7b370c0..ed7500e0819 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeInstrumentationTest.java +++ b/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> 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; diff --git a/dd-smoke-tests/debugger-integration-tests/src/main/java/datadog/smoketest/debugger/ServerDebuggerTestApplication.java b/dd-smoke-tests/debugger-integration-tests/src/main/java/datadog/smoketest/debugger/ServerDebuggerTestApplication.java index 75ee4ee5ef3..6462849d025 100644 --- a/dd-smoke-tests/debugger-integration-tests/src/main/java/datadog/smoketest/debugger/ServerDebuggerTestApplication.java +++ b/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 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); } diff --git a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/ExceptionDebuggerIntegrationTest.java b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/ExceptionDebuggerIntegrationTest.java index 3bd483c648a..f73ad478fcf 100644 --- a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/ExceptionDebuggerIntegrationTest.java +++ b/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") diff --git a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/InProductEnablementIntegrationTest.java b/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/InProductEnablementIntegrationTest.java index 1964fcaa7ce..d844492ea82 100644 --- a/dd-smoke-tests/debugger-integration-tests/src/test/java/datadog/smoketest/InProductEnablementIntegrationTest.java +++ b/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");