kafka reporter sendBatch #148
Conversation
Signed-off-by: wanglongfei <wanglongfei@100tal.com>
|
Thanks for this contribution @sixiaobai, this PR has unbounded goroutines
issue that was fixed for the http one at
#146 so ideally we should
address it here.
I have some other concerns here that I could not find out answer myself for:
1. Should we enable batching/non batching reporting?
2. Are the default values for message size good enough?
Any thoughts on this @adriancole @basvanbeek?
|
Signed-off-by: wanglongfei <wanglongfei@100tal.com>
| const ( | ||
| defaultBatchInterval = time.Second * 1 // BatchInterval in seconds | ||
| defaultBatchSize = 100 | ||
| defaultMaxBacklog = 1000 |
There was a problem hiding this comment.
@jeqo are these good defaults?
There was a problem hiding this comment.
iiuc we are batching 100, and max size is 1000 bytes (?).
We have these defaults in java reporter: https://github.com/openzipkin/zipkin-reporter-java/blob/3f466e56012384ee685dd5cc91012129d312289b/kafka/src/main/java/zipkin2/reporter/kafka/KafkaSender.java#L81-L90
max request size is 1MB (default in kafka producer) and but somewhere we've discuss to reduce it to have and avoid too big batches, so 500KB should be a good default.
Make sure to align producer properties to reporter ones, similar to https://github.com/openzipkin/zipkin-reporter-java/blob/3f466e56012384ee685dd5cc91012129d312289b/kafka/src/main/java/zipkin2/reporter/kafka/KafkaSender.java#L131-L140
There was a problem hiding this comment.
@jeqo similar to http reporter:
When batch size reaches BatchSize, a collect will be triggered.
When batch size reaches MaxBacklog, spans from the beginning of the batch will be disposed.
So, the real batch size is dynamic, between 100 and 1000 (Exclude clock-triggered).
Assuming that the average span size is 500 bytes, so max size will be 500KB and 1000 should be a good default. The average span size is the thing we need to focus on.
Max request size is also 1MB in sarama kafka producer: https://github.com/Shopify/sarama/blob/master/config.go#L411
reporter/kafka/kafka.go
Outdated
| @@ -30,15 +32,31 @@ import ( | |||
| // defaultKafkaTopic sets the standard Kafka topic our Reporter will publish | |||
| // on. The default topic for zipkin-receiver-kafka is "zipkin", see: | |||
| // https://github.com/openzipkin/zipkin/tree/master/zipkin-receiver-kafka | |||
There was a problem hiding this comment.
| const ( | ||
| defaultBatchInterval = time.Second * 1 // BatchInterval in seconds | ||
| defaultBatchSize = 100 | ||
| defaultMaxBacklog = 1000 |
There was a problem hiding this comment.
iiuc we are batching 100, and max size is 1000 bytes (?).
We have these defaults in java reporter: https://github.com/openzipkin/zipkin-reporter-java/blob/3f466e56012384ee685dd5cc91012129d312289b/kafka/src/main/java/zipkin2/reporter/kafka/KafkaSender.java#L81-L90
max request size is 1MB (default in kafka producer) and but somewhere we've discuss to reduce it to have and avoid too big batches, so 500KB should be a good default.
Make sure to align producer properties to reporter ones, similar to https://github.com/openzipkin/zipkin-reporter-java/blob/3f466e56012384ee685dd5cc91012129d312289b/kafka/src/main/java/zipkin2/reporter/kafka/KafkaSender.java#L131-L140
| return func(r *kafkaReporter) { r.maxBacklog = n } | ||
| } | ||
|
|
||
| // Topic sets the kafka topic to attach the reporter producer on. |
Signed-off-by: wanglongfei <wanglongfei@100tal.com>
|
Will check this PR with @jeqo this wednesday. |
|
Should we align this number also in here @jeqo ? |
|
@jcchavezs I think so. Will give a try. |
|
@jcchavezs in order to apply a similar boundary as java reporter, we will need to batch based on bytes size, instead of number of elements. We can check this on a follow up PR. |
Signed-off-by: wanglongfei <wanglongfei@100tal.com>
Signed-off-by: wanglongfei wanglongfei@100tal.com