-
Notifications
You must be signed in to change notification settings - Fork 319
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
Implement outcome for transactions & spans #1613
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪💚 Flaky test reportTests succeeded. Expand to view the summary
Test stats 🧪
|
.../apm-servlet-plugin/src/main/java/co/elastic/apm/agent/servlet/ServletTransactionHelper.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
@Test | ||
void destroyWithoutProcessTerminatedShouldEndSpan() { | ||
Process process = mock(Process.class); | ||
verifyNoMoreInteractions(process); // we should not even use any method of process | ||
|
||
// we have to opt-in to allow unknown outcome | ||
reporter.checkUnknownOutcome(false); |
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.
@eyalkoren external process execution is one case where we might not always know the outcome if the process hasn't yet terminated. But I have to admit that it's quite likely to be a very narrow corner case.
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 this is a fair exception to make - if we never report unknown
to terminated processes and always report unknown
to unterminated processes it sounds reasonable.
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.
Looks great. The main comment is what we discussed about reflecting the revised spec where by default we set an outcome to spans, with some exceptions for specific span types.
Other than that, ResultUtilTest
requires updating.
apm-agent-core/src/main/java/co/elastic/apm/agent/impl/transaction/Span.java
Outdated
Show resolved
Hide resolved
apm-agent-core/src/test/java/co/elastic/apm/agent/MockReporter.java
Outdated
Show resolved
Hide resolved
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.
Once the tests pass, I am good with merging this PR.
If it's easy enough, switching to not allow unknown
outcome for spans in the MockReporter
and changing only for specific span types would be preferable as it means we cover all plugins, including future ones. If this complicates things, we can wait with that.
@@ -85,7 +85,7 @@ | |||
private final List<byte[]> bytes = new CopyOnWriteArrayList<>(); | |||
private final ObjectMapper objectMapper; | |||
private final boolean verifyJsonSchema; | |||
private boolean checkUnknownOutcomes = false; | |||
private boolean checkUnknownOutcomes = true; |
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.
❤️
What does this PR do?
Fixes #1354
Fixes #1298
Fixes #1280
Implementation checklist
add outcome to the following plugins:
Extra:
Checklist