-
Notifications
You must be signed in to change notification settings - Fork 278
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.
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
Fix mixing log/span decoration probes #7246
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ | |
import com.datadog.debugger.util.ExceptionHelper; | ||
import datadog.trace.agent.tooling.AgentStrategies; | ||
import datadog.trace.api.Config; | ||
import datadog.trace.bootstrap.debugger.MethodLocation; | ||
import datadog.trace.bootstrap.debugger.ProbeId; | ||
import datadog.trace.bootstrap.debugger.ProbeImplementation; | ||
import datadog.trace.util.Strings; | ||
|
@@ -515,7 +516,8 @@ private InstrumentationResult applyInstrumentation( | |
InstrumentationResult.Status status = preCheckInstrumentation(diagnostics, methodInfo); | ||
if (status != InstrumentationResult.Status.ERROR) { | ||
try { | ||
List<ToInstrumentInfo> toInstruments = filterAndSortDefinitions(definitions); | ||
List<ToInstrumentInfo> toInstruments = | ||
filterAndSortDefinitions(definitions, methodInfo.getClassFileLines()); | ||
for (ToInstrumentInfo toInstrumentInfo : toInstruments) { | ||
ProbeDefinition definition = toInstrumentInfo.definition; | ||
List<DiagnosticMessage> probeDiagnostics = diagnostics.get(definition.getProbeId()); | ||
|
@@ -541,7 +543,8 @@ static class ToInstrumentInfo { | |
} | ||
} | ||
|
||
private List<ToInstrumentInfo> filterAndSortDefinitions(List<ProbeDefinition> definitions) { | ||
private List<ToInstrumentInfo> filterAndSortDefinitions( | ||
List<ProbeDefinition> definitions, ClassFileLines classFileLines) { | ||
List<ToInstrumentInfo> toInstrument = new ArrayList<>(); | ||
List<ProbeDefinition> capturedContextProbes = new ArrayList<>(); | ||
boolean addedExceptionProbe = false; | ||
|
@@ -566,7 +569,8 @@ private List<ToInstrumentInfo> filterAndSortDefinitions(List<ProbeDefinition> de | |
if (!capturedContextProbes.isEmpty()) { | ||
List<ProbeId> probesIds = | ||
capturedContextProbes.stream().map(ProbeDefinition::getProbeId).collect(toList()); | ||
ProbeDefinition referenceDefinition = selectReferenceDefinition(capturedContextProbes); | ||
ProbeDefinition referenceDefinition = | ||
selectReferenceDefinition(capturedContextProbes, classFileLines); | ||
toInstrument.add(new ToInstrumentInfo(referenceDefinition, probesIds)); | ||
} | ||
// LOGGER.debug("exception probe is already instrumented for {}", preciseWhere); | ||
|
@@ -581,17 +585,73 @@ private List<ToInstrumentInfo> filterAndSortDefinitions(List<ProbeDefinition> de | |
} | ||
|
||
// 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<ProbeDefinition> capturedContextProbes) { | ||
ProbeDefinition first = capturedContextProbes.get(0); | ||
return capturedContextProbes.stream() | ||
.filter(it -> it instanceof LogProbe) | ||
.findFirst() | ||
.orElse(first); | ||
// used to generate the instrumentation. This method generate a synthetic definition that | ||
// match the type of the probe to instrument: if at least one probe is LogProbe then we are | ||
// creating a LogProbe definition. The synthetic definition contains the union of all the capture, | ||
// snapshot and evaluateAt parameters. | ||
private ProbeDefinition selectReferenceDefinition( | ||
List<ProbeDefinition> capturedContextProbes, ClassFileLines classFileLines) { | ||
boolean hasLogProbe = false; | ||
MethodLocation evaluateAt = MethodLocation.EXIT; | ||
LogProbe.Capture capture = null; | ||
boolean captureSnapshot = false; | ||
Where where = capturedContextProbes.get(0).getWhere(); | ||
ProbeId probeId = capturedContextProbes.get(0).getProbeId(); | ||
for (ProbeDefinition definition : capturedContextProbes) { | ||
if (definition instanceof LogProbe) { | ||
if (definition instanceof ExceptionProbe) { | ||
where = Where.convertLineToMethod(where, classFileLines); | ||
} | ||
hasLogProbe = true; | ||
LogProbe logProbe = (LogProbe) definition; | ||
captureSnapshot = captureSnapshot | logProbe.isCaptureSnapshot(); | ||
capture = mergeCapture(capture, logProbe.getCapture()); | ||
} | ||
if (definition.getEvaluateAt() == MethodLocation.ENTRY | ||
|| definition.getEvaluateAt() == MethodLocation.DEFAULT) { | ||
evaluateAt = definition.getEvaluateAt(); | ||
} | ||
} | ||
if (hasLogProbe) { | ||
return LogProbe.builder() | ||
.probeId(probeId) | ||
.where(where) | ||
.evaluateAt(evaluateAt) | ||
.capture(capture) | ||
.captureSnapshot(captureSnapshot) | ||
.build(); | ||
} | ||
return SpanDecorationProbe.builder() | ||
.probeId(probeId) | ||
.where(where) | ||
.evaluateAt(evaluateAt) | ||
.build(); | ||
} | ||
|
||
private LogProbe.Sampling mergeSampling( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we care about merge the sampling? I thought we rate-limit each probe independently.. do we use the sampling of the reference definition? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. left over. this method is not called. removed |
||
LogProbe.Sampling current, LogProbe.Sampling newSampling) { | ||
if (current == null) { | ||
return newSampling; | ||
} | ||
if (newSampling == null) { | ||
return current; | ||
} | ||
return new LogProbe.Sampling( | ||
Math.max(current.getSnapshotsPerSecond(), newSampling.getSnapshotsPerSecond())); | ||
} | ||
|
||
private LogProbe.Capture mergeCapture(LogProbe.Capture current, LogProbe.Capture newCapture) { | ||
if (current == null) { | ||
return newCapture; | ||
} | ||
if (newCapture == null) { | ||
return current; | ||
} | ||
return new LogProbe.Capture( | ||
Math.max(current.getMaxReferenceDepth(), newCapture.getMaxReferenceDepth()), | ||
Math.max(current.getMaxCollectionSize(), newCapture.getMaxCollectionSize()), | ||
Math.max(current.getMaxLength(), newCapture.getMaxLength()), | ||
Math.max(current.getMaxFieldCount(), newCapture.getMaxFieldCount())); | ||
} | ||
|
||
private InstrumentationResult.Status preCheckInstrumentation( | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -356,25 +356,36 @@ public void ordering() { | |
.add(metricProbe) | ||
.add(logProbe) | ||
.build(); | ||
DebuggerTransformer debuggerTransformer = new DebuggerTransformer(config, configuration); | ||
DebuggerTransformer debuggerTransformer = | ||
new DebuggerTransformer( | ||
config, | ||
configuration, | ||
(definition, result) -> { | ||
if (result.isInstalled()) { | ||
invocationOrder.add(definition); | ||
} | ||
}, | ||
new DebuggerSink( | ||
config, new ProbeStatusSink(config, config.getFinalDebuggerSnapshotUrl(), false))); | ||
debuggerTransformer.transform( | ||
ClassLoader.getSystemClassLoader(), | ||
ArrayList.class.getName(), // always FQN | ||
ArrayList.class, | ||
null, | ||
getClassFileBytes(ArrayList.class)); | ||
assertEquals(3, invocationOrder.size()); | ||
assertEquals(4, invocationOrder.size()); | ||
assertEquals(metricProbe, invocationOrder.get(0)); | ||
assertEquals(logProbe, invocationOrder.get(1)); | ||
assertEquals(spanProbe, invocationOrder.get(2)); | ||
assertEquals(spanDecorationProbe, invocationOrder.get(3)); | ||
} | ||
|
||
<T extends ProbeDefinition> T createMock( | ||
Class<T> clazz, List<ProbeDefinition> invocationOrder, String id) { | ||
ProbeDefinition mock = mock(clazz); | ||
doAnswer( | ||
invocation -> { | ||
invocationOrder.add(mock); | ||
// invocationOrder.add(mock); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. remove? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
return InstrumentationResult.Status.INSTALLED; | ||
}) | ||
.when(mock) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -55,6 +55,8 @@ public class SpanDecorationProbeInstrumentationTest extends ProbeInstrumentation | |
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 static final ProbeId PROBE_ID3 = new ProbeId("beae1807-f3b0-4ea8-a74f-826790c5e6f8", 0); | ||
private static final ProbeId PROBE_ID4 = new ProbeId("beae1807-f3b0-4ea8-a74f-826790c5e6f9", 0); | ||
|
||
private TestTraceInterceptor traceInterceptor = new TestTraceInterceptor(); | ||
|
||
|
@@ -312,38 +314,51 @@ public void nullActiveSpan() throws IOException, URISyntaxException { | |
@Test | ||
public void mixedWithLogProbes() throws IOException, URISyntaxException { | ||
final String CLASS_NAME = "com.datadog.debugger.CapturedSnapshot20"; | ||
SpanDecorationProbe.Decoration decoration = createDecoration("tag1", "{intLocal}"); | ||
SpanDecorationProbe spanDecoProbe = | ||
SpanDecorationProbe.Decoration decoration1 = createDecoration("tag1", "{intLocal}"); | ||
SpanDecorationProbe.Decoration decoration2 = createDecoration("tag2", "{arg}"); | ||
SpanDecorationProbe spanDecoProbe1 = | ||
createProbeBuilder( | ||
PROBE_ID, ACTIVE, singletonList(decoration), CLASS_NAME, "process", null, null) | ||
PROBE_ID1, ACTIVE, singletonList(decoration1), CLASS_NAME, "process", null, null) | ||
.evaluateAt(MethodLocation.EXIT) | ||
.build(); | ||
SpanDecorationProbe spanDecoProbe2 = | ||
createProbeBuilder( | ||
PROBE_ID2, ACTIVE, singletonList(decoration2), CLASS_NAME, "process", null, null) | ||
.evaluateAt(MethodLocation.ENTRY) | ||
.build(); | ||
LogProbe logProbe1 = | ||
LogProbe.builder() | ||
.probeId(PROBE_ID1) | ||
.probeId(PROBE_ID3) | ||
.where(CLASS_NAME, "process") | ||
.captureSnapshot(true) | ||
.sampling(3.0) | ||
.capture(1, 50, 50, 10) | ||
.build(); | ||
LogProbe logProbe2 = | ||
LogProbe.builder() | ||
.probeId(PROBE_ID2) | ||
.probeId(PROBE_ID4) | ||
.where(CLASS_NAME, "process") | ||
.captureSnapshot(true) | ||
.sampling(10.0) | ||
.capture(5, 200, 200, 30) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. do we want to test that we merge the sampling and captures limit correctly? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added test |
||
.build(); | ||
Configuration configuration = | ||
Configuration.builder() | ||
.setService(SERVICE_NAME) | ||
.add(logProbe1) | ||
.add(logProbe2) | ||
.add(spanDecoProbe) | ||
.add(spanDecoProbe1) | ||
.add(spanDecoProbe2) | ||
.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")); | ||
assertEquals(PROBE_ID1.getId(), span.getTags().get("_dd.di.tag1.probe_id")); | ||
assertEquals("1", span.getTags().get("tag2")); | ||
assertEquals(PROBE_ID2.getId(), span.getTags().get("_dd.di.tag2.probe_id")); | ||
List<Snapshot> snapshots = mockSink.getSnapshots(); | ||
assertEquals(2, snapshots.size()); | ||
CapturedContext.CapturedValue intLocal = | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why we overwrite EXIT with default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there should be a priority here.. if there are two probes one with DEFAULT and one with ENTRY.. the order of us going over the probe would have determine which one wins.
Does this even matter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DEFAULT
==ENTRY
from the beginning.The fact that by default, UI put
EXIT
is independent of what we are doing here.