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

Refactors such that Kafka bundles spans and shares more code #143

Merged
merged 1 commit into from
Mar 5, 2016

Conversation

codefromthecrypt
Copy link
Member

This rewrites KafkaSpanCollector to bundle multiple spans into the same
Kafka message. Basically, as many messages that come in one second will
send in the same message. This change resulted in several times higher
throughput in Yelp's kafka+cassandra Zipkin architecture
.

Flushing can be controlled via KafkaSpanCollector.Config.flushInterval

Incidentally, this shares a lot of code with HttpSpanCollector, which
should reduce the amount of bugs and maintenance around queue-based
collection.

@kristofa
Copy link
Member

kristofa commented Mar 2, 2016

Good that we try to share more code between collectors. The queueing and submitting logic was implemented multiple times and can easy contain subtle bugs. Makes sense to abstract and re-use.

@codefromthecrypt
Copy link
Member Author

@kristofa PTAL

@codefromthecrypt
Copy link
Member Author

PS the code would be less complex if we mandated those using Kafka to
update to zipkin 1.35+ collectors prior to upgrading brave. If that
policy sounds OK, I can rip out some dodgy code.

@kristofa
Copy link
Member

kristofa commented Mar 3, 2016

I think it is fine to ask people who use Kafka to upgrade to 1.35+ collectors. We can add it in the release notes. In that case we can remove the bundlingEnabled config entry and code that deals with it. That's what you mean with dodgy code I guess?

@kristofa
Copy link
Member

kristofa commented Mar 3, 2016

I also like the approach with flushInterval=0 for testing. Makes tests a lot more reliable if we don't have to deal with background thread. 👍

@codefromthecrypt codefromthecrypt changed the title Refactors such that Kafka and Http collectors share more code Refactors such that Kafka bundles spans and shares more code Mar 4, 2016
@codefromthecrypt
Copy link
Member Author

ready to go

This rewrites KafkaSpanCollector to bundle multiple spans into the same
Kafka message. Basically, as many messages that come in one second will
send in the same message. This change resulted in several times higher
throughput in Yelp's kafka+cassandra Zipkin architecture.

Flushing can be controlled via KafkaSpanCollector.Config.flushInterval

Incidentally, this shares a lot of code with HttpSpanCollector, which
should reduce the amount of bugs and maintenance around queue-based
collection.
@kristofa
Copy link
Member

kristofa commented Mar 5, 2016

Not sure why travis build is unhappy. Tests run fine in my local environment. Will merge.

kristofa added a commit that referenced this pull request Mar 5, 2016
Refactors such that Kafka bundles spans and shares more code
@kristofa kristofa merged commit af557af into master Mar 5, 2016
@kristofa kristofa deleted the kafka-deux branch March 5, 2016 10:50
@kristofa
Copy link
Member

kristofa commented Mar 5, 2016

@adriancole After merge master build is failing. There was a warning about missing version for maven-shade-plugin which I fixed but I can't see build log for latest 2 builds... (latest one: https://travis-ci.org/openzipkin/brave/builds/113873969) any idea what is wrong?

@kristofa
Copy link
Member

kristofa commented Mar 5, 2016

I can't reproduce build failure in my local environment.

@kristofa kristofa restored the kafka-deux branch March 5, 2016 11:36
@kristofa kristofa deleted the kafka-deux branch March 5, 2016 11:55
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.

2 participants