instrument spring scheduling @Async #1873
Conversation
richardstartin
force-pushed
the
rgs/spring-scheduling-async
branch
from
eae92f3 to
02cc033
Compare
richardstartin
force-pushed
the
rgs/spring-scheduling-async
branch
7 times, most recently
from
4893d7f to
a9f2d81
Compare
Well, instrumenting the interceptor was your idea... |
....1/src/main/java/datadog/trace/instrumentation/springscheduling/SpannedMethodInvocation.java
Show resolved
Hide resolved
richardstartin
force-pushed
the
rgs/spring-scheduling-async
branch
from
a9f2d81 to
a7d7f25
Compare
richardstartin
force-pushed
the
rgs/spring-scheduling-async
branch
from
a7d7f25 to
bc2fb5e
Compare
dd-java-agent/instrumentation/spring-scheduling-3.1/spring-scheduling-3.1.gradle
Outdated
Show resolved
Hide resolved
|
|
||
| @Override | ||
| public ElementMatcher<? super TypeDescription> typeMatcher() { | ||
| return named("org.springframework.aop.interceptor.AsyncExecutionInterceptor"); |
There was a problem hiding this comment.
I'm a little concerned about adding instrumentation for a class that seems relatively unrelated to scheduling in the module for spring-scheduling. Is @Async used for anything besides spring scheduling?
There was a problem hiding this comment.
This intercepts the annotation (notice the package) org.springframework.scheduling.annotation.Async.
There was a problem hiding this comment.
Yes, this seems pretty clearly related to Async to me. Although, I'll admit their package naming makes me a little concern that maybe AsyncExecutionInterceptor does more.
But presuming AsyncExecutionInterceptor is specific to @Async annotations, I think this is probably the right place.
It solves my general aim which to let the frameworks find the target methods for us -- rather than us seeking them out.
| scope.setAsyncPropagation(true); | ||
| return delegate.proceed(); | ||
| } finally { | ||
| // question: Why can't this just be AutoCloseable? Dogma? |
There was a problem hiding this comment.
There's a fair bit of history here, but I can't find any original sources. I think these summarize the problem well:
opentracing/opentracing-java#361 (comment)
https://github.com/Nike-Inc/wingtips#warning-about-error-handling-when-using-try-with-resources-to-autoclose-spans
There was a problem hiding this comment.
That reads like an issue with programming discipline, and sacrifices convenience in the common case for the inability to be incorrect in the rare case. Just my opinion.
| // question: is this necessary? What does it do? | ||
| // if the delegate does async work is everything OK because of this? | ||
| // if the delegate does async work, should I need to worry about it here? | ||
| scope.setAsyncPropagation(true); |
There was a problem hiding this comment.
This will hopefully be unnecessary with the planned changes to PendingTrace.
| @Advice.OnMethodEnter(suppress = Throwable.class) | ||
| public static void scheduleAsync( | ||
| @Advice.Argument(value = 0, readOnly = false) MethodInvocation invocation) { | ||
| invocation = new SpannedMethodInvocation(activeSpan(), invocation); |
There was a problem hiding this comment.
One other thing to mention is that currently this will result in the span likely being reported as a separate trace since you're not using continuations to force the trace to stick around.
There was a problem hiding this comment.
I think that is fine modeling wise because it's timing does not matter for the critical path
There was a problem hiding this comment.
I'm confused by saying it is a separate trace, isn't it just spans sent separately? That's what I think it should be.
I think today. These spans might go to PendingTrace that is thought to be done, but I think that's a problem with PendingTrace.
There was a problem hiding this comment.
@dougqh to clarify, I mean it would likely be reported independently from the rest of the trace.
There was a problem hiding this comment.
This feels like the wrong API to me. I should need to call AgentSpan.finish() to finish the span. DDSpanContext has a PendingTrace - so why shouldn't AgentSpan.finish() be enough to hold the PendingTrace back?
There was a problem hiding this comment.
Yes, right now, an unfinished span holds back sending the trace leg.
But that's leading to problems where an unfinished span, holds back the whole trace.
I think we want to get away from that.
My thought is that a span is published in the first payload when it finishes before the root span.
(Except for long root spans for batch jobs where we need to send sooner).
Otherwise, the span is deemed to have finished late. If it makes it with a grace period, it can be included. Otherwise, it goes separately. But basically, it is only guarantee if it finishes before the root.
richardstartin
force-pushed
the
rgs/spring-scheduling-async
branch
from
cb4317d to
cd7bd80
Compare
|
Fixes #1510 |
This works by instrumenting the @ Async interceptor and wrapping the
MethodInvocationto capture the active context, if it exists.