-
Notifications
You must be signed in to change notification settings - Fork 141
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
Collect consumer metrics #1143
Collect consumer metrics #1143
Conversation
- add metric `allQueueSizeHistogram` - better metrics descriptions - better bucket boundaries - observe metrics in the background
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.
Nice, we should definitely have this!
I added suggestions for additional metrics. Let's think about the histogram dimension.
zio-kafka/src/main/scala/zio/kafka/consumer/ConsumerSettings.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
As last fallback use hash code of ConsumerSettings instead of random value.
zio-kafka/src/main/scala/zio/kafka/consumer/ConsumerSettings.scala
Outdated
Show resolved
Hide resolved
@svroonland I would like to stop here and merge the PR. The following metrics from your list were not implemented:
Your review and ideas are welcome and valued as always. |
Of course, we can always implement the other metrics as follow-ups, consider them more ideas. Will have 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.
Great additions. More stuff to consider in the comments
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/Runloop.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/Runloop.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/Runloop.scala
Outdated
Show resolved
Hide resolved
zio-kafka/src/main/scala/zio/kafka/consumer/internal/ConsumerMetrics.scala
Outdated
Show resolved
Hide resolved
Latency of aggregated commits no longer includes the lead time from commit request to start of commit. Also: use unit in metric name as recommended by Prometheus guide.
maxPollInterval: Duration, | ||
commitTimeout: Duration, |
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.
Good to have less parameters here
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.
One tiny thing to think about, but otherwise looks great!
Collect metrics for the consumer using the zio-metrics API. This allows any zio-metrics backend to access and process the observed values.
By default no tags are added, but this can be configured via the new method
ConsumerSettings.withMetricsLabels
.The following metrics are collected (kudos to @svroonland for most of the ideas):
1
for subscribed,0
of unsubscribed (gauge).Like the zio-metrics API we follow Prometheus conventions. This means that:
The histograms each use 10 buckets. To reach a decent range while keeping sufficient accuracy at the low end, most bucket boundaries use an exponential series based on 𝑒.
The following metric ideas were also raised, but these are kept for future work: