[improvement] Add support for tracing-aware Guava FutureCallbacks.#77
[improvement] Add support for tracing-aware Guava FutureCallbacks.#77dsd987 wants to merge 5 commits intopalantir:developfrom
Conversation
| /** | ||
| * Like {@link #wrapWithNewTrace(String, Callable)}, but for Guava's FutureCallback. | ||
| */ | ||
| public static <V> FutureCallback<V> wrapWithNewTrace(String operation, FutureCallback<V> delegate) { |
There was a problem hiding this comment.
I don't think we ever want to create a new trace around just the callback
carterkozak
left a comment
carterkozak
left a comment
There was a problem hiding this comment.
@ellisjoe is putting some thought into support for tracing asynchronous operations as well.
| ListenableFuture<Void> success = listeningExecutorService.submit(() -> null); | ||
| ListenableFuture<Void> failure = listeningExecutorService.submit(() -> { | ||
| throw new IllegalStateException(); | ||
| }); |
There was a problem hiding this comment.
should use separate test cases for the successful/failed futures. Otherwise when tests fail, it. takes additional debugging to figure out what the problem is.
| ListenableFuture<Void> success = listeningExecutorService.submit(() -> null); | ||
| ListenableFuture<Void> failure = listeningExecutorService.submit(() -> { | ||
| throw new IllegalStateException(); | ||
| }); |
There was a problem hiding this comment.
Should use separate test cases for these
|
|
||
| /** Like {@link #wrap(Callable)}, but for Guava's FutureCallback. */ | ||
| public static <V> FutureCallback<V> wrap(FutureCallback<V> delegate) { | ||
| return new TracingAwareFutureCallback<>(delegate); |
There was a problem hiding this comment.
Should this take an operation name? Once #71 merges DeferredTracer can take an operation name. I'd prefer we we required an operation name for new methods, and did not provide a method using a default.
There was a problem hiding this comment.
Sure. Can wait until #71 merges and then rebase to use the DeferredTracer with the operation name.
| Tracer.startSpan("before-construction"); | ||
| FutureCallback<Void> successCallback = Tracers.wrap(futureCallback); | ||
| Tracer.startSpan("after-construction"); | ||
| Futures.addCallback(success, successCallback, MoreExecutors.directExecutor()); |
There was a problem hiding this comment.
We should have a test that runs on an executor using a different thread, since tracing-java heavily uses thread state. The direct executor just invokes run() on the current thread.
2 participants
Before this PR
Functionality defined as part of a Guava FutureCallback may not be correctly traced as the thread it runs on may not be the thread adding the callback.
After this PR
FutureCallbacks can be wrapped such that their traces are parented to the thread adding the callback or wrapped in a new trace if desired.