From d06271af3b8f6454e8a40ab5e8d16256c832abec Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 30 Aug 2023 09:06:45 +0200 Subject: [PATCH 1/2] Merge span decoration and log instrumentation Span decoration probes and log probes share the same instrumentor so we need to generate the same instrumentation for both otherwise we fall into the same issues than duplicated probes, and local var access issue with scoping. CapturedContextInstrumentor is therefore the only instrumentor for both span decoration and log probes --- .../debugger/agent/DebuggerTransformer.java | 19 ++-- .../instrumentation/LogInstrumentor.java | 37 -------- .../SpanDecorationInstrumentor.java | 24 ----- .../com/datadog/debugger/probe/LogProbe.java | 14 ++- .../debugger/probe/SpanDecorationProbe.java | 6 +- ...panDecorationProbeInstrumentationTest.java | 87 +++++++++++++++---- .../datadog/debugger/CapturedSnapshot20.java | 3 +- 7 files changed, 98 insertions(+), 92 deletions(-) delete mode 100644 dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LogInstrumentor.java delete mode 100644 dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/SpanDecorationInstrumentor.java 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 af79ea47f23..d587537192a 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 @@ -7,6 +7,7 @@ import com.datadog.debugger.instrumentation.InstrumentationResult; import com.datadog.debugger.probe.LogProbe; import com.datadog.debugger.probe.ProbeDefinition; +import com.datadog.debugger.probe.SpanDecorationProbe; import com.datadog.debugger.probe.Where; import com.datadog.debugger.util.ExceptionHelper; import datadog.trace.agent.tooling.AgentStrategies; @@ -462,19 +463,23 @@ private InstrumentationResult applyInstrumentation( preCheckInstrumentation(diagnostics, classLoader, methodNode); if (status != InstrumentationResult.Status.ERROR) { try { - List logProbes = new ArrayList<>(); + List capturedContextProbes = new ArrayList<>(); for (ProbeDefinition definition : definitions) { - if (definition instanceof LogProbe) { - logProbes.add(definition); + // Log and span decoration probe shared the same instrumentor: CaptureContextInstrumentor + // and therefore need to be instrumented once + if (definition instanceof LogProbe || definition instanceof SpanDecorationProbe) { + capturedContextProbes.add(definition); } else { List probeDiagnostics = diagnostics.get(definition.getProbeId()); definition.instrument(classLoader, classNode, methodNode, probeDiagnostics); } } - if (logProbes.size() > 0) { - List probesIds = logProbes.stream().map(ProbeDefinition::getId).collect(toList()); - List probeDiagnostics = diagnostics.get(logProbes.get(0).getProbeId()); - logProbes + if (capturedContextProbes.size() > 0) { + List probesIds = + capturedContextProbes.stream().map(ProbeDefinition::getId).collect(toList()); + List probeDiagnostics = + diagnostics.get(capturedContextProbes.get(0).getProbeId()); + capturedContextProbes .get(0) .instrument(classLoader, classNode, methodNode, probeDiagnostics, probesIds); } diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LogInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LogInstrumentor.java deleted file mode 100644 index 7b234703b3b..00000000000 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/LogInstrumentor.java +++ /dev/null @@ -1,37 +0,0 @@ -package com.datadog.debugger.instrumentation; - -import static com.datadog.debugger.probe.LogProbe.Capture.toLimits; - -import com.datadog.debugger.probe.LogProbe; -import java.util.List; -import org.objectweb.asm.tree.ClassNode; -import org.objectweb.asm.tree.MethodNode; - -/** Handles generating instrumentation for snapshot/log method & line probes */ -public final class LogInstrumentor extends CapturedContextInstrumentor { - private final LogProbe.Capture capture; - - public LogInstrumentor( - LogProbe logProbe, - ClassLoader classLoader, - ClassNode classNode, - MethodNode methodNode, - List diagnostics, - List probeIds) { - super( - logProbe, - classLoader, - classNode, - methodNode, - diagnostics, - probeIds, - logProbe.isCaptureSnapshot(), - toLimits(logProbe.getCapture())); - this.capture = logProbe.getCapture(); - } - - @Override - public void instrument() { - super.instrument(); - } -} diff --git a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/SpanDecorationInstrumentor.java b/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/SpanDecorationInstrumentor.java deleted file mode 100644 index 2c1722a9722..00000000000 --- a/dd-java-agent/agent-debugger/src/main/java/com/datadog/debugger/instrumentation/SpanDecorationInstrumentor.java +++ /dev/null @@ -1,24 +0,0 @@ -package com.datadog.debugger.instrumentation; - -import com.datadog.debugger.probe.SpanDecorationProbe; -import datadog.trace.bootstrap.debugger.Limits; -import java.util.List; -import org.objectweb.asm.tree.ClassNode; -import org.objectweb.asm.tree.MethodNode; - -public class SpanDecorationInstrumentor extends CapturedContextInstrumentor { - public SpanDecorationInstrumentor( - SpanDecorationProbe probe, - ClassLoader classLoader, - ClassNode classNode, - MethodNode methodNode, - List diagnostics, - List probeIds) { - super(probe, classLoader, classNode, methodNode, diagnostics, probeIds, false, Limits.DEFAULT); - } - - @Override - public void instrument() { - super.instrument(); - } -} 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 6eab9ca0b2b..fccbef1ff16 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 @@ -1,13 +1,15 @@ package com.datadog.debugger.probe; +import static com.datadog.debugger.probe.LogProbe.Capture.toLimits; + import com.datadog.debugger.agent.DebuggerAgent; import com.datadog.debugger.agent.Generated; import com.datadog.debugger.agent.LogMessageTemplateBuilder; import com.datadog.debugger.el.EvaluationException; import com.datadog.debugger.el.ProbeCondition; import com.datadog.debugger.el.ValueScript; +import com.datadog.debugger.instrumentation.CapturedContextInstrumentor; import com.datadog.debugger.instrumentation.DiagnosticMessage; -import com.datadog.debugger.instrumentation.LogInstrumentor; import com.datadog.debugger.sink.Sink; import com.datadog.debugger.sink.Snapshot; import com.squareup.moshi.Json; @@ -347,7 +349,15 @@ public void instrument( MethodNode methodNode, List diagnostics, List probeIds) { - new LogInstrumentor(this, classLoader, classNode, methodNode, diagnostics, probeIds) + new CapturedContextInstrumentor( + this, + classLoader, + classNode, + methodNode, + diagnostics, + probeIds, + isCaptureSnapshot(), + 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 4313a6b7c4d..d843fdc75ad 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 @@ -5,12 +5,13 @@ import com.datadog.debugger.agent.LogMessageTemplateBuilder; import com.datadog.debugger.el.EvaluationException; import com.datadog.debugger.el.ProbeCondition; +import com.datadog.debugger.instrumentation.CapturedContextInstrumentor; import com.datadog.debugger.instrumentation.DiagnosticMessage; -import com.datadog.debugger.instrumentation.SpanDecorationInstrumentor; import com.datadog.debugger.sink.Snapshot; import datadog.trace.api.Pair; import datadog.trace.bootstrap.debugger.CapturedContext; import datadog.trace.bootstrap.debugger.EvaluationError; +import datadog.trace.bootstrap.debugger.Limits; import datadog.trace.bootstrap.debugger.MethodLocation; import datadog.trace.bootstrap.debugger.ProbeId; import datadog.trace.bootstrap.debugger.ProbeImplementation; @@ -137,7 +138,8 @@ public void instrument( MethodNode methodNode, List diagnostics, List probeIds) { - new SpanDecorationInstrumentor(this, classLoader, classNode, methodNode, diagnostics, probeIds) + new CapturedContextInstrumentor( + this, classLoader, classNode, methodNode, diagnostics, probeIds, false, Limits.DEFAULT) .instrument(); } diff --git a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SpanDecorationProbeInstrumentationTest.java b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SpanDecorationProbeInstrumentationTest.java index 1e09cc7a4f9..0413631fe9e 100644 --- a/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SpanDecorationProbeInstrumentationTest.java +++ b/dd-java-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/SpanDecorationProbeInstrumentationTest.java @@ -6,8 +6,10 @@ import static com.datadog.debugger.probe.SpanDecorationProbe.TargetSpan.ACTIVE; import static com.datadog.debugger.probe.SpanDecorationProbe.TargetSpan.ROOT; import static com.datadog.debugger.util.LogProbeTestHelper.parseTemplate; +import static java.util.Collections.singletonList; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -16,12 +18,14 @@ import com.datadog.debugger.el.DSL; import com.datadog.debugger.el.ProbeCondition; import com.datadog.debugger.el.expressions.BooleanExpression; +import com.datadog.debugger.probe.LogProbe; import com.datadog.debugger.probe.SpanDecorationProbe; import com.datadog.debugger.sink.Snapshot; import datadog.trace.agent.tooling.TracerInstaller; import datadog.trace.api.Config; import datadog.trace.api.interceptor.MutableSpan; import datadog.trace.api.interceptor.TraceInterceptor; +import datadog.trace.bootstrap.debugger.CapturedContext; import datadog.trace.bootstrap.debugger.DebuggerContext; import datadog.trace.bootstrap.debugger.MethodLocation; import datadog.trace.bootstrap.debugger.ProbeId; @@ -41,6 +45,8 @@ public class SpanDecorationProbeInstrumentationTest extends ProbeInstrumentationTest { private static final String LANGUAGE = "java"; private static final ProbeId PROBE_ID = new ProbeId("beae1807-f3b0-4ea8-a74f-826790c5e6f8", 0); + private static final ProbeId PROBE_ID1 = new ProbeId("beae1807-f3b0-4ea8-a74f-826790c5e6f6", 0); + private static final ProbeId PROBE_ID2 = new ProbeId("beae1807-f3b0-4ea8-a74f-826790c5e6f7", 0); private TestTraceInterceptor traceInterceptor = new TestTraceInterceptor(); @@ -59,7 +65,7 @@ public void methodActiveSpanSimpleTag() throws IOException, URISyntaxException { CLASS_NAME, ACTIVE, decoration, "process", "int (java.lang.String)"); Class testClass = compileAndLoadClass(CLASS_NAME); int result = Reflect.on(testClass).call("main", "1").get(); - assertEquals(42, result); + assertEquals(84, result); MutableSpan span = traceInterceptor.getFirstSpan(); assertEquals("1", span.getTags().get("tag1")); assertEquals(PROBE_ID.getId(), span.getTags().get("_dd.di.tag1.probe_id")); @@ -81,7 +87,7 @@ public void methodActiveSpanTagList() throws IOException, URISyntaxException { "int (java.lang.String)"); Class testClass = compileAndLoadClass(CLASS_NAME); int result = Reflect.on(testClass).call("main", "1").get(); - assertEquals(42, result); + assertEquals(84, result); MutableSpan span = traceInterceptor.getFirstSpan(); assertEquals("1", span.getTags().get("tag1")); assertEquals("42", span.getTags().get("tag2")); @@ -124,7 +130,7 @@ public void methodActiveSpanCondition() throws IOException, URISyntaxException { Class testClass = compileAndLoadClass(CLASS_NAME); for (int i = 0; i < 10; i++) { int result = Reflect.on(testClass).call("main", String.valueOf(i)).get(); - assertEquals(42, result); + assertEquals(84, result); } assertEquals(10, traceInterceptor.getAllTraces().size()); MutableSpan span = traceInterceptor.getAllTraces().get(5).get(0); @@ -139,7 +145,7 @@ public void methodTagEvalError() throws IOException, URISyntaxException { CLASS_NAME, ACTIVE, decoration, "process", "int (java.lang.String)"); Class testClass = compileAndLoadClass(CLASS_NAME); int result = Reflect.on(testClass).call("main", "1").get(); - assertEquals(42, result); + assertEquals(84, result); MutableSpan span = traceInterceptor.getFirstSpan(); assertNull(span.getTags().get("tag1")); assertEquals("Cannot find symbol: noarg", span.getTags().get("_dd.di.tag1.evaluation_error")); @@ -154,7 +160,7 @@ public void methodActiveSpanInvalidCondition() throws IOException, URISyntaxExce CLASS_NAME, ACTIVE, decoration, "process", "int (java.lang.String)"); Class testClass = compileAndLoadClass(CLASS_NAME); int result = Reflect.on(testClass).call("main", "5").get(); - assertEquals(42, result); + assertEquals(84, result); assertFalse(traceInterceptor.getFirstSpan().getTags().containsKey("tag1")); assertEquals(1, mockSink.getSnapshots().size()); Snapshot snapshot = mockSink.getSnapshots().get(0); @@ -166,10 +172,10 @@ public void methodActiveSpanInvalidCondition() throws IOException, URISyntaxExce public void lineActiveSpanSimpleTag() throws IOException, URISyntaxException { final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20"; SpanDecorationProbe.Decoration decoration = createDecoration("tag1", "{arg}"); - installSingleSpanDecoration(CLASS_NAME, ACTIVE, decoration, "CapturedSnapshot20.java", 37); + installSingleSpanDecoration(CLASS_NAME, ACTIVE, decoration, "CapturedSnapshot20.java", 38); Class testClass = compileAndLoadClass(CLASS_NAME); int result = Reflect.on(testClass).call("main", "1").get(); - assertEquals(42, result); + assertEquals(84, result); MutableSpan span = traceInterceptor.getFirstSpan(); assertEquals("1", span.getTags().get("tag1")); assertEquals(PROBE_ID.getId(), span.getTags().get("_dd.di.tag1.probe_id")); @@ -198,11 +204,11 @@ public void lineActiveSpanCondition() throws IOException, URISyntaxException { final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20"; SpanDecorationProbe.Decoration decoration = createDecoration(eq(ref("arg"), value("5")), "arg == '5'", "tag1", "{arg}"); - installSingleSpanDecoration(CLASS_NAME, ACTIVE, decoration, "CapturedSnapshot20.java", 37); + installSingleSpanDecoration(CLASS_NAME, ACTIVE, decoration, "CapturedSnapshot20.java", 38); Class testClass = compileAndLoadClass(CLASS_NAME); for (int i = 0; i < 10; i++) { int result = Reflect.on(testClass).call("main", String.valueOf(i)).get(); - assertEquals(42, result); + assertEquals(84, result); } assertEquals(10, traceInterceptor.getAllTraces().size()); MutableSpan span = traceInterceptor.getAllTraces().get(5).get(0); @@ -214,10 +220,10 @@ public void lineActiveSpanInvalidCondition() throws IOException, URISyntaxExcept final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20"; SpanDecorationProbe.Decoration decoration = createDecoration(eq(ref("noarg"), value("5")), "arg == '5'", "tag1", "{arg}"); - installSingleSpanDecoration(CLASS_NAME, ACTIVE, decoration, "CapturedSnapshot20.java", 37); + installSingleSpanDecoration(CLASS_NAME, ACTIVE, decoration, "CapturedSnapshot20.java", 38); Class testClass = compileAndLoadClass(CLASS_NAME); int result = Reflect.on(testClass).call("main", "5").get(); - assertEquals(42, result); + assertEquals(84, result); assertFalse(traceInterceptor.getFirstSpan().getTags().containsKey("tag1")); assertEquals(1, mockSink.getSnapshots().size()); Snapshot snapshot = mockSink.getSnapshots().get(0); @@ -236,6 +242,48 @@ public void nullActiveSpan() throws IOException, URISyntaxException { assertEquals(0, traceInterceptor.getAllTraces().size()); } + @Test + public void mixedWithLogProbes() throws IOException, URISyntaxException { + final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20"; + SpanDecorationProbe.Decoration decoration = createDecoration("tag1", "{intLocal}"); + SpanDecorationProbe spanDecoProbe = + createProbeBuilder( + PROBE_ID, ACTIVE, singletonList(decoration), CLASS_NAME, "process", null, null) + .evaluateAt(MethodLocation.EXIT) + .build(); + LogProbe logProbe1 = + LogProbe.builder() + .probeId(PROBE_ID1) + .where(CLASS_NAME, "process") + .captureSnapshot(true) + .build(); + LogProbe logProbe2 = + LogProbe.builder() + .probeId(PROBE_ID2) + .where(CLASS_NAME, "process") + .captureSnapshot(true) + .build(); + Configuration configuration = + Configuration.builder() + .setService(SERVICE_NAME) + .add(logProbe1) + .add(logProbe2) + .add(spanDecoProbe) + .build(); + installSpanDecorationProbes(CLASS_NAME, configuration); + Class testClass = compileAndLoadClass(CLASS_NAME); + int result = Reflect.on(testClass).call("main", "1").get(); + assertEquals(84, result); + MutableSpan span = traceInterceptor.getFirstSpan(); + assertEquals("84", span.getTags().get("tag1")); + assertEquals(PROBE_ID.getId(), span.getTags().get("_dd.di.tag1.probe_id")); + List snapshots = mockSink.getSnapshots(); + assertEquals(2, snapshots.size()); + CapturedContext.CapturedValue intLocal = + snapshots.get(0).getCaptures().getReturn().getLocals().get("intLocal"); + assertNotNull(intLocal); + } + private SpanDecorationProbe.Decoration createDecoration(String tagName, String valueDsl) { List tags = Arrays.asList( @@ -305,6 +353,7 @@ private static SpanDecorationProbe createProbe( String... lines) { return createProbeBuilder( id, targetSpan, decorationList, typeName, methodName, signature, lines) + .evaluateAt(MethodLocation.EXIT) .build(); } @@ -367,19 +416,19 @@ private void installSpanDecorationProbes(String expectedClassName, Configuration mockSink = new MockSink(); DebuggerAgentHelper.injectSink(mockSink); DebuggerContext.init( - (id, callingClass) -> - resolver(id, callingClass, expectedClassName, configuration.getSpanDecorationProbes()), - null); + (id, callingClass) -> resolver(id, callingClass, expectedClassName, configuration), null); DebuggerContext.initClassFilter(new DenyListHelper(null)); } private ProbeImplementation resolver( - String id, - Class callingClass, - String expectedClassName, - Collection spanDecorationProbes) { + String id, Class callingClass, String expectedClassName, Configuration configuration) { Assertions.assertEquals(expectedClassName, callingClass.getName()); - for (SpanDecorationProbe probe : spanDecorationProbes) { + for (SpanDecorationProbe probe : configuration.getSpanDecorationProbes()) { + if (probe.getId().equals(id)) { + return probe; + } + } + for (LogProbe probe : configuration.getLogProbes()) { if (probe.getId().equals(id)) { return probe; } diff --git a/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot20.java b/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot20.java index 4ceff843986..07e5fa48828 100644 --- a/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot20.java +++ b/dd-java-agent/agent-debugger/src/test/resources/com/datadog/debugger/CapturedSnapshot20.java @@ -34,6 +34,7 @@ public static int main(String arg) { } private int process(String arg) { - return intField; + int intLocal = intField + 42; + return intLocal; } } From 6e5b6b7b23f46808c69f5f4c477b79162f01ad90 Mon Sep 17 00:00:00 2001 From: jean-philippe bempel Date: Wed, 30 Aug 2023 13:52:15 +0200 Subject: [PATCH 2/2] enforce log probe selection --- .../debugger/agent/DebuggerTransformer.java | 22 +++++++++++++++---- 1 file changed, 18 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 d587537192a..0f0e99b5cfb 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 @@ -477,11 +477,11 @@ private InstrumentationResult applyInstrumentation( if (capturedContextProbes.size() > 0) { List probesIds = capturedContextProbes.stream().map(ProbeDefinition::getId).collect(toList()); + ProbeDefinition referenceDefinition = selectReferenceDefinition(capturedContextProbes); List probeDiagnostics = - diagnostics.get(capturedContextProbes.get(0).getProbeId()); - capturedContextProbes - .get(0) - .instrument(classLoader, classNode, methodNode, probeDiagnostics, probesIds); + diagnostics.get(referenceDefinition.getProbeId()); + referenceDefinition.instrument( + classLoader, classNode, methodNode, probeDiagnostics, probesIds); } } catch (Throwable t) { log.warn("Exception during instrumentation: ", t); @@ -493,6 +493,20 @@ private InstrumentationResult applyInstrumentation( return new InstrumentationResult(status, diagnostics, classNode, methodNode); } + // Log & Span Decoration probes share the same instrumentor so only one definition should be + // selected to + // generate the instrumentation. Log probes needs capture limits provided by the configuration + // so if the list of definition contains at least 1 log probe this is the log probe that need to + // be picked. + // TODO: handle the conflicting limits for log probes + mixing CaptureSnapshot or not + private ProbeDefinition selectReferenceDefinition(List capturedContextProbes) { + ProbeDefinition first = capturedContextProbes.get(0); + return capturedContextProbes.stream() + .filter(it -> it instanceof LogProbe) + .findFirst() + .orElse(first); + } + private InstrumentationResult.Status preCheckInstrumentation( Map> diagnostics, ClassLoader classLoader,