-
Notifications
You must be signed in to change notification settings - Fork 859
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
make servlet3 + spring webmvc + jaxrs 2.0 indy compatible #12432
base: main
Are you sure you want to change the base?
Conversation
...telemetry/javaagent/instrumentation/springweb/v3_1/WebApplicationContextInstrumentation.java
Outdated
Show resolved
Hide resolved
…nstrumentation into indy-servlet3
…nstrumentation into indy-servlet3
…nstrumentation into indy-servlet3
} | ||
Object bean = springCtx.getBean("otelAutoDispatcherFilter"); | ||
OpenTelemetryHandlerMappingFilter filter; | ||
if (bean instanceof OpenTelemetryHandlerMappingFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[for reviewer] this is an edge-case where the inlined advice will be able to directly call the OpenTelemetryHandlerMappingFilter
class directly and with indy we get a proxy instance that needs to be unwrapped (thus the delegate
fields needs to be made public for simplicity).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add a TODO here to drop this branch when we remove support for inlined advice then maybe?
|
||
Object bean = springCtx.getBean("otelAutoDispatcherFilter"); | ||
OpenTelemetryHandlerMappingFilter filter; | ||
if (bean instanceof OpenTelemetryHandlerMappingFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[for reviewer] this is an edge-case where the inlined advice will be able to directly call the OpenTelemetryHandlerMappingFilter
class directly and with indy we get a proxy instance that needs to be unwrapped (thus the delegate
fields needs to be made public for simplicity).
…nstrumentation into indy-servlet3
} | ||
Object bean = springCtx.getBean("otelAutoDispatcherFilter"); | ||
OpenTelemetryHandlerMappingFilter filter; | ||
if (bean instanceof OpenTelemetryHandlerMappingFilter) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest to add a TODO here to drop this branch when we remove support for inlined advice then maybe?
...main/java/io/opentelemetry/javaagent/instrumentation/grails/GrailsInstrumentationModule.java
Outdated
Show resolved
Hide resolved
…nstrumentation into indy-servlet3
…nstrumentation into indy-servlet3
...o/opentelemetry/javaagent/instrumentation/springweb/v3_1/SpringWebInstrumentationModule.java
Show resolved
Hide resolved
if (bean instanceof OpenTelemetryHandlerMappingFilter) { | ||
// inline advice: no proxy class is used | ||
filter = (OpenTelemetryHandlerMappingFilter) bean; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo instead of doing the reflection here there should be an API to do this. Something that would unwrap the object when it is proxy and return the input when it is not. So you could write OpenTelemetryHandlerMappingFilter filter = = (OpenTelemetryHandlerMappingFilter) unwrap(springCtx.getBean("otelAutoDispatcherFilter"));
I guess using the reflection as currently is fine for start. Instead of making the delegate field public I'd probably try to make the proxy instance implement an interface that allow unwrapping. If needed that interface could be hidden from reflection
Line 59 in 647fc45
public static Class<?>[] filterInterfaces(Class<?>[] interfaces, Class<?> containingClass) { |
part of #11457