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 #146

Merged
merged 1 commit into from
Mar 5, 2016

Conversation

kristofa
Copy link
Member

@kristofa kristofa commented Mar 5, 2016

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 Author

kristofa commented Mar 5, 2016

@adriancole This pr has same content as #143 which was reverted. I think we should try to find out why the Travis build fails prior merging. wdyt?

@kristofa
Copy link
Member Author

kristofa commented Mar 5, 2016

This is the error I see in the logs:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-compiler-plugin:3.1:compile (default-compile) on project brave-spancollector-kafka: Compilation failure: Compilation failure:
[ERROR] /home/travis/build/openzipkin/brave/brave-spancollector-kafka/src/main/java/com/github/kristofa/brave/kafka/KafkaSpanCollector.java:[13,23] package zipkin.internal does not exist
[ERROR] /home/travis/build/openzipkin/brave/brave-spancollector-kafka/src/main/java/com/github/kristofa/brave/kafka/KafkaSpanCollector.java:[61,17] cannot find symbol
[ERROR] symbol:   class ThriftCodec
[ERROR] location: class com.github.kristofa.brave.kafka.KafkaSpanCollector

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.
@codefromthecrypt
Copy link
Member

sorry.. some leftovers. should be good now!

codefromthecrypt pushed a commit that referenced this pull request Mar 5, 2016
Refactors such that Kafka bundles spans and shares more code
@codefromthecrypt codefromthecrypt merged commit 5c7d73f into master Mar 5, 2016
@codefromthecrypt codefromthecrypt deleted the kafka-trois branch March 5, 2016 14:57
@kristofa
Copy link
Member Author

kristofa commented Mar 6, 2016

This change is released as part of brave 3.5.0

@ewhauser
Copy link
Contributor

@kristofa @adriancole - I don't believe it is necessary to front the KafkaSpanCollector with a queue. The Kafka producer that is in use is already async:

https://kafka.apache.org/082/javadoc/org/apache/kafka/clients/producer/KafkaProducer.html#send(org.apache.kafka.clients.producer.ProducerRecord)

@codefromthecrypt
Copy link
Member

@ewhauser ack. the main point is to move span reporting off the client thread, which has a side-effect of protecting it from exceptions and what-not as well. There's no intention to limit implementations to synchronous invocation, although what you've noted surely deserves a comment in the code.

@codefromthecrypt
Copy link
Member

ex. there's several exceptions that could crash the calling thread: this queue is implicitly protecting the caller from this sort of thing (although it isn't well tested). I can update the code to make this point more explicit.

https://github.com/apache/kafka/blob/trunk/clients/src/main/java/org/apache/kafka/clients/producer/KafkaProducer.java#L427

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