KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics#11474
KAFKA-13354: Topic metrics count request rate inconsistently with other request metrics#11474dongjinleekr wants to merge 9 commits intoapache:trunkfrom
Conversation
|
@splett2 Is this approach the same thing you thought? |
There was a problem hiding this comment.
we would want this metric to be incremented once per request instead of once per TP.
ditto for totalProduceRequestRate, totalFetchRequestRate, failedFetchRequestRate
Otherwise the allTopicsStats will be inconsistent with the sum of individual topic stats.
There was a problem hiding this comment.
(After some thinking) Oh, you are right. I omitted them. 😓
There was a problem hiding this comment.
nit: maybe something like
requestedTopics.map(brokerTopicStats.topicStats).foreach(_.totalProduceRequestRate.mark())
flows a bit better.
There was a problem hiding this comment.
The downside of using 'map' is that it will create an intermediate collection.
There was a problem hiding this comment.
this is a good point, although I would imagine that the set of topics per request is usually small. In that case it may be fine to keep the update code as is. Maybe change:
case topic => to just topic => as i dont think the case is necessary for iterating over a set.
There was a problem hiding this comment.
all // in short, just replace case topic => to topic =>; do I understand correctly?
|
@dajac @splett2 Here is the update. I updated/added some tests to check whether the metrics work correctly. Add to this, I found that some methods and variables in |
…rTopicStats.topicStats. 2. Remove redundant 'case's from the blocks.
dongjinleekr
force-pushed
the
feature/KAFKA-13354
branch
from
9f8065a to
a583517
Compare
|
This PR is being marked as stale since it has not had any activity in 90 days. If you If you are having difficulty finding a reviewer, please reach out on the [mailing list](https://kafka.apache.org/contact). If this PR is no longer valid or desired, please feel free to close it. If no activity occurs in the next 30 days, it will be automatically closed. |
|
This PR has been closed since it has not had any activity in 120 days. If you feel like this |
3 participants
I inspected this issue a little bit and found the following:
There are 18 metrics in
kafka.server:type=BrokerTopicMetricsand 4 of them should be counted per request, but actually counted perTopicPartition:kafka.server:type=BrokerTopicMetrics,name=TotalProduceRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=TotalFetchRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=FailedProduceRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=FailedFetchRequestsPerSec5 of them are omitted in the documentation (see: KAFKA-13436: Omitted BrokerTopicMetrics metrics in the documentation #11473)
kafka.server:type=BrokerTopicMetrics,name=TotalProduceRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=TotalFetchRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=FailedProduceRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=FailedFetchRequestsPerSeckafka.server:type=BrokerTopicMetrics,name=BytesRejectedPerSecCommitter Checklist (excluded from commit message)