-
Notifications
You must be signed in to change notification settings - Fork 114
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
kafka reporter sendBatch #148
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jeqo are these good defaults?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: invalid url
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const ( | ||
defaultBatchInterval = time.Second * 1 // BatchInterval in seconds | ||
defaultBatchSize = 100 | ||
defaultMaxBacklog = 1000 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: dup line
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