fix metric reporting #638
fix metric reporting #638
Conversation
|
@bai Any update on ETA for this MR to be accepted/merged? |
There was a problem hiding this comment.
@fr0stbyte it looks like the why of these can only be found in #637 (comment), I'd rather have this information as part of the commit messages of this PR.
there is also some new assertion to be made on the tests to check that this PR is doing what is supposed to be doing
| consumerPartitionLagGauge.With(labels).Set(float64(partition.CurrentLag)) | ||
|
|
||
| if partition.Complete >= 1.0 { |
There was a problem hiding this comment.
could Complete be greather than 1, or is this only a percentage and we could use here == 1.0?
| @@ -151,6 +151,5 @@ func TestHttpServer_handlePrometheusMetrics(t *testing.T) { | |||
| assert.Contains(t, promExp, `burrow_kafka_topic_partition_offset{cluster="testcluster",partition="1",topic="testtopic"} 5566`) | |||
| assert.Contains(t, promExp, `burrow_kafka_topic_partition_offset{cluster="testcluster",partition="0",topic="testtopic1"} 54`) | |||
There was a problem hiding this comment.
wasn't the idea to get a lag report for the topic incomplete? I think we are missing and assertion above as consumerPartitionLagGauge.With(labels).Set(float64(partition.CurrentLag)) will be called now even if Complete is below 1
|
I came across the same issue earlier this week, sorry i wasn't keeping an eye on the repo Which is very similar to this one 👍 |
|
Hullo, is there anything I can do to help move this PR forward? :) |
|
Sorry folks, I have been busy at work, will try to take another pass at this in the following days. |
|
I have the same issue and would love to see this PR merged. Has there been any progress? |
|
@d1egoaz let me know if the tests added are sufficient or you have other comments |
|
Going on a full year now - how can I help get this merged? Tagging @bai as a code owner. |
|
Thanks all! 🎉 🚢 |
ran into the same issue as #637 with partitions with 0 offsets - the patch below worked for me