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

Sending multiple spans per HTTP request #211

Open
cpx86 opened this issue Jun 15, 2018 · 7 comments
Open

Sending multiple spans per HTTP request #211

cpx86 opened this issue Jun 15, 2018 · 7 comments

Comments

@cpx86
Copy link
Contributor

cpx86 commented Jun 15, 2018

The Zipkin API supports sending arrays of spans instead of single spans. In the current implementation it's not possible to take advantage of this since spans are serialized one-by-one. By buffering the spans and sending them in batches, the amount of network chatter with the API can be significantly reduced for systems with lots of incoming and outgoing requests.

Based on the current design, it seems ZipkinTraceReporter is the best place to make this change since it is the place where completed spans are serialized and sent. I would propose to buffer the spans here and asynchronously flush an array of spans to be serialized and sent. The Thrift serializers would need to be changed/regenerated as well to support serializing arrays instead of single spans.

Is this something you'd be interested in a PR for?

@fedj
Copy link
Collaborator

fedj commented Jun 15, 2018

While it would be great for HTTP, I think most of Kafka transports already support batching messages. However, I must admit that it can be really useful for HTTP.

However, I'm sure to understand what ZipkinTraceReporter refers to. From the top of my head, I would suggest to introduce a new class between ZipkinTracer and IZipkinSender.

What do you have in mind ?

@cpx86
Copy link
Contributor Author

cpx86 commented Jun 15, 2018

Good point. Using Confluent.Kafka you can set a queue buffering delay on the producer, so it waits for messages to buffer up before flushing. Having batched spans then could actually be harmful - if I recall correctly Kafka performs better with many small messages than a few large messages.

https://github.com/openzipkin/zipkin4net/blob/master/Src/zipkin4net/Src/Tracers/Zipkin/ZipkinTracerReporter.cs
This is the one I was referring to and it already sits in between ZipkinTracer and IZipkinSender :) So we could add buffering there if we want to.

To address your point about Kafka though, a solution could be to do control the serialization from the sender - then the sender can decide to serialize the spans as a batch or separately, depending on what's preferable for the transport. In that case we wouldn't really need the ZipkinTracerReporter anymore, so the call chain would then look something like e.g.:
ZipkinTracer -> HttpZipkinSender -> ThriftSerializer (serializing/sending spans as arrays)
ZipkinTracer -> KafkaZipkinSender -> ThriftSerializer (serializing/sending spans separately)

In this case, the HttpZipkinSender would be responsible for buffering the spans - which would be consistent with a Kafka sender since in that case buffering would be handled by the Kafka client library.

Sorry for the wall of text - this issue is apparently a bit more complex than I first considered it to be 😄

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Jun 15, 2018 via email

@cpx86
Copy link
Contributor Author

cpx86 commented Jun 15, 2018

@adriancole That's good input. In that case my initial idea might work, which was to implement buffering in ZipkinTracerReporter instead. Thoughts?

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Jun 15, 2018 via email

@cpx86
Copy link
Contributor Author

cpx86 commented Jun 15, 2018

That's an interesting idea. This would make the serializers/senders quite a bit more complex though - since the size isn't known until after serialization, the sender would need to serialize the spans one by one up to the optimal message size. Then the serializer would need to wrap them such that they become an array, right?

@codefromthecrypt
Copy link
Member

codefromthecrypt commented Jun 21, 2018 via email

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

No branches or pull requests

3 participants