search

opentracing-contrib / java-spring-web

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

Run tests on Netty for WebFlux #112

Open malafeev opened 5 years ago

malafeev commented 5 years ago

WebFlux doesn't require jetty or tomcat. It uses netty. Is it possible to get rid of jetty from WebFlux integration tests?

geoand commented 5 years ago

Is this causing problems for you for some reason?

malafeev commented 5 years ago

yes, it looks like TracingSubscriber.onComplete() is never called, therefore span is not finished: https://github.com/opentracing-contrib/java-spring-web/blob/master/opentracing-spring-web/src/main/java/io/opentracing/contrib/spring/web/webfilter/TracingSubscriber.java#L77

geoand commented 5 years ago

So I might be missing something here... Is this tied to another issue perhaps?

malafeev commented 5 years ago

If I use in my app tomcat then I see that TracingSubscriber.onComplete() is called. But if I don't use tomcat and rely on netty which comes with spring-webflux I don't see calling TracingSubscriber.onComplete(). That's why I'm curious why integration test for WebFlux is based on jetty. I use the latest version of spring.

geoand commented 5 years ago

@csabakos can you shed some light on this perhaps since you were the one that added those tests?

Thanks

csabakos commented 5 years ago

@geoand WebFlux should work equally over any runtime (Tomcat, Jetty, Netty, etc.), so there was no reason to switch away from the runtime that was already used in other tests.

If the observed behavior is different on Netty, then I think it would make sense to add Netty-based integration tests to make sure that all cases are covered.

geoand commented 5 years ago

So you just added like that for convenience, thanks that's good to know.

@malafeev Would you like to take a stab at it? If you are unavailable, I can take a look but it will probably be a while...

malafeev commented 5 years ago

@geoand it would be great if you take a look even in a while. Currently I don't have enough time.

geoand commented 5 years ago

Same here :).

I'll take a look as soon as I can

geoand commented 5 years ago

I want to upgrade to Opentracing 0.33 first. Then I'll look into this