search

opentracing-contrib / java-spring-web

OpenTracing Spring Web instrumentation
Apache License 2.0
107 stars 59 forks source link

Fix runtime exception when running with webflux #72

Closed pavolloffay closed 6 years ago

pavolloffay commented 6 years ago

Resolves #59

Signed-off-by: Pavol Loffay ploffay@redhat.com

pavolloffay commented 6 years ago

PS I have tested it locally with hello world style webflux app. It didn't crash

pavolloffay commented 6 years ago

@darrenhaken could you please verify this?

darrenhaken commented 6 years ago

@pavolloffay sure thing, what version should I pull down to test against?

pavolloffay commented 6 years ago

@darrenhaken you have to compile this repo locally and use snapshot

darrenhaken commented 6 years ago

@pavolloffay OK great I'll try to take a look tomorrow

darrenhaken commented 6 years ago

@pavolloffay the runtime exception doesn't occur with a WebFlux API.

However, we do not see any tracing in Jaeger unless we explicately declare a span within one of the routes. Is that expected?

Also, we did experience an issue which I thought you should be aware of; it's an edge case though based on our environment. In order to support a legacy component in our platform, we had to add into the servlet API dependency to the WebFlux based service.

I thought it was worth mentioning because it's hard to say how much of an edge case this is.

This did start to cause issues again. Not sure if there is a more elegant code change which would prevent this from happening. Exception as follows:

Caused by: java.lang.NoClassDefFoundError: org/springframework/web/servlet/config/annotation/WebMvcConfigurerAdapter
    at java.lang.Class.getDeclaredMethods0(Native Method) ~[na:1.8.0_144]
    at java.lang.Class.privateGetDeclaredMethods(Class.java:2701) ~[na:1.8.0_144]
    at java.lang.Class.getDeclaredMethods(Class.java:1975) ~[na:1.8.0_144]
    at org.springframework.util.ReflectionUtils.getDeclaredMethods(ReflectionUtils.java:641) ~[spring-core-5.0.6.RELEASE.jar:5.0.6.RELEASE]
    ... 25 common frames omitted
Caused by: java.lang.ClassNotFoundException: org.springframework.web.servlet.config.annotation.WebMvcConfigurerAdapter
    at java.net.URLClassLoader.findClass(URLClassLoader.java:381) ~[na:1.8.0_144]
    at java.lang.ClassLoader.loadClass(ClassLoader.java:424) ~[na:1.8.0_144]
    at sun.misc.Launcher$AppClassLoader.loadClass(Launcher.java:335) ~[na:1.8.0_144]
    at java.lang.ClassLoader.loadClass(ClassLoader.java:357) ~[na:1.8.0_144]
    ... 29 common frames omitted
pavolloffay commented 6 years ago

@darrenhaken thanks!

If you have it locally cloned, would you like to try using @ConditionalOnClass(WebMvcConfigurerAdapter.class) instead of Servlet.class?

darrenhaken commented 6 years ago

@pavolloffay Will check that today

timpharo commented 6 years ago

@pavolloffay following on from discussions with @darrenhaken i have retested this locally with @ConditionalOnClass(WebMvcConfigurerAdapter.class) and this fixes our issues regarding startup once more, cheers.

That said i am still not seeing any tracing hitting Jaeger unless i explicitly declare a span within one of the routes.

pavolloffay commented 6 years ago

@timpharo thanks for the test.

That said i am still not seeing any tracing hitting Jaeger unless i explicitly declare a span within one of the routes.

That is expected, webflux will need a different instrumentation.

darrenhaken commented 6 years ago

What's the different instrumentation needed for Webflux?

geoand commented 6 years ago

Webflux is an entirely different web stack (a reactive one based based on project reactor), it has nothing to do with Web MVC (which is servlet based), therefore it needs an entirely new instrumentation.

On Wed, May 30, 2018, 22:38 Darren Haken notifications@github.com wrote:

What's the different instrumentation needed for Webflux?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/opentracing-contrib/java-spring-web/pull/72#issuecomment-393290718, or mute the thread https://github.com/notifications/unsubscribe-auth/AELBv-32QPIBiUdxnCTejrC5jk1uifecks5t3vVJgaJpZM4URcEr .

darrenhaken commented 6 years ago

Would you imagine this library to add instrumentation or is it lower down the stack like the reactor project?

geoand commented 6 years ago

I would imagine that instrumentation will be added at some point in time and it would probably be based on some of the context propagation features in Reactor (but that is just a guess 😀)

On Wed, May 30, 2018, 23:19 Darren Haken notifications@github.com wrote:

Would you imagine this library to add instrumentation or is it lower down the stack like the reactor project?

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub https://github.com/opentracing-contrib/java-spring-web/pull/72#issuecomment-393304413, or mute the thread https://github.com/notifications/unsubscribe-auth/AELBv1CCEUOSTvx7QLfjPwI81oJheZx2ks5t3v7KgaJpZM4URcEr .

pavolloffay commented 6 years ago

Here is an issue to track it https://github.com/opentracing-contrib/java-spring-cloud/issues/150.

https://docs.spring.io/spring/docs/5.0.0.BUILD-SNAPSHOT/spring-framework-reference/html/web-reactive.html#web-reactive-server it mentions that webflux can run on servlet containers. If it's your case you can servlet filter integration https://github.com/opentracing-contrib/java-web-servlet-filter, or use the filter from this repository.