diff --git a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java index 74fd8ea536a..fe7b336f6ff 100644 --- a/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java +++ b/dd-java-agent/agent-debugger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java @@ -299,7 +299,7 @@ public Status evaluate( MethodLocation methodLocation) { Status status = statusByProbeId.computeIfAbsent(encodedProbeId, key -> probeImplementation.createStatus()); - if (methodLocation == MethodLocation.EXIT) { + if (methodLocation == MethodLocation.EXIT && startTimestamp > 0) { duration = System.nanoTime() - startTimestamp; addExtension( ValueReferences.DURATION_EXTENSION_NAME, duration / 1_000_000.0); // convert to ms diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java index 7295bfd4efb..785a17c5a67 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/agent/DebuggerTransformer.java @@ -430,7 +430,6 @@ private byte[] writeClassFile( classNode.version = Opcodes.V1_8; } ClassWriter writer = new SafeClassWriter(loader); - log.debug("Generating bytecode for class: {}", Strings.getClassName(classFilePath)); try { classNode.accept(writer); @@ -695,6 +694,8 @@ private static void processCapturedContextLineProbes( private ProbeDefinition selectReferenceDefinition( List capturedContextProbes, ClassFileLines classFileLines) { boolean hasLogProbe = false; + boolean hasOnlyExceptionProbe = + capturedContextProbes.stream().allMatch(def -> def instanceof ExceptionProbe); MethodLocation evaluateAt = MethodLocation.EXIT; LogProbe.Capture capture = null; boolean captureSnapshot = false; @@ -719,6 +720,10 @@ private ProbeDefinition selectReferenceDefinition( evaluateAt = definition.getEvaluateAt(); } } + if (hasOnlyExceptionProbe) { + // only exception probes return the first one + return capturedContextProbes.get(0); + } if (hasLogProbe) { return LogProbe.builder() .probeId(probeId) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java index c63caf3db8d..ea904a3f43e 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/CapturedContextInstrumentor.java @@ -104,7 +104,7 @@ public InstrumentationResult.Status instrument() { instrumentMethodEnter(); instrumentTryCatchHandlers(); processInstructions(); - addFinallyHandler(returnHandlerLabel); + addFinallyHandler(contextInitLabel, returnHandlerLabel); installFinallyBlocks(); return InstrumentationResult.Status.INSTALLED; } @@ -317,7 +317,11 @@ protected InsnList getReturnHandlerInsnList() { private InsnList commit() { InsnList insnList = new InsnList(); // stack [] - getContext(insnList, entryContextVar); + if (entryContextVar != -1) { + getContext(insnList, entryContextVar); + } else { + getStatic(insnList, CAPTURED_CONTEXT_TYPE, "EMPTY_CAPTURING_CONTEXT"); + } // stack [capturedcontext] getContext(insnList, exitContextVar); // stack [capturedcontext, capturedcontext] @@ -342,7 +346,7 @@ private InsnList commit() { return insnList; } - private void addFinallyHandler(LabelNode endLabel) { + protected void addFinallyHandler(LabelNode startLabel, LabelNode endLabel) { // stack: [exception] if (methodNode.tryCatchBlocks == null) { methodNode.tryCatchBlocks = new ArrayList<>(); @@ -351,18 +355,28 @@ private void addFinallyHandler(LabelNode endLabel) { InsnList handler = new InsnList(); handler.add(handlerLabel); // stack [exception] - handler.add(new VarInsnNode(Opcodes.ALOAD, entryContextVar)); - // stack [exception, capturedcontext] - LabelNode targetNode = new LabelNode(); - invokeVirtual(handler, CAPTURED_CONTEXT_TYPE, "isCapturing", BOOLEAN_TYPE); - // stack [exception, boolean] - handler.add(new JumpInsnNode(Opcodes.IFEQ, targetNode)); + LabelNode targetNode = null; + if (entryContextVar != -1) { + handler.add(new VarInsnNode(Opcodes.ALOAD, entryContextVar)); + // stack [exception, capturedcontext] + targetNode = new LabelNode(); + invokeVirtual(handler, CAPTURED_CONTEXT_TYPE, "isCapturing", BOOLEAN_TYPE); + // stack [exception, boolean] + handler.add(new JumpInsnNode(Opcodes.IFEQ, targetNode)); + } + if (exitContextVar == -1) { + exitContextVar = newVar(CAPTURED_CONTEXT_TYPE); + } // stack [exception] handler.add(collectCapturedContext(Snapshot.Kind.UNHANDLED_EXCEPTION, endLabel)); // stack: [exception, capturedcontext] ldc(handler, Type.getObjectType(classNode.name)); // stack [exception, capturedcontext, class] - handler.add(new VarInsnNode(Opcodes.LLOAD, timestampStartVar)); + if (timestampStartVar != -1) { + handler.add(new VarInsnNode(Opcodes.LLOAD, timestampStartVar)); + } else { + ldc(handler, -1L); + } // stack [exception, capturedcontext, class, long] getStatic(handler, METHOD_LOCATION_TYPE, "EXIT"); // stack [exception, capturedcontext, class, long, methodlocation] @@ -382,12 +396,14 @@ private void addFinallyHandler(LabelNode endLabel) { invokeStatic(handler, DEBUGGER_CONTEXT_TYPE, "disableInProbe", VOID_TYPE); // stack [exception] handler.add(commit()); - handler.add(targetNode); + if (targetNode != null) { + handler.add(targetNode); + } // stack [exception] handler.add(new InsnNode(Opcodes.ATHROW)); // stack: [] methodNode.instructions.add(handler); - finallyBlocks.add(new FinallyBlock(contextInitLabel, endLabel, handlerLabel)); + finallyBlocks.add(new FinallyBlock(startLabel, endLabel, handlerLabel)); } private void instrumentMethodEnter() { @@ -921,7 +937,10 @@ private void collectLocalVariables(AbstractInsnNode location, InsnList insnList) } } } - + if (applicableVars.isEmpty()) { + // no applicable local variables - bail out + return; + } insnList.add(new InsnNode(Opcodes.DUP)); // stack: [capturedcontext, capturedcontext] ldc(insnList, applicableVars.size()); diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ExceptionInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ExceptionInstrumentor.java new file mode 100644 index 00000000000..74f9963943c --- /dev/null +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ExceptionInstrumentor.java @@ -0,0 +1,37 @@ +package com.datadog.debugger.instrumentation; + +import com.datadog.debugger.probe.ProbeDefinition; +import datadog.trace.bootstrap.debugger.Limits; +import datadog.trace.bootstrap.debugger.ProbeId; +import java.util.List; +import org.objectweb.asm.tree.AbstractInsnNode; +import org.objectweb.asm.tree.InsnList; + +public class ExceptionInstrumentor extends CapturedContextInstrumentor { + + public ExceptionInstrumentor( + ProbeDefinition definition, + MethodInfo methodInfo, + List diagnostics, + List probeIds) { + super(definition, methodInfo, diagnostics, probeIds, true, false, Limits.DEFAULT); + } + + @Override + public InstrumentationResult.Status instrument() { + processInstructions(); // fill returnHandlerLabel + addFinallyHandler(methodEnterLabel, returnHandlerLabel); + installFinallyBlocks(); + return InstrumentationResult.Status.INSTALLED; + } + + @Override + protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) { + return null; + } + + @Override + protected InsnList getReturnHandlerInsnList() { + return new InsnList(); + } +} diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java index 61307e5059b..59b8e9eb41c 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/ExceptionProbe.java @@ -7,6 +7,9 @@ import com.datadog.debugger.el.ProbeCondition; import com.datadog.debugger.exception.ExceptionProbeManager; import com.datadog.debugger.exception.Fingerprinter; +import com.datadog.debugger.instrumentation.DiagnosticMessage; +import com.datadog.debugger.instrumentation.ExceptionInstrumentor; +import com.datadog.debugger.instrumentation.InstrumentationResult; import com.datadog.debugger.instrumentation.MethodInfo; import com.datadog.debugger.sink.Snapshot; import datadog.trace.bootstrap.debugger.CapturedContext; @@ -48,6 +51,12 @@ public ExceptionProbe( this.chainedExceptionIdx = chainedExceptionIdx; } + @Override + public InstrumentationResult.Status instrument( + MethodInfo methodInfo, List diagnostics, List probeIds) { + return new ExceptionInstrumentor(this, methodInfo, diagnostics, probeIds).instrument(); + } + @Override public boolean isLineProbe() { // Exception probe are always method probe even if there is a line number @@ -62,7 +71,11 @@ public CapturedContext.Status createStatus() { @Override public void evaluate( CapturedContext context, CapturedContext.Status status, MethodLocation methodLocation) { - if (!(status instanceof ExceptionProbeStatus)) { + ExceptionProbeStatus exceptionStatus; + if (status instanceof ExceptionProbeStatus) { + exceptionStatus = (ExceptionProbeStatus) status; + exceptionStatus.setCapture(false); + } else { throw new IllegalStateException("Invalid status: " + status.getClass()); } if (methodLocation != MethodLocation.EXIT) { @@ -82,7 +95,6 @@ public void evaluate( if (exceptionProbeManager.shouldCaptureException(fingerprint)) { LOGGER.debug("Capturing exception matching fingerprint: {}", fingerprint); // capture only on uncaught exception matching the fingerprint - ExceptionProbeStatus exceptionStatus = (ExceptionProbeStatus) status; ExceptionProbeManager.ThrowableState state = exceptionProbeManager.getStateByThrowable(innerMostThrowable); if (state != null) { @@ -148,7 +160,7 @@ public void buildLocation(MethodInfo methodInfo) { } public static class ExceptionProbeStatus extends LogStatus { - private boolean capture; + private boolean capture = true; // default to true for status entry when mixed with log probe public ExceptionProbeStatus(ProbeImplementation probeImplementation) { super(probeImplementation); 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 5342486296d..30e02b5fd52 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 @@ -22,6 +22,8 @@ import com.datadog.debugger.agent.JsonSnapshotSerializer; import com.datadog.debugger.agent.MockSampler; import com.datadog.debugger.probe.ExceptionProbe; +import com.datadog.debugger.probe.LogProbe; +import com.datadog.debugger.probe.ProbeDefinition; import com.datadog.debugger.sink.DebuggerSink; import com.datadog.debugger.sink.ProbeStatusSink; import com.datadog.debugger.sink.Snapshot; @@ -33,14 +35,19 @@ import datadog.trace.api.interceptor.MutableSpan; import datadog.trace.bootstrap.debugger.DebuggerContext; import datadog.trace.bootstrap.debugger.DebuggerContext.ClassNameFilter; +import datadog.trace.bootstrap.debugger.ProbeId; import datadog.trace.bootstrap.debugger.ProbeLocation; import datadog.trace.bootstrap.debugger.ProbeRateLimiter; import datadog.trace.core.CoreTracer; +import java.io.IOException; import java.lang.instrument.ClassFileTransformer; import java.lang.instrument.Instrumentation; +import java.net.URISyntaxException; import java.time.Clock; import java.time.Duration; import java.time.Instant; +import java.util.Arrays; +import java.util.Collection; import java.util.Map; import java.util.Set; import java.util.stream.Collectors; @@ -54,6 +61,8 @@ import org.junit.jupiter.api.condition.DisabledIf; public class ExceptionProbeInstrumentationTest { + protected static final ProbeId PROBE_ID = new ProbeId("beae1807-f3b0-4ea8-a74f-826790c5e6f5", 0); + private final Instrumentation instr = ByteBuddyAgent.install(); private final TestTraceInterceptor traceInterceptor = new TestTraceInterceptor(); private ClassFileTransformer currentTransformer; @@ -84,6 +93,7 @@ public void before() { ProbeRateLimiter.setGlobalSnapshotRate(1000); // to activate the call to DebuggerContext.handleException setFieldInConfig(Config.get(), "debuggerExceptionEnabled", true); + setFieldInConfig(Config.get(), "debuggerClassFileDumpEnabled", true); } @AfterEach @@ -252,6 +262,34 @@ public void captureOncePerHour() throws Exception { assertEquals(1, listener.snapshots.size()); } + @Test + public void mixedWithLogProbes() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20"; + Config config = createConfig(); + ExceptionProbeManager exceptionProbeManager = new ExceptionProbeManager(classNameFiltering); + LogProbe logProbe = + LogProbe.builder().probeId(PROBE_ID).where(CLASS_NAME, "processWithException").build(); + Collection definitions = Arrays.asList(logProbe); + TestSnapshotListener listener = + setupExceptionDebugging(config, exceptionProbeManager, classNameFiltering, definitions); + Class testClass = compileAndLoadClass(CLASS_NAME); + String fingerprint = + callMethodThrowingRuntimeException(testClass); // instrument exception stacktrace + assertWithTimeout( + () -> exceptionProbeManager.isAlreadyInstrumented(fingerprint), Duration.ofSeconds(30)); + assertEquals(2, exceptionProbeManager.getProbes().size()); + callMethodThrowingRuntimeException(testClass); // generate snapshots + Map> probeIdsByMethodName = + extractProbeIdsByMethodName(exceptionProbeManager); + assertEquals(3, listener.snapshots.size()); // 2 log snapshots + 1 exception snapshot + Snapshot snapshot0 = listener.snapshots.get(0); + assertEquals(PROBE_ID.getId(), snapshot0.getProbe().getId()); + Snapshot snapshot1 = listener.snapshots.get(1); + assertEquals(PROBE_ID.getId(), snapshot1.getProbe().getId()); + Snapshot snapshot2 = listener.snapshots.get(2); + assertProbeId(probeIdsByMethodName, "processWithException", snapshot2.getProbe().getId()); + } + private static void assertExceptionMsg(String expectedMsg, Snapshot snapshot) { assertEquals( expectedMsg, snapshot.getCaptures().getReturn().getCapturedThrowable().getMessage()); @@ -314,6 +352,14 @@ private TestSnapshotListener setupExceptionDebugging( Config config, ExceptionProbeManager exceptionProbeManager, ClassNameFilter classNameFiltering) { + return setupExceptionDebugging(config, exceptionProbeManager, classNameFiltering, null); + } + + private TestSnapshotListener setupExceptionDebugging( + Config config, + ExceptionProbeManager exceptionProbeManager, + ClassNameFilter classNameFiltering, + Collection definitions) { ProbeStatusSink probeStatusSink = mock(ProbeStatusSink.class); ConfigurationUpdater configurationUpdater = new ConfigurationUpdater( @@ -330,7 +376,7 @@ private TestSnapshotListener setupExceptionDebugging( new DefaultExceptionDebugger( exceptionProbeManager, configurationUpdater, classNameFiltering, 100); DebuggerContext.initExceptionDebugger(exceptionDebugger); - configurationUpdater.accept(REMOTE_CONFIG, null); + configurationUpdater.accept(REMOTE_CONFIG, definitions); return listener; }