Skip to content
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

Add evaluateAt support for probe instrumentations #4320

Merged
merged 3 commits into from
Nov 29, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,9 @@ private void addCapturingProbeId(ProbeDetails probe) {
public void setEntry(CapturedContext context) {
summaryBuilder.addEntry(context);
context.setThisClassName(thisClassName);
if (checkCapture(context)) {
if ((probe.getEvaluateAt() == MethodLocation.DEFAULT
|| probe.getEvaluateAt() == MethodLocation.ENTRY)
&& checkCapture(context)) {
captures.setEntry(context);
}
}
Expand All @@ -102,7 +104,9 @@ public void setExit(CapturedContext context) {
context.addExtension(ValueReferences.DURATION_EXTENSION_NAME, duration);
summaryBuilder.addExit(context);
context.setThisClassName(thisClassName);
if (checkCapture(context)) {
if ((probe.getEvaluateAt() == MethodLocation.DEFAULT
|| probe.getEvaluateAt() == MethodLocation.EXIT)
&& checkCapture(context)) {
captures.setReturn(context);
}
}
Expand Down Expand Up @@ -220,7 +224,8 @@ private Snapshot copy(String probeId, String newSnapshotId) {
duration,
stack,
captures,
new ProbeDetails(probeId, probe.location, probe.script, probe.tags, summaryBuilder),
new ProbeDetails(
probeId, probe.location, probe.evaluateAt, probe.script, probe.tags, summaryBuilder),
language,
thread,
thisClassName,
Expand Down Expand Up @@ -290,6 +295,12 @@ public enum Kind {
AFTER;
}

public enum MethodLocation {
DEFAULT,
ENTRY,
EXIT
}

/** Probe information associated with a snapshot */
public static class ProbeDetails {
public static final String ITW_PROBE_ID = "instrument-the-world-probe";
Expand All @@ -299,33 +310,44 @@ public static class ProbeDetails {

private final String id;
private final ProbeLocation location;
private final MethodLocation evaluateAt;
private final DebuggerScript script;
private final transient List<ProbeDetails> additionalProbes;
private final String tags;
private final transient SummaryBuilder summaryBuilder;

public ProbeDetails(String id, ProbeLocation location) {
this(id, location, null, null, new SnapshotSummaryBuilder(location), Collections.emptyList());
this(
id,
location,
MethodLocation.DEFAULT,
null,
null,
new SnapshotSummaryBuilder(location),
Collections.emptyList());
}

public ProbeDetails(
String id,
ProbeLocation location,
MethodLocation evaluateAt,
DebuggerScript script,
String tags,
SummaryBuilder summaryBuilder) {
this(id, location, script, tags, summaryBuilder, Collections.emptyList());
this(id, location, evaluateAt, script, tags, summaryBuilder, Collections.emptyList());
}

public ProbeDetails(
String id,
ProbeLocation location,
MethodLocation evaluateAt,
DebuggerScript script,
String tags,
SummaryBuilder summaryBuilder,
List<ProbeDetails> additionalProbes) {
this.id = id;
this.location = location;
this.evaluateAt = evaluateAt;
this.script = script;
this.additionalProbes = additionalProbes;
this.tags = tags;
Expand All @@ -340,6 +362,10 @@ public ProbeLocation getLocation() {
return location;
}

public MethodLocation getEvaluateAt() {
return evaluateAt;
}

public DebuggerScript getScript() {
return script;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -294,6 +294,7 @@ private Snapshot.ProbeDetails convertToProbeDetails(
return new Snapshot.ProbeDetails(
probe.getId(),
location,
convertMethodLocation(probe.getEvaluateAt()),
probeCondition,
probe.concatTags(),
summaryBuilder,
Expand All @@ -302,6 +303,19 @@ private Snapshot.ProbeDetails convertToProbeDetails(
.collect(Collectors.toList()));
}

private Snapshot.MethodLocation convertMethodLocation(
ProbeDefinition.MethodLocation methodLocation) {
switch (methodLocation) {
case DEFAULT:
return Snapshot.MethodLocation.DEFAULT;
case ENTRY:
return Snapshot.MethodLocation.ENTRY;
case EXIT:
return Snapshot.MethodLocation.EXIT;
}
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will this become a NPE in the caller?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmmm no, why?

}

private void applyRateLimiter(ConfigurationComparer changes) {
Collection<SnapshotProbe> probes = currentConfiguration.getSnapshotProbes();
if (probes == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ public class Instrumentor {
protected int localVarBaseOffset;
protected int argOffset;
protected final String[] argumentNames;
protected LabelNode returnHandlerLabel;

public Instrumentor(
ProbeDefinition definition,
Expand Down Expand Up @@ -134,6 +135,70 @@ protected void fillLineMap() {
}
}

protected void processInstructions() {
AbstractInsnNode node = methodNode.instructions.getFirst();
while (node != null && !node.equals(returnHandlerLabel)) {
if (node.getType() == AbstractInsnNode.LINE) {
lineMap.addLine((LineNumberNode) node);
} else {
node = processInstruction(node);
}
node = node.getNext();
}
if (returnHandlerLabel == null) {
// if no return found, fallback to use the last instruction as last resort
returnHandlerLabel = new LabelNode();
methodNode.instructions.insert(methodNode.instructions.getLast(), returnHandlerLabel);
}
}

protected AbstractInsnNode processInstruction(AbstractInsnNode node) {
switch (node.getOpcode()) {
case Opcodes.RET:
case Opcodes.RETURN:
case Opcodes.IRETURN:
case Opcodes.FRETURN:
case Opcodes.LRETURN:
case Opcodes.DRETURN:
case Opcodes.ARETURN:
{
InsnList beforeReturnInsnList = getBeforeReturnInsnList(node);
methodNode.instructions.insertBefore(node, beforeReturnInsnList);
AbstractInsnNode prev = node.getPrevious();
methodNode.instructions.remove(node);
methodNode.instructions.insert(
prev, new JumpInsnNode(Opcodes.GOTO, getReturnHandler(node)));
return prev;
}
}
return node;
}

protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) {
return null;
}

protected LabelNode getReturnHandler(AbstractInsnNode exitNode) {
// exit node must have been removed from the original instruction list
if (exitNode.getNext() != null || exitNode.getPrevious() != null) {
throw new IllegalArgumentException("exitNode is not removed from original instruction list");
}
if (returnHandlerLabel != null) {
return returnHandlerLabel;
}
returnHandlerLabel = new LabelNode();
methodNode.instructions.add(returnHandlerLabel);
// stack top is return value (if any)
InsnList handler = getReturnHandlerInsnList();
handler.add(exitNode); // stack: []
methodNode.instructions.add(handler);
return returnHandlerLabel;
}

protected InsnList getReturnHandlerInsnList() {
return new InsnList();
}

protected static void invokeStatic(
InsnList insnList, Type owner, String name, Type returnType, Type... argTypes) {
// expected stack: [arg_type_1 ... arg_type_N]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
import java.util.regex.Pattern;
import org.objectweb.asm.Opcodes;
import org.objectweb.asm.Type;
import org.objectweb.asm.tree.AbstractInsnNode;
import org.objectweb.asm.tree.ClassNode;
import org.objectweb.asm.tree.FieldInsnNode;
import org.objectweb.asm.tree.FieldNode;
Expand Down Expand Up @@ -55,11 +56,31 @@ public void instrument() {
fillLineMap();
addLineMetric(lineMap);
} else {
InsnList insnList = callMetric(metricProbe);
methodNode.instructions.insert(methodEnterLabel, insnList);
switch (definition.getEvaluateAt()) {
case ENTRY:
case DEFAULT:
{
InsnList insnList = callMetric(metricProbe);
methodNode.instructions.insert(methodEnterLabel, insnList);
break;
}
case EXIT:
{
processInstructions();
break;
}
default:
throw new IllegalArgumentException(
"Invalid evaluateAt attribute: " + definition.getEvaluateAt());
}
}
}

@Override
protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) {
return callMetric(metricProbe);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this callMetric would have access to @return and @duration?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would we be able to add condition for metrics in this fashion as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will this callMetric would have access to @return and @duration?

Nope, they are not introduced in this context, yet

would we be able to add condition for metrics in this fashion as well?

yes it's possible, need some work, though

}

private InsnList callCount(MetricProbe metricProbe) {
InsnList insnList = new InsnList();
if (metricProbe.getValue() == null) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import static org.objectweb.asm.Type.INT_TYPE;

import com.datadog.debugger.probe.LogProbe;
import com.datadog.debugger.probe.ProbeDefinition;
import com.datadog.debugger.probe.SnapshotProbe;
import com.datadog.debugger.probe.Where;
import datadog.trace.api.Config;
Expand All @@ -31,7 +32,6 @@
import org.objectweb.asm.tree.InsnNode;
import org.objectweb.asm.tree.JumpInsnNode;
import org.objectweb.asm.tree.LabelNode;
import org.objectweb.asm.tree.LineNumberNode;
import org.objectweb.asm.tree.LocalVariableNode;
import org.objectweb.asm.tree.MethodInsnNode;
import org.objectweb.asm.tree.MethodNode;
Expand All @@ -45,7 +45,6 @@ public final class SnapshotInstrumentor extends Instrumentor {
private final boolean captureFullState;
private final LabelNode snapshotInitLabel = new LabelNode();
private int snapshotVar = -1;
private LabelNode returnHandlerLabel = null;

public SnapshotInstrumentor(
SnapshotProbe snapshotProbe,
Expand Down Expand Up @@ -81,45 +80,6 @@ public void instrument() {
}
}

private void processInstructions() {
AbstractInsnNode node = methodNode.instructions.getFirst();
while (node != null && !node.equals(returnHandlerLabel)) {
if (node.getType() == AbstractInsnNode.LINE) {
lineMap.addLine((LineNumberNode) node);
} else {
node = processInstruction(node);
}
node = node.getNext();
}
if (returnHandlerLabel == null) {
// if no return found, fallback to use the last instruction as last resort
returnHandlerLabel = new LabelNode();
methodNode.instructions.insert(methodNode.instructions.getLast(), returnHandlerLabel);
}
}

private AbstractInsnNode processInstruction(AbstractInsnNode node) {
switch (node.getOpcode()) {
case Opcodes.RET:
case Opcodes.RETURN:
case Opcodes.IRETURN:
case Opcodes.FRETURN:
case Opcodes.LRETURN:
case Opcodes.DRETURN:
case Opcodes.ARETURN:
{
methodNode.instructions.insertBefore(
node, collectSnapshotCapture(-1, Snapshot.Kind.RETURN, node));
AbstractInsnNode prev = node.getPrevious();
methodNode.instructions.remove(node);
methodNode.instructions.insert(
prev, new JumpInsnNode(Opcodes.GOTO, getReturnHandler(node)));
return prev;
}
}
return node;
}

private void addLineCaptures(LineMap lineMap) {
Where.SourceLine[] targetLines = definition.getWhere().getSourceLines();
if (targetLines == null) {
Expand Down Expand Up @@ -156,19 +116,14 @@ private void addLineCaptures(LineMap lineMap) {
}
}

private LabelNode getReturnHandler(AbstractInsnNode exitNode) {
// exit node must have been removed from the original instruction list
assert exitNode.getNext() == null && exitNode.getPrevious() == null;
if (returnHandlerLabel != null) {
return returnHandlerLabel;
}
returnHandlerLabel = new LabelNode();
methodNode.instructions.add(returnHandlerLabel);
// stack top is return value (if any)
InsnList handler = commitSnapshot();
handler.add(exitNode); // stack: []
methodNode.instructions.add(handler);
return returnHandlerLabel;
@Override
protected InsnList getBeforeReturnInsnList(AbstractInsnNode node) {
return collectSnapshotCapture(-1, Snapshot.Kind.RETURN, node);
}

@Override
protected InsnList getReturnHandlerInsnList() {
return commitSnapshot();
}

private InsnList commitSnapshot() {
Expand Down Expand Up @@ -203,8 +158,16 @@ private void addFinallyHandler(LabelNode endLabel) {
}

private void instrumentMethodEnter() {
methodNode.instructions.insert(
snapshotInitLabel, collectSnapshotCapture(-1, Snapshot.Kind.ENTER, null));
InsnList insnList;
if (definition.getEvaluateAt() == ProbeDefinition.MethodLocation.EXIT) {
// if evaluation is at exit, skip collecting data at enter
// but create the snapshot at entry anyway
insnList = new InsnList();
getSnapshot(insnList);
} else {
insnList = collectSnapshotCapture(-1, Snapshot.Kind.ENTER, null);
}
methodNode.instructions.insert(snapshotInitLabel, insnList);
}

private void instrumentTryCatchHandlers() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ public String toString() {
// no-arg constructor is required by Moshi to avoid creating instance with unsafe and by-passing
// constructors, including field initializers.
public LogProbe() {
this(LANGUAGE, null, true, null, null, MethodLocation.NONE, null, new ArrayList<>());
this(LANGUAGE, null, true, null, null, MethodLocation.DEFAULT, null, new ArrayList<>());
}

public LogProbe(
Expand Down Expand Up @@ -180,7 +180,7 @@ public static class Builder extends ProbeDefinition.Builder<Builder> {
private String template;
private List<Segment> segments;

public LogProbe.Builder template(String template) {
public Builder template(String template) {
this.template = template;
this.segments = parseTemplate(template);
return this;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public enum MetricKind {
// no-arg constructor is required by Moshi to avoid creating instance with unsafe and by-passing
// constructors, including field initializers.
public MetricProbe() {
this(LANGUAGE, null, true, null, null, MethodLocation.NONE, MetricKind.COUNT, null, null);
this(LANGUAGE, null, true, null, null, MethodLocation.DEFAULT, MetricKind.COUNT, null, null);
}

public MetricProbe(
Expand Down
Loading