-
Notifications
You must be signed in to change notification settings - Fork 12
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
Invoke onSuccess and onFailure handlers properly for async methods #181
Conversation
@Test | ||
public void testInstrumentCompletableFuture() { | ||
|
||
} |
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.
Did you intend to commit this?
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.
haha nope. removed :)
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 will break tracing-java and any other handler which sets and removes thread-local state. We need some way to opt into asynchronous callbacks to avoid breaking existing implementations.
There's a separate related conversation around how we want to support tracing-java for asynchronous operations.
private Object handleResult(InvocationContext context, Object result) { | ||
if (result instanceof ListenableFuture) { | ||
return handleFuture(context, (ListenableFuture) result); | ||
} else if (result instanceof CompletableFuture) { |
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.
Should use CompletionStage
over CompletableFuture
@cakofony cool. Let's find some time to work through the current issues and what a better solution would look like |
return false; | ||
} | ||
} | ||
return 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.
Tritium bundles all handlers together using the composite handler, I think this will disable async support if the tracing handler is enabled.
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 not sure CompositeInvocationEventHandler is the right abstraction for InvocationEventProxy any longer given this change.
@@ -87,13 +88,19 @@ public String toString() { | |||
private boolean isEnabled(Object instance, Method method, Object[] args) { | |||
try { | |||
return eventHandler.isEnabled() | |||
&& filter.shouldInstrument(instance, method, args); | |||
&& filter.shouldInstrument(instance, method, args) | |||
&& (eventHandler.asyncSupport() || !isAsync(method)); |
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 we want to remove this line. Tracing doesn't support async operations, but the tracing thread state is passed to executors, which will provide valuable information.
@ellisjoe @cakofony interested in your thoughts on the following questions:
|
I can imagine requests for https://projectreactor.io and http://reactivex.io types. Perhaps we should provide special handling for retrofit in conjure-java-runtime without making changes to tritium.
All handlers are expected be invoked as though they're the only handler registered, regardless of configuration. We shouldn't change this. |
Hey so I'm gonna have to punt on working through the rest of the changes here. At the end of the day it doesn't actually fix my issue which is that I need tracing to support async invocations so I'm gonna close this for now and can come back later or someone else can feel free to pick it up. |
No description provided.