Wrap Runnable in AsyncContext.start #388
Conversation
| @Nullable | ||
| private Runnable delegate; | ||
| @Nullable | ||
| private Transaction transaction; |
There was a problem hiding this comment.
can this be changed to AbstractSpan?
There was a problem hiding this comment.
Good point- it can and should for other usages
Codecov Report
@@ Coverage Diff @@
## master #388 +/- ##
============================================
- Coverage 71.97% 71.46% -0.52%
+ Complexity 1203 1199 -4
============================================
Files 132 133 +1
Lines 4564 4612 +48
Branches 468 471 +3
============================================
+ Hits 3285 3296 +11
- Misses 1066 1103 +37
Partials 213 213
Continue to review full report at Codecov.
|
|
|
||
| @Override | ||
| public void run() { | ||
| Scope scope = null; |
There was a problem hiding this comment.
I think this can be simplified to
if (transaction != null) {
transaction.activate();
try {
delegate.run();
} catch (Exception e) {
transaction.captureException(e);
throw e;
} finally {
transaction.deactivate();
}
}
There was a problem hiding this comment.
Well, I don't want to give up the try/catch around activate/deactivate - it's just like your other PR to catch errors during advices - our code should never prevent user code from running. I can give up the usage of scope though.
Your suggestion to capture the Exception and report as error is tempting, but is not safe here. Notice that this code is not taking responsibility on ending and reporting the Transaction, it's just responsible for scoping in that thread. For example, the usage of AsyncContext.onComplete would end the transaction in this PR (and AsyncContext.onError will report errors). So I don't know what the status of the transaction is at this stage.
There was a problem hiding this comment.
Good point on the problems with captureException. However, AbstractSpan#activate already catches exceptions.
There was a problem hiding this comment.
Is it valid to deactivate a finished span? Ideally, #end should be the last call on a span because after that the span is recycled and could be reused any time. Deactivating after that is borderline but probably unproblematic if the state isn't changed after #end.
There was a problem hiding this comment.
Although it seems weird, I think it is completely valid and safe. deactivate does two things:
- removes from the current threads stack
- invokes registered
SpanListener.onDeactivate(which currently only implemented for MDC)
Both of these are only doing un-registration on a thread context level, so even if the span has been recycled and reused and even if it is active on a different thread, all this call is doing is making it non-active in regard to this specific thread (there is nothing preventing a span object from being active on two threads concurrently).
We just need to make sure future SpanListener implementations are maintaining this behavior. Maybe it's best if we add to the SpanListener interface documentation that it should only deal with registering/unregistering span on thread context, but should never assume anything about span state.
eyalkoren
force-pushed
the
supporting-AsyncContext.start
branch
from
e89c0c4 to
159f368
Compare
| if (tracer != null) { | ||
| final Transaction transaction = tracer.currentTransaction(); | ||
| if (transaction != null && runnable != null && asyncHelper != null) { | ||
| runnable = asyncHelper.getForClassLoaderOfClass(AsyncContext.class).wrapRunnable(runnable, transaction); |
There was a problem hiding this comment.
wouldn't it be easier to just call the wrapRunnable method on the tracer instance? Then you could also remove the wrapRunnable from the helper.
There was a problem hiding this comment.
When implementing I imagined it will do some additional stuff, but probably no need for that as it is now
| @Nullable | ||
| private AbstractSpan<?> span; | ||
| @Nullable | ||
| private ElasticApmTracer tracer; |
There was a problem hiding this comment.
make final and remove @Nullable
There was a problem hiding this comment.
Hmm, didn't realize we don't recycle this reference...
Will do
| if (span != null) { | ||
| span.deactivate(); | ||
| } | ||
| //noinspection ConstantConditions |
There was a problem hiding this comment.
can be removed when making tracer nonnull
| } | ||
|
|
||
| try { | ||
| //noinspection ConstantConditions |
There was a problem hiding this comment.
it's safer to add a null check than to suppress these inspections
There was a problem hiding this comment.
I know it's not null. I don't like to add null checks just to satisfy inspections... There are already too many non-required null checks.
| this.tracer = tracer; | ||
| } | ||
|
|
||
| public InScopeRunnableWrapper wrap(Runnable delegate) { |
There was a problem hiding this comment.
Maybe add AbstractSpan as a second parameter and remove the withScope method? Calling one without the other would lead to an invalid state anyway.
eyalkoren
force-pushed
the
supporting-AsyncContext.start
branch
from
4457d4e to
ae72743
Compare
No description provided.