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.

Sign up for GitHub

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

Jump to bottom

Fix sampling for line probe without condition #6135

Merged
merged 3 commits into from
Nov 2, 2023
Merged

Fix sampling for line probe without condition #6135

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
69b777a
Fix sampling for line probe without condition
jpbempel Nov 2, 2023
810c321
Factorize the line probe condition for evluateAt
jpbempel Nov 2, 2023
ef53842
add comment explaining the usage of DEFAULT
jpbempel Nov 2, 2023
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
12 changes: 2 additions & 10 deletions ...er/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/CapturedContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,22 +327,14 @@ public Status evaluate(
ValueReferences.DURATION_EXTENSION_NAME, duration / 1_000_000.0); // convert to ms
}
this.thisClassName = thisClassName;
boolean shouldEvaluate = resolveEvaluateAt(probeImplementation, methodLocation);
boolean shouldEvaluate =
MethodLocation.isSame(methodLocation, probeImplementation.getEvaluateAt());
if (shouldEvaluate) {
probeImplementation.evaluate(this, status, methodLocation);
}
return status;
}

private static boolean resolveEvaluateAt(
ProbeImplementation probeImplementation, MethodLocation methodLocation) {
if (methodLocation == MethodLocation.DEFAULT) {
// line probe, no evaluation of probe's evaluateAt
return true;
}
return MethodLocation.isSame(methodLocation, probeImplementation.getEvaluateAt());
}

public Status getStatus(String probeId) {
Status result = statusByProbeId.get(probeId);
if (result == null) {
Expand Down
5 changes: 5 additions & 0 deletions ...ger/debugger-bootstrap/src/main/java/datadog/trace/bootstrap/debugger/MethodLocation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,11 @@ public enum MethodLocation {
EXIT;

public static boolean isSame(MethodLocation methodLocation, MethodLocation evaluateAt) {
if (methodLocation == MethodLocation.DEFAULT) {
Copy link
Contributor

Choose a reason for hiding this comment

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

if we do this only for line probe. maybe the line probe logic should not call this instead of adding this case in the method?

Aka, the evaluate function should already have the probe definition, right? so that function can simply skip this if the probe location does not need to check method location

Copy link
Member Author

Choose a reason for hiding this comment

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

it centralizes the logic for both calls:

    LogStatus logStatus = (LogStatus) status;
    if (!hasCondition()) {
      // sample when no condition associated
      sample(logStatus, methodLocation);
    }
    logStatus.setCondition(evaluateCondition(context, logStatus));
    CapturedContext.CapturedThrowable throwable = context.getThrowable();
    if (logStatus.hasConditionErrors() && throwable != null) {
      logStatus.addError(
          new EvaluationError(
              "uncaught exception", throwable.getType() + ": " + throwable.getMessage()));
    }
    if (hasCondition() && logStatus.getCondition()) {
      // sample if probe has condition and condition is true
      sample(logStatus, methodLocation);
    }

Copy link
Member Author

Choose a reason for hiding this comment

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

actually it's better to centralize also the other isSame call

Copy link
Contributor

Choose a reason for hiding this comment

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

I would explain this in the comment. that MethodLocation.DEFAULT is used to mark that this is a line probe and therefore we don't check the probeLocation as it meaningless.

// line probe, no evaluation of probe's evaluateAt
// MethodLocation.DEFAULT is used for line probe when evaluating the context
return true;
}
if (methodLocation == MethodLocation.ENTRY) {
return evaluateAt == MethodLocation.DEFAULT || evaluateAt == MethodLocation.ENTRY;
}
Expand Down
20 changes: 18 additions & 2 deletions ...a-agent/agent-debugger/src/test/java/com/datadog/debugger/agent/CapturedSnapshotTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ public void methodProbe() throws IOException, URISyntaxException {
public void singleLineProbe() throws IOException, URISyntaxException {
final String CLASS_NAME = "CapturedSnapshot01";
DebuggerTransformerTest.TestSnapshotListener listener =
installSingleProbe(CLASS_NAME, "main", "int (java.lang.String)", "8");
installSingleProbeAtExit(CLASS_NAME, "main", "int (java.lang.String)", "8");
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.on(testClass).call("main", "1").get();
assertEquals(3, result);
Expand Down Expand Up @@ -1582,7 +1582,7 @@ private Snapshot doUnknownCount(String CLASS_NAME) throws IOException, URISyntax
public void beforeForLoopLineProbe() throws IOException, URISyntaxException {
final String CLASS_NAME = "CapturedSnapshot02";
DebuggerTransformerTest.TestSnapshotListener listener =
installSingleProbe(CLASS_NAME, null, null, "46");
installSingleProbeAtExit(CLASS_NAME, null, null, "46");
Class<?> testClass = compileAndLoadClass(CLASS_NAME);
int result = Reflect.on(testClass).call("main", "synchronizedBlock").get();
assertEquals(76, result);
Expand All @@ -1597,10 +1597,12 @@ public void dupLineProbeSameTemplate() throws IOException, URISyntaxException {
LogProbe probe1 =
createProbeBuilder(PROBE_ID1, CLASS_NAME, null, null, "39")
.template(LOG_TEMPLATE, parseTemplate(LOG_TEMPLATE))
.evaluateAt(MethodLocation.EXIT)
.build();
LogProbe probe2 =
createProbeBuilder(PROBE_ID2, CLASS_NAME, null, null, "39")
.template(LOG_TEMPLATE, parseTemplate(LOG_TEMPLATE))
.evaluateAt(MethodLocation.EXIT)
.build();
DebuggerTransformerTest.TestSnapshotListener listener =
installProbes(CLASS_NAME, probe1, probe2);
Expand Down Expand Up @@ -1910,6 +1912,12 @@ private DebuggerTransformerTest.TestSnapshotListener installSingleProbe(
return installProbes(typeName, logProbes);
}

private DebuggerTransformerTest.TestSnapshotListener installSingleProbeAtExit(
String typeName, String methodName, String signature, String... lines) {
LogProbe logProbes = createProbeAtExit(PROBE_ID, typeName, methodName, signature, lines);
return installProbes(typeName, logProbes);
}

private DebuggerTransformerTest.TestSnapshotListener installProbes(
String expectedClassName, Configuration configuration) {
Config config = mock(Config.class);
Expand Down Expand Up @@ -2169,6 +2177,13 @@ private static LogProbe createProbe(
return createProbeBuilder(id, typeName, methodName, signature, lines).build();
}

private static LogProbe createProbeAtExit(
ProbeId id, String typeName, String methodName, String signature, String... lines) {
return createProbeBuilder(id, typeName, methodName, signature, lines)
.evaluateAt(MethodLocation.EXIT)
.build();
}

private static LogProbe.Builder createProbeBuilder(
ProbeId id, String typeName, String methodName, String signature, String... lines) {
return LogProbe.builder()
Expand All @@ -2186,6 +2201,7 @@ private static LogProbe createSourceFileProbe(ProbeId id, String sourceFile, int
.probeId(id)
.captureSnapshot(true)
.where(null, null, null, line, sourceFile)
.evaluateAt(MethodLocation.EXIT)
.build();
}

Expand Down
Loading