From 9d160dc6661bd28dfe9dbba1ba3fff6253ee9637 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 8 Jan 2025 10:34:12 +0100 Subject: [PATCH 1/3] Capture values at entry for method probe method probe when evaluateAt is Exit and no condition should capture entry values. When there is a condition, entry values are not captured instrumentation check the presence of the condition to generate the entry instrumentation or not. therefore, the dummy definition used when duplicate probes need to merge the probe conditions if at least one probe has one condition. --- .../debugger/agent/DebuggerTransformer.java | 7 ++++ .../CapturedContextInstrumentor.java | 8 +++- .../debugger/agent/CapturedSnapshotTest.java | 37 +++++++++++++++++++ 3 files changed, 50 insertions(+), 2 deletions(-) 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 a2cc8074f1c..f94dd77a47a 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 @@ -3,6 +3,7 @@ import static java.util.Collections.singletonList; import static java.util.stream.Collectors.toList; +import com.datadog.debugger.el.ProbeCondition; import com.datadog.debugger.instrumentation.DiagnosticMessage; import com.datadog.debugger.instrumentation.InstrumentationResult; import com.datadog.debugger.instrumentation.MethodInfo; @@ -693,6 +694,7 @@ private ProbeDefinition selectReferenceDefinition( MethodLocation evaluateAt = MethodLocation.EXIT; LogProbe.Capture capture = null; boolean captureSnapshot = false; + ProbeCondition probeCondition = null; Where where = capturedContextProbes.get(0).getWhere(); ProbeId probeId = capturedContextProbes.get(0).getProbeId(); for (ProbeDefinition definition : capturedContextProbes) { @@ -704,6 +706,10 @@ private ProbeDefinition selectReferenceDefinition( LogProbe logProbe = (LogProbe) definition; captureSnapshot = captureSnapshot | logProbe.isCaptureSnapshot(); capture = mergeCapture(capture, logProbe.getCapture()); + probeCondition = + probeCondition == null && logProbe.getProbeCondition() != null + ? logProbe.getProbeCondition() + : probeCondition; } if (definition.getEvaluateAt() == MethodLocation.ENTRY || definition.getEvaluateAt() == MethodLocation.DEFAULT) { @@ -713,6 +719,7 @@ private ProbeDefinition selectReferenceDefinition( if (hasLogProbe) { return LogProbe.builder() .probeId(probeId) + .when(probeCondition) .where(where) .evaluateAt(evaluateAt) .capture(capture) 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 dff976aa2fd..c1d396df5b8 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 @@ -27,6 +27,7 @@ import static org.objectweb.asm.Type.VOID_TYPE; import static org.objectweb.asm.Type.getType; +import com.datadog.debugger.probe.ExceptionProbe; import com.datadog.debugger.probe.ProbeDefinition; import com.datadog.debugger.probe.SpanDecorationProbe; import com.datadog.debugger.probe.Where; @@ -422,8 +423,11 @@ private void instrumentMethodEnter() { LabelNode targetNode = new LabelNode(); LabelNode gotoNode = new LabelNode(); insnList.add(new JumpInsnNode(Opcodes.IFEQ, targetNode)); - // if evaluation is at exit, skip collecting data at enter - if (definition.getEvaluateAt() != MethodLocation.EXIT) { + // if evaluation is at exit and with condition and not exception probe, skip collecting data at + // enter + if ((definition.getEvaluateAt() != MethodLocation.EXIT + || !definition.hasCondition()) + && !(definition instanceof ExceptionProbe)) { LabelNode inProbeStartLabel = new LabelNode(); insnList.add(inProbeStartLabel); // stack [] diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java index 05e81a0c1a0..ce3ea578b0f 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java @@ -134,6 +134,43 @@ public void methodProbe() throws IOException, URISyntaxException { assertEquals("CapturedSnapshot01.main", snapshot.getStack().get(0).getFunction()); } + @Test + public void methodProbeAtExit() throws IOException, URISyntaxException { + final String CLASS_NAME = "CapturedSnapshot01"; + TestSnapshotListener listener = + installSingleProbeAtExit(CLASS_NAME, "main", "int (java.lang.String)"); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "1").get(); + assertEquals(3, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertCaptureArgs(snapshot.getCaptures().getEntry(), "arg", "java.lang.String", "1"); + assertCaptureArgs(snapshot.getCaptures().getReturn(), "arg", "java.lang.String", "1"); + assertTrue(snapshot.getDuration() > 0); + assertTrue(snapshot.getStack().size() > 0); + assertEquals("CapturedSnapshot01.main", snapshot.getStack().get(0).getFunction()); + } + + @Test + public void methodProbeAtExitWithCondition() throws IOException, URISyntaxException { + final String CLASS_NAME = "CapturedSnapshot01"; + LogProbe probe = + createProbeBuilder(PROBE_ID, CLASS_NAME, "main", "int (java.lang.String)") + .when( + new ProbeCondition(DSL.when(DSL.eq(DSL.ref("arg"), DSL.value("1"))), "arg == '1'")) + .evaluateAt(MethodLocation.EXIT) + .build(); + TestSnapshotListener listener = installProbes(probe); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.onClass(testClass).call("main", "1").get(); + assertEquals(3, result); + Snapshot snapshot = assertOneSnapshot(listener); + assertEquals(CapturedContext.EMPTY_CAPTURING_CONTEXT, snapshot.getCaptures().getEntry()); + assertCaptureArgs(snapshot.getCaptures().getReturn(), "arg", "java.lang.String", "1"); + assertTrue(snapshot.getDuration() > 0); + assertTrue(snapshot.getStack().size() > 0); + assertEquals("CapturedSnapshot01.main", snapshot.getStack().get(0).getFunction()); + } + @Test public void localVarHoistingNoPreviousStore() throws IOException, URISyntaxException { final String CLASS_NAME = "com.fasterxml.jackson.core.json.ByteSourceJsonBootstrapper"; From 06de71b0d10cec877b3c0919531f431ce9d816a3 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Thu, 9 Jan 2025 09:41:21 +0100 Subject: [PATCH 2/3] fix unit tests We are delegating the decision to capture through a new field in CapturedContextInstrumentor --- .../debugger/exception/ExceptionProbeManager.java | 8 +------- .../instrumentation/CapturedContextInstrumentor.java | 10 ++++------ .../com/datadog/debugger/probe/ExceptionProbe.java | 5 +++-- .../main/java/com/datadog/debugger/probe/LogProbe.java | 10 +++++++++- .../datadog/debugger/probe/SpanDecorationProbe.java | 3 ++- .../java/com/datadog/debugger/probe/TriggerProbe.java | 3 ++- 6 files changed, 21 insertions(+), 18 deletions(-) diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java index 2eafd524318..73a5b9737a9 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/exception/ExceptionProbeManager.java @@ -118,13 +118,7 @@ private static ExceptionProbe createMethodProbe( ExceptionProbeManager exceptionProbeManager, Where where, int chainedExceptionIdx) { String probeId = UUID.randomUUID().toString(); return new ExceptionProbe( - new ProbeId(probeId, 0), - where, - null, - null, - null, - exceptionProbeManager, - chainedExceptionIdx); + new ProbeId(probeId, 0), where, null, null, exceptionProbeManager, chainedExceptionIdx); } public boolean isAlreadyInstrumented(String fingerprint) { 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 c1d396df5b8..c63caf3db8d 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 @@ -27,7 +27,6 @@ import static org.objectweb.asm.Type.VOID_TYPE; import static org.objectweb.asm.Type.getType; -import com.datadog.debugger.probe.ExceptionProbe; import com.datadog.debugger.probe.ProbeDefinition; import com.datadog.debugger.probe.SpanDecorationProbe; import com.datadog.debugger.probe.Where; @@ -70,6 +69,7 @@ public class CapturedContextInstrumentor extends Instrumentor { private static final Logger LOGGER = LoggerFactory.getLogger(CapturedContextInstrumentor.class); private final boolean captureSnapshot; + private final boolean captureEntry; private final Limits limits; private final LabelNode contextInitLabel = new LabelNode(); private int entryContextVar = -1; @@ -84,9 +84,11 @@ public CapturedContextInstrumentor( List diagnostics, List probeIds, boolean captureSnapshot, + boolean captureEntry, Limits limits) { super(definition, methodInfo, diagnostics, probeIds); this.captureSnapshot = captureSnapshot; + this.captureEntry = captureEntry; this.limits = limits; } @@ -423,11 +425,7 @@ private void instrumentMethodEnter() { LabelNode targetNode = new LabelNode(); LabelNode gotoNode = new LabelNode(); insnList.add(new JumpInsnNode(Opcodes.IFEQ, targetNode)); - // if evaluation is at exit and with condition and not exception probe, skip collecting data at - // enter - if ((definition.getEvaluateAt() != MethodLocation.EXIT - || !definition.hasCondition()) - && !(definition instanceof ExceptionProbe)) { + if (captureEntry) { LabelNode inProbeStartLabel = new LabelNode(); insnList.add(inProbeStartLabel); // stack [] 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 c727bece3a1..247dd5ad7eb 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 @@ -3,6 +3,7 @@ import static com.datadog.debugger.util.ExceptionHelper.getInnerMostThrowable; import static java.util.Collections.emptyList; +import com.datadog.debugger.el.DSL; import com.datadog.debugger.el.ProbeCondition; import com.datadog.debugger.exception.ExceptionProbeManager; import com.datadog.debugger.exception.Fingerprinter; @@ -26,7 +27,6 @@ public class ExceptionProbe extends LogProbe implements ForceMethodInstrumentati public ExceptionProbe( ProbeId probeId, Where where, - ProbeCondition probeCondition, Capture capture, Sampling sampling, ExceptionProbeManager exceptionProbeManager, @@ -40,7 +40,8 @@ public ExceptionProbe( null, null, true, - probeCondition, + // forcing a useless condition to be instrumented with captureEntry=false + new ProbeCondition(DSL.when(DSL.TRUE), "true"), capture, sampling); this.exceptionProbeManager = exceptionProbeManager; diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/LogProbe.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/LogProbe.java index 81f1145bb54..c9896878b4d 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/LogProbe.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/LogProbe.java @@ -409,8 +409,16 @@ public Sampling getSampling() { @Override public InstrumentationResult.Status instrument( MethodInfo methodInfo, List diagnostics, List probeIds) { + // if evaluation is at exit and with condition, skip collecting data at entry + boolean captureEntry = !(getEvaluateAt() == MethodLocation.EXIT && hasCondition()); return new CapturedContextInstrumentor( - this, methodInfo, diagnostics, probeIds, isCaptureSnapshot(), toLimits(getCapture())) + this, + methodInfo, + diagnostics, + probeIds, + isCaptureSnapshot(), + captureEntry, + toLimits(getCapture())) .instrument(); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/SpanDecorationProbe.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/SpanDecorationProbe.java index 04ea5f7465e..3c8ef044814 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/SpanDecorationProbe.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/SpanDecorationProbe.java @@ -135,8 +135,9 @@ public SpanDecorationProbe( @Override public InstrumentationResult.Status instrument( MethodInfo methodInfo, List diagnostics, List probeIds) { + boolean captureEntry = evaluateAt != MethodLocation.EXIT; return new CapturedContextInstrumentor( - this, methodInfo, diagnostics, probeIds, false, Limits.DEFAULT) + this, methodInfo, diagnostics, probeIds, false, captureEntry, Limits.DEFAULT) .instrument(); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/TriggerProbe.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/TriggerProbe.java index f9dcf2f38b8..9b1d077b993 100644 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/TriggerProbe.java +++ b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/probe/TriggerProbe.java @@ -54,7 +54,8 @@ public TriggerProbe(ProbeId probeId, Where where) { @Override public InstrumentationResult.Status instrument( MethodInfo methodInfo, List diagnostics, List probeIds) { - return new CapturedContextInstrumentor(this, methodInfo, diagnostics, probeIds, false, null) + return new CapturedContextInstrumentor( + this, methodInfo, diagnostics, probeIds, false, false, null) .instrument(); } From 9b1534a6ccba9c10cb532153d2b1db9a6099accf Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Tue, 14 Jan 2025 19:38:33 +0100 Subject: [PATCH 3/3] simplify condition --- .../com/datadog/debugger/agent/DebuggerTransformer.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) 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 f94dd77a47a..74591842a7f 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 @@ -706,10 +706,9 @@ private ProbeDefinition selectReferenceDefinition( LogProbe logProbe = (LogProbe) definition; captureSnapshot = captureSnapshot | logProbe.isCaptureSnapshot(); capture = mergeCapture(capture, logProbe.getCapture()); - probeCondition = - probeCondition == null && logProbe.getProbeCondition() != null - ? logProbe.getProbeCondition() - : probeCondition; + if (probeCondition == null) { + probeCondition = logProbe.getProbeCondition(); + } } if (definition.getEvaluateAt() == MethodLocation.ENTRY || definition.getEvaluateAt() == MethodLocation.DEFAULT) {