Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make Exception debugging instrumentation async #6829

Merged
merged 2 commits into from
Mar 22, 2024
Merged

Make Exception debugging instrumentation async #6829

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1c59b38
Make Exception debugging instrumentation async
jpbempel Mar 21, 2024
e53c4bc
Refactor waitForInstrumentation
jpbempel Mar 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions ...agent-debugger/src/main/java/com/datadog/debugger/exception/DefaultExceptionDebugger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe add check: if (exceptionProbeManager.isAlreadyInstrumented(fingerprint)) { to avoid race condition?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm no because IsAlreadyInstrumented rely on the fact we add the fingerprint which is now done after applying the config...

exceptionProbeManager.addFingerprint(fingerprint);
}

private static void processSnapshotsAndSetTags(
Throwable t, AgentSpan span, ThrowableState state, Throwable innerMostException) {
if (span.getTag(DD_DEBUG_ERROR_EXCEPTION_ID) != null) {
Expand Down
7 changes: 3 additions & 4 deletions ...nt/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}

Expand All @@ -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);
Expand Down
6 changes: 1 addition & 5 deletions dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/util/ExceptionHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ 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.add(null); // add a stack frame representing the exception message
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right!

result.addAll(Arrays.asList(t.getStackTrace()));
if (t.getCause() != null) {
internalFlattenStackTrace(t.getCause(), t.getStackTrace(), result);
Expand All @@ -90,13 +90,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);
}
Expand Down
15 changes: 9 additions & 6 deletions ...t-debugger/src/test/java/com/datadog/debugger/exception/DefaultExceptionDebuggerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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.exception.ExceptionProbeManagerTest.waitForInstrumentation;
import static java.util.Collections.emptyList;
import static java.util.stream.Collectors.toMap;
import static org.junit.jupiter.api.Assertions.*;
Expand Down Expand Up @@ -61,8 +62,8 @@ public void simpleException() {
String fingerprint = Fingerprinter.fingerprint(exception, classNameFiltering);
AgentSpan span = mock(AgentSpan.class);
exceptionDebugger.handleException(exception, span);
waitForInstrumentation(exceptionDebugger.getExceptionProbeManager(), fingerprint);
exceptionDebugger.handleException(exception, span);
assertTrue(exceptionDebugger.getExceptionProbeManager().isAlreadyInstrumented(fingerprint));
verify(configurationUpdater).accept(eq(ConfigurationAcceptor.Source.EXCEPTION), any());
}

Expand Down Expand Up @@ -95,7 +96,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(
Expand All @@ -105,7 +106,7 @@ public void nestedException() {
"com.datadog.debugger.exception.DefaultExceptionDebuggerTest",
"createNestException");
expectedFrameIndex =
findLine(
findFrameIndex(
lines, "com.datadog.debugger.exception.DefaultExceptionDebuggerTest.nestedException");
assertSnapshot(
tags,
Expand All @@ -114,7 +115,7 @@ public void nestedException() {
"com.datadog.debugger.exception.DefaultExceptionDebuggerTest",
"nestedException");
expectedFrameIndex =
findLine(
findFrameIndex(
lines,
"com.datadog.debugger.exception.DefaultExceptionDebuggerTest.createTest1Exception");
assertSnapshot(
Expand All @@ -125,7 +126,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;
Expand All @@ -143,7 +144,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();
Expand Down
29 changes: 20 additions & 9 deletions ...ugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeInstrumentationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.exception.ExceptionProbeManagerTest.waitForInstrumentation;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertTrue;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -77,7 +78,9 @@ 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
waitForInstrumentation(exceptionProbeManager, fingerprint);
assertEquals(2, exceptionProbeManager.getProbes().size());
callMethodNoException(testClass);
assertEquals(0, listener.snapshots.size());
Expand All @@ -91,7 +94,9 @@ 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
waitForInstrumentation(exceptionProbeManager, fingerprint);
assertEquals(2, exceptionProbeManager.getProbes().size());
callMethodThrowingRuntimeException(testClass); // generate snapshots
Map<String, Set<String>> probeIdsByMethodName =
Expand All @@ -107,7 +112,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
Expand All @@ -119,10 +124,12 @@ 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);
waitForInstrumentation(exceptionProbeManager, fingerprint0);
assertEquals(2, exceptionProbeManager.getProbes().size());
// instrument IllegalArgumentException stacktrace
callMethodThrowingIllegalArgException(testClass);
String fingerprint1 = callMethodThrowingIllegalArgException(testClass);
waitForInstrumentation(exceptionProbeManager, fingerprint1);
assertEquals(4, exceptionProbeManager.getProbes().size());
Map<String, Set<String>> probeIdsByMethodName =
extractProbeIdsByMethodName(exceptionProbeManager);
Expand All @@ -142,11 +149,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) {
Expand All @@ -160,22 +167,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) {
Expand Down
20 changes: 18 additions & 2 deletions ...gent-debugger/src/test/java/com/datadog/debugger/exception/ExceptionProbeManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import com.datadog.debugger.probe.ExceptionProbe;
import com.datadog.debugger.util.ClassNameFiltering;
import java.time.Duration;
import java.util.Arrays;
import java.util.Collections;
import org.junit.jupiter.api.Test;
Expand All @@ -18,7 +19,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
Expand All @@ -28,7 +29,7 @@ void instrumentSingleFrame() {
ExceptionProbeManager exceptionProbeManager = new ExceptionProbeManager(classNameFiltering);

String fingerprint = Fingerprinter.fingerprint(exception, classNameFiltering);
assertEquals("aa4a4dd768f6ef0fcc2b39a3bdedcbe44baff2e9dd0a779228db7bd8bf58", fingerprint);
assertEquals("ca4d9f3a1033d7262a89855f4b5cbdc225ed63c592c6cdf83fc5a88589e5fb", fingerprint);
exceptionProbeManager.createProbesForException(fingerprint, exception.getStackTrace());
assertEquals(1, exceptionProbeManager.getProbes().size());
ExceptionProbe exceptionProbe = exceptionProbeManager.getProbes().iterator().next();
Expand All @@ -52,4 +53,19 @@ void filterAllFrames() {
exceptionProbeManager.createProbesForException("", exception.getStackTrace());
assertEquals(0, exceptionProbeManager.getProbes().size());
}

static void waitForInstrumentation(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like a great utility function to have

assertWithTimeout(()->exceptionProbeManager.isAlreadyInstrumented(fingerprint), Duration.OfSeconds(30))

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

ExceptionProbeManager exceptionProbeManager, String fingerprint) {
Duration timeout = Duration.ofSeconds(30);
Duration sleepTime = Duration.ofMillis(10);
long count = timeout.toMillis() / sleepTime.toMillis();
while (count-- > 0 && !exceptionProbeManager.isAlreadyInstrumented(fingerprint)) {
try {
Thread.sleep(sleepTime.toMillis());
} catch (InterruptedException e) {
throw new RuntimeException(e);
}
}
assertTrue(exceptionProbeManager.isAlreadyInstrumented(fingerprint));
}
}
4 changes: 2 additions & 2 deletions ...ava-agent/agent-debugger/src/test/java/com/datadog/debugger/util/ExceptionHelperTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions dd-trace-api/src/main/java/datadog/trace/api/config/DebuggerConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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() {}
}
2 changes: 2 additions & 0 deletions dd-trace-core/src/main/java/datadog/trace/core/StatusLogger.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down
Loading