Skip to content
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

Remove the Zipkin Stream server #727

Closed
marcingrzejszczak opened this issue Oct 9, 2017 · 9 comments
Assignees
Milestone

Comments

@marcingrzejszczak
Copy link
Contributor

@marcingrzejszczak marcingrzejszczak commented Oct 9, 2017

Zipkin server is basically incompatible with Boot 2.0, we have to hack it around to make it work. Also, Zipkin's getting more support in terms of messaging reporters. What we could do is to tell the users to use the 1.5.x version of the server and cooperate with the Zipkin project to add support for Kafka and a bridge to the Sleuth version of a span.

WDYT @spencergibb @ryanjbaxter @adriancole @dsyer @devinsba ?

@adriancole

This comment has been minimized.

Copy link
Contributor

@adriancole adriancole commented Oct 9, 2017

nearly a dupe of #555 which formerly discussed decoupling this from the server build. IMHO now is the best time to cut out the stream zipkin server component, which could be less revlocked somewhere else (like another repo) or obviated by the recent support for rabbit on zipkin side.

@adriancole

This comment has been minimized.

Copy link
Contributor

@adriancole adriancole commented Oct 9, 2017

fyi, there will be a zipkin boot 2 server, but we are reserving v2 to also finish some transport-level concerns.. basically it will be a fresh server with no old deps, but sadly won't happen this month.

@dsyer

This comment has been minimized.

Copy link
Contributor

@dsyer dsyer commented Oct 9, 2017

What is it that is "incompatible"? Isn't it just a Spring Cloud Stream app? I'm reluctant to drop it (or move it to another repo) if there is no support for equivalent features in zipkin. Is the rabbit piece of zipkin GA? Also, existing users that have live apps with Spring Cloud Sleuth sending messages via Spring Cloud Stream would need somewhere to send those messages, so we need at least an anti-corruption layer on top of zipkin (assuming the contract is different).

@marcingrzejszczak

This comment has been minimized.

Copy link
Contributor Author

@marcingrzejszczak marcingrzejszczak commented Oct 9, 2017

Zipkin doesn't work with Boot 2. So the idea would be to tell the users to use the 1.5.x version of sleuth stream. The acl would have to be somewhere for sure to translate from our span to the zipkin span.

@dsyer

This comment has been minimized.

Copy link
Contributor

@dsyer dsyer commented Oct 9, 2017

I see. The 1.x version will be fine for existing users as long as we are careful then (no need even for a separate acl probably, as long as zipkin doesn't change). I think we should just ditch it in Sleuth 2.x until we have time to replace it, or recommend something from zipkin. It will need some careful documentation in release notes. And some of your amazing automated tests to verify that the 2.0 client works with the 1.5 server.

@adriancole

This comment has been minimized.

Copy link
Contributor

@adriancole adriancole commented Oct 9, 2017

@adriancole

This comment has been minimized.

Copy link
Contributor

@adriancole adriancole commented Oct 9, 2017

I will exercise some freedom and responsibility.. adding something to autowire to the correct zipkin config.

@adriancole

This comment has been minimized.

Copy link
Contributor

@adriancole adriancole commented Oct 9, 2017

k. bringing the pow-wow together based on a chat w/ marcin

First: transition-wise, we assume the hybrid zipkin+stream servers are built independently than traced applications. This assumption makes everything else easier as we can discuss client and server decoupled from eachother.

End game is that sleuth 3.0 gets out of the "reporting business", this includes but is not limited to stream. For example, a zipkin stream reporter makes sense, but at the point it uses zipkin types, it doesn't need to be pinned to sleuth anyway (ex others can use it).

The transition plan is apps move off sc-sleuth-stream to one of:

  • built-in autoconfig http, kafka or rabbitmq
  • custom zipkin sender (ex one of the myriad of cloud-specific ones)

Having this plan will allow folks to scream, scream implying all of the above won't work for them, and so we actually need to make a zipkin+stream reporter sooner vs later. Meanwhile in 2.0 it is deprecated, and not removed, so no-one is broke anyway.

They aren't broke because they can run their existing 1.5 hybrid stream+zipkin servers while we move along. Either they replace with a stock zipkin server (in the case of http, kafka or rabbitmq or one of the cloudy ones), or they also scream saying whatever transport they need isn't available.

So the only things missing are auto-config for kafka and rabbit which isn't hard, and I offer to take on. lemme know if things are unclear or don't work!

adriancole added a commit that referenced this issue Oct 20, 2017
Fixes #727
@adriancole adriancole self-assigned this Oct 20, 2017
adriancole added a commit that referenced this issue Oct 20, 2017
This deprecates span transports in favor of `spring-cloud-sleuth-zipkin2`
which:
* Supports RabbitMQ and Kafka automatically when adding spring-rabbit or spring-kafka deps
* Supports any io.zipkin.reporter2:zipkin-reporter sender, including Amazon X-Ray
* Defaults to zipkin v2 format, but can be configured to use zipkin v1 format as needed

This lowers the amount of maintenance on the project, as discussed in various issues

See #727
See #711
adriancole added a commit that referenced this issue Oct 22, 2017
This deprecates span transports in favor of `spring-cloud-sleuth-zipkin2`
which:
* Supports RabbitMQ and Kafka automatically when adding spring-rabbit or spring-kafka deps
* Supports any io.zipkin.reporter2:zipkin-reporter sender, including Amazon X-Ray
* Defaults to zipkin v2 format, but can be configured to use zipkin v1 format as needed

This lowers the amount of maintenance on the project, as discussed in various issues

See #727
See #711
adriancole added a commit that referenced this issue Oct 22, 2017
Fixes #727
adriancole added a commit that referenced this issue Oct 22, 2017
@marcingrzejszczak

This comment has been minimized.

Copy link
Contributor Author

@marcingrzejszczak marcingrzejszczak commented Oct 23, 2017

Done

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.