From abc89dcb050275fcc2446533b201421f3da40dc2 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 15 Jan 2025 11:44:32 +0100 Subject: [PATCH] Add Exception probe custom instrumentation When there is only exception probes at a specific ode location we strip down the instrumentation to only capture values inside the catch (when an exception happens). this way, in normal execution no overhead is pay for an exception probe --- .../bootstrap/debugger/CapturedContext.java | 2 +- .../debugger/agent/DebuggerTransformer.java | 7 ++- .../CapturedContextInstrumentor.java | 45 ++++++++++++----- .../ExceptionInstrumentor.java | 37 ++++++++++++++ .../debugger/probe/ExceptionProbe.java | 18 +++++-- .../ExceptionProbeInstrumentationTest.java | 48 ++++++++++++++++++- 6 files changed, 138 insertions(+), 19 deletions(-) create mode 100644 dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/ExceptionInstrumentor.java 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; }