-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Expose producer metrics with go-metrics #746
Conversation
} | ||
// Better be safe than sorry when computing the compression ratio | ||
if len(messageBlock.Msg.Value) != 0 { | ||
compressionRatio := float64(messageBlock.Msg.payloadSize) / |
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.
Is this calculation correct? Compression ratios are usually given as uncompressed / compressed
not the other way around. Or am I misunderstanding which value is which?
Perhaps you'd better rename payloadSize
to compressedPayloadSize
to be explicit about this.
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.
I think this makes sens as a metric, because it will give you a basic percentage,
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.
I followed the Java client for that metric, they named it compression-rate-avg
(The average compression rate of record batches) and it is the inverse of a regular compression ratio indeed, computed as a float:
https://github.com/apache/kafka/blob/7115c66aefb11efa802e61a42bcc13fadf92598d/clients/src/main/java/org/apache/kafka/common/record/Compressor.java#L150
But I found it confusing the first time I tried to interpret that metric from the Java client to be honest so I can change it to expose 200 for representing a compression ratio of 2.0 instead of 50.
Renaming payloadSize
to compressedPayloadSize
make sense.
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.
Ya, I think I'd prefer to do the normal thing than to follow upstream just for the sake of following upstream. It's always a toss-up though.
With the exception of the compression ratio issue I think this looks fine. @wvanbergen care to take a look? |
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.
This is a nice addition 👍
Do we have any idea what the performance impact of enabling metric collection is?
} | ||
// Better be safe than sorry when computing the compression ratio | ||
if len(messageBlock.Msg.Value) != 0 { | ||
compressionRatio := float64(messageBlock.Msg.payloadSize) / |
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.
I think this makes sens as a metric, because it will give you a basic percentage,
I published some benchmark results from the first PR about metrics in #701. But here are others against commit b17c88c when using the NOOP implementation for metrics and the real one:
Results are very similar again so it probably comes down to I/O and caching on my VM for the differences. |
Let me know what you think about the compression ratio format and I'll rebase the changes against the master branch to get rid of the new conflicts. |
- provide the following metrics: - batch-size histogram (global and per topic) - record-send-rate meter (global and per topic) - records-per-request histogram (global and per topic) - compression-ratio histogram (global and per topic) - add metrics.Registry parameter to the encode function - provide underlying MessageSet when encoding fake compressed "message" - use len(msg.Set.Messages) for counting records - use compressedSize property in Message for knowing the size of the compressed payload - expose the configured metric registry in ProduceRequest - expose current offset in packetEncoder for batch size metric - expose real encoder flag in packetEncoder for recording metrics only once - record metrics in produce_request.go - add unit tests and functional tests - use Spew library for building better DeepEqual error message when comparing raw bodies - add documentation for new metrics
b17c88c
to
124e7c6
Compare
I rebased against master to fix the merge conflict and applied the following changes:
|
batch-size
histogram (global and per topic)record-send-rate
meter (global and per topic)records-per-request
histogram (global and per topic)compression-rate
histogram (global and per topic)metrics.Registry
parameter to the encode functionMessageSet
when encoding fake compressed "message"len(msg.Set.Messages)
for counting recordspayloadSize
property inMessage
for knowing the size of the compressed payloadProduceRequest
packetEncoder
for batch size metricpacketEncoder
for recording metrics only onceproduce_request.go
DeepEqual
error message when comparing raw bodies