Add auto configuration for spring scheduling instrumentation using aop #12438
Conversation
github-actions
bot
added
the
test native
| unproxiedTesterSimpleClassName = unproxiedTester.getClass().getSimpleName(); | ||
| unproxiedTesterClassName = unproxiedTester.getClass().getName(); | ||
|
|
||
| AspectJProxyFactory factory = new AspectJProxyFactory(); |
There was a problem hiding this comment.
Are you trying to enable the AspectJ support with Spring? In this case, you could use @EnableAspectJAutoProxy (https://docs.spring.io/spring-framework/reference/core/aop/ataspectj/aspectj-support.html)
There was a problem hiding this comment.
Yes, these codes are purposed for unit tests to test the functions of SpringSchedulingInstrumentationAspect. If we use @EnableAspectJAutoProxy annotation, we must start the AnnotationConfigApplicationContext which is too heavy for tests since we only need to test the function of Aop I think. Using AspectJProxyFactory to create proxy instance and let it parse and apply the SpringSchedulingInstrumentationAspect will also meet our demands for tests.
kyy1996
force-pushed
the
feature/scheduling_aop
branch
from
dc6ca08 to
77d7cd0
Compare
| @ConditionalOnEnabledInstrumentation(module = "spring-scheduling") | ||
| @ConditionalOnClass({Scheduled.class, Aspect.class}) | ||
| @Configuration | ||
| public class SpringSchedulingInstrumentationAutoConfiguration { |
There was a problem hiding this comment.
| public class SpringSchedulingInstrumentationAutoConfiguration { | |
| class SpringSchedulingInstrumentationAutoConfiguration { |
| @NonNull Context parentContext, | ||
| @NonNull REQUEST classAndMethod) { | ||
| attributes | ||
| .put(ThreadIncubatingAttributes.THREAD_ID, Thread.currentThread().getId()) |
There was a problem hiding this comment.
Is the same thing done with the Java agent instrumentation?
There was a problem hiding this comment.
The final result is quite the same as java agent, but it's implemented differently. Java agent uses AddThreadDetailsSpanProcessor to add thread.id to the attributes of a span, while in spring boot auto configuration this information seems missing in span. Hence, to match the final span as java agent I write this.
For further extension, this class may be extracted as a common span processor and configured automatically just like the one in java agent?
There was a problem hiding this comment.
Thank you for your explanations. The current implementation is good to me.
There was a problem hiding this comment.
For further extension, this class may be extracted as a common span processor and configured automatically just like the one in java agent?
I think this is a good idea 👍
I'd recommend removing it from this PR (since in the current state it's currently inconsistent with the other spring starter instrumentation), then we can merge, and then you can send a follow-up PR for this
There was a problem hiding this comment.
That sounds cool to me😊. I've made the change.
kyy1996
force-pushed
the
feature/scheduling_aop
branch
from
77d7cd0 to
27aba3b
Compare
kyy1996
force-pushed
the
feature/scheduling_aop
branch
from
27aba3b to
8da1085
Compare
Since library instrumentation for spring scheduling is missing. I would like to provide a simple library instrumentation using AOP for spring scheduling.
Using this approach, we could intercept calls of any
@Scheduledannotated method and start spans for them. Though this can cover most of the use cases, there's a limitation that tasks registered throughScheduledTaskRegistrar#addCronTaskprogrammatically will not be recorded since they don't have@Scheduledannotation.Also, I've added an auto configuration for it, so that this can be an out-of-box instrumentation.
Properties for this instrumentation such as
otel.instrumentation.spring-scheduling.experimental-span-attributesandotel.instrumentation.spring-scheduling.enabledare aligned with spring scheduling javaagent instrumentation.