Wrap Runnable in AsyncContext.start

[eyalkoren]

No description provided.

[felixbarny]

can this be changed to AbstractSpan?

[eyalkoren]

Good point- it can and should for other usages

[codecov-io]

Codecov Report

Merging #388 into master will decrease coverage by 0.51%.
The diff coverage is 36.36%.

@@             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

Impacted Files	Coverage Δ	Complexity Δ
...t/servlet/helper/AsyncContextAdviceHelperImpl.java	0% <0%> (ø)	0 <0> (?)
...elastic/apm/agent/impl/InScopeRunnableWrapper.java	0% <0%> (ø)	0 <0> (?)
...va/co/elastic/apm/agent/impl/ElasticApmTracer.java	75.6% <33.33%> (-1.61%)	43 <0> (ø)
...lastic/apm/agent/servlet/AsyncInstrumentation.java	68.57% <68.75%> (-6.43%)	3 <2> (-4)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 915b929...ae72743. Read the comment docs.

[felixbarny]

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();
    }
}

[eyalkoren]

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.

[felixbarny]

Good point on the problems with captureException. However, AbstractSpan#activate already catches exceptions.

[felixbarny]

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.

[eyalkoren]

Although it seems weird, I think it is completely valid and safe. deactivate does two things:

1. removes from the current threads stack
2. 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.

[felixbarny]

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.

[eyalkoren]

When implementing I imagined it will do some additional stuff, but probably no need for that as it is now

[felixbarny]

make final and remove @Nullable

[eyalkoren]

Hmm, didn't realize we don't recycle this reference...
Will do

[felixbarny]

can be removed when making tracer nonnull

[felixbarny]

it's safer to add a null check than to suppress these inspections

[eyalkoren]

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.

[felixbarny]

Maybe add AbstractSpan as a second parameter and remove the withScope method? Calling one without the other would lead to an invalid state anyway.