-
Notifications
You must be signed in to change notification settings - Fork 145
trace RxJava #4 #7
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
Conversation
} | ||
|
||
@PostConstruct | ||
public void init() { |
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.
Why this has to be in post construct? Why not directly in the constructor?
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.
fixed
I think we need to figure out these things:
|
@pavolloffay dependency trick is added |
@pavolloffay can I merge it? |
@malafeev sorry for the delay, we usually wait until PR is approved. I will have a closer look tomorrow. |
To keep you busy, you can look at zuul or messaging #5 |
Before merging I would like to know following: @malafeev @cmoulliard @alesj could you please tell me how users are using rxjava in spring boot apps? I found this http://cloud.spring.io/spring-cloud-static/spring-cloud.html#netflix-rxjava-springmvc @malafeev could you please add a test which would ilustrate this scenario? Span created in rxjava should be wired with server span. |
@malafeev could you please add a test which would wire span created in spring-web with span created in rxjava? In other words integration test with one rest controller where you call rxJava and then assert that there are two spans wired together? |
@pavolloffay I'm trying to do it, but getting into spring hell. Cannot return Edit: fixed |
Because Hystrix internally uses RxJava it introduce some problems.If RxJava instrumentation is enabled (via auto configuration) then Hystrix (and feign-hystrix) traces include RxJava spans.All hystrix-feign tests fails because instead of one finished spans there tens of finished spans. |
Sleuth uses |
@malafeev I think we should have a similar behaviour to sleuth. If some instrumentation creates a lot of spans then it might be hard to analyze what is happening. In other words we probably don't wan't to trace internal executions and instead just provide a high level view.
It seems that is was possible to implement it in sleuth. |
@pavolloffay if we want similar sleuth behaviour then we need to create new rxjava instrumentation inside this project only. We cannot change existing opentracing RxJava instrumentation without making it useless for cases when it is not used inside spring cloud application. |
It's your call, I don't know what is required to change current rxJava instrumentation, but maybe some configuration property could solve this. |
sleuth doesn't trace rxjava part from examples: http://cloud.spring.io/spring-cloud-static/spring-cloud.html#netflix-rxjava-springmvc |
it would be good to investigate that. Otherwise we should probably trace only user created rxJava code, and not internals of HystrixFeign (especially if there is a large number of spans). We should clearly say what is being traced and what not. |
@pavolloffay |
The only property which you can read is baggage. But we cannot use baggage for this. |
I didn't find any solution to prevent feign to produce rxjava spans. Even worse looks like feign-hystrix produces spans in the background thread endlessly. |
I created simple example of failure RxJava instrumentation working with spring-cloud, feign, hystrix: https://github.com/malafeev/rxjava-feign-hystrix-failure/blob/master/src/test/java/io/opentracing/contrib/rxjava_feign/MockTracerTest.java |
@pavolloffay looks like @malafeev has found an issue with the current implementation, but has a better approach in mind. Should eliminate the extra/dropped spans. |
👍 Cool! I hope it will make |
I need to |
closing it because I will create new PR based on new API |
No description provided.