Skip to content

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

Closed
wants to merge 8 commits into from
Closed

trace RxJava #4 #7

wants to merge 8 commits into from

Conversation

malafeev
Copy link

No description provided.

}

@PostConstruct
public void init() {
Copy link
Contributor

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@pavolloffay
Copy link
Contributor

I think we need to figure out these things:

  • rxjava dependency should be pulled in from a spring dependency not an instrumentation lib.
  • if user app does not depend or rxjava instrumentation artifact does not do anything

@malafeev
Copy link
Author

@pavolloffay dependency trick is added

@malafeev
Copy link
Author

@pavolloffay can I merge it?

@pavolloffay
Copy link
Contributor

@malafeev sorry for the delay, we usually wait until PR is approved. I will have a closer look tomorrow.

@pavolloffay
Copy link
Contributor

To keep you busy, you can look at zuul or messaging #5

@pavolloffay
Copy link
Contributor

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.

@pavolloffay pavolloffay mentioned this pull request Jul 20, 2017
@pavolloffay
Copy link
Contributor

@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?

@malafeev
Copy link
Author

malafeev commented Aug 1, 2017

@pavolloffay I'm trying to do it, but getting into spring hell. Cannot return Single<String> in spring controller: something always missing :(

Edit: fixed

@pavolloffay pavolloffay mentioned this pull request Aug 1, 2017
@malafeev
Copy link
Author

malafeev commented Aug 1, 2017

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.

@malafeev
Copy link
Author

malafeev commented Aug 2, 2017

Sleuth uses DEFAULT_IGNORED_THREADS = Arrays.asList("HystrixMetricPoller", "^RxComputation.*$");
Ignored threads are not supported by RxJava instrumentation. And I don't see a way to implement that.
The reason why sleuth rxjava/feign-hystrix works is that sleuth create rxjava spans only in case of multithreaded rxjava execution. Single threaded rxjava is not supported. Also it's strange idea to ignore ^RxComputation.*$ threads by default.

@pavolloffay
Copy link
Contributor

@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.

And I don't see a way to implement that.

It seems that is was possible to implement it in sleuth.

@malafeev
Copy link
Author

malafeev commented Aug 2, 2017

@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.

@pavolloffay
Copy link
Contributor

It's your call, I don't know what is required to change current rxJava instrumentation, but maybe some configuration property could solve this.

@malafeev
Copy link
Author

malafeev commented Aug 2, 2017

sleuth doesn't trace rxjava part from examples: http://cloud.spring.io/spring-cloud-static/spring-cloud.html#netflix-rxjava-springmvc
Only spring web part is traced.
I don't understand how sleuth supports RxJava. Looks like it is useless. Probably there are special cases where sleuth trace rxjava.

@pavolloffay
Copy link
Contributor

I don't understand how sleuth supports RxJava. Looks like it is useless. Probably there are special cases where sleuth trace rxjava.

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.

@malafeev
Copy link
Author

malafeev commented Aug 3, 2017

@pavolloffay BaseSpan doesn't have getters. How I can get information that current active span is from HystrixFeign to avoid child span creation?

@pavolloffay
Copy link
Contributor

@pavolloffay BaseSpan doesn't have getters. How I can get information that current active span is from HystrixFeign to avoid child span creation?

The only property which you can read is baggage. But we cannot use baggage for this.

@pavolloffay
Copy link
Contributor

@malafeev @tedsuo what is the status of this?

@malafeev
Copy link
Author

malafeev commented Aug 8, 2017

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.

@malafeev
Copy link
Author

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

@tedsuo
Copy link

tedsuo commented Aug 22, 2017

@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.

@pavolloffay
Copy link
Contributor

👍 Cool! I hope it will make rxJava instrumentation work here.

@malafeev
Copy link
Author

malafeev commented Aug 28, 2017

I need to activate and then deactivate span, but not finish it on deactivate.
I hope to get opentracing/specification#80 fixed soon.

@malafeev
Copy link
Author

closing it because I will create new PR based on new API

@malafeev malafeev closed this Sep 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants