dongjinleekr commented on PR #14228: URL: https://github.com/apache/kafka/pull/14228#issuecomment-1681837633
I reviewed this problem and found that: **the documentation states that the following metrics has per-topic stats but, actually they do not exist in per-topic `BrokerTopicMetrics` object.** 1. `kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesInPerSec` (`BrokerTopicStats.ReplicationBytesInPerSec`) 2. `kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesOutPerSec` (`BrokerTopicStats.ReplicationBytesOutPerSec`) 3. `kafka.server:type=BrokerTopicMetrics,name=ReassignmentBytesInPerSec` (`BrokerTopicStats.ReassignmentBytesInPerSec`) 4. `kafka.server:type=BrokerTopicMetrics,name=ReassignmentBytesOutPerSec` (`BrokerTopicStats.ReassignmentBytesOutPerSec`) # Initialization All `kafka.server:type=BrokerTopicMetrics` metrics are initialized into `BrokerTopicMetrics.metricTypeMap`. However, those 4 metrics are initialized only when the `name` parameter is `None` - that is, all-topic metrics only, without the `topic=XXX` tag. https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L270-L292 `BrokerTopicMetrics` object also does not return the `Meter` object for those 4 metrics, unless it is an all-topic metrics object. https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L317-L331 # Stat Update Dislike to the other per-topic metrics, those 4 metris uses different update methods. other per-topic metrics: https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L402-L403 4 metrics above: https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L405-L427 # Removal Oddly, 2 and 4 are removed from `topicMetrics` with other per-topic metrics as leader-only metrics. (Line 439, 440) https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L430-L448 Similarly, 1 and 3 are removed from `topicMetrics` as follower-only metrics. https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L451-L457 # Other ecosystem tools Cruise Control uses these phantom metrics (1 and 2) in one of their metric samplers. Note `topic!=""` [here](https://github.com/linkedin/cruise-control/blob/28395928a2fed7dd01ff48efedd27ab351ca912a/cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/monitor/sampling/prometheus/DefaultPrometheusQuerySupplier.java#L181-L184). # Conclusion It seems like we have the following two options: ## A. Make the implementation follow the documentation - that is, update the `BrokerTopicStats.updateXXXX` methods. `BrokerTopicStats.update(Replication|Reassignment)BytesIn` methods are called from `ReplicaFetcherThread.processPartitionData` so we can be sure they are applied to the exact per-topic metrics only. https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/ReplicaFetcherThread.scala#L147-L150 Plus, the `BrokerTopicStats.update(Replication|Reassignment)BytesOut` methods are called from `BrokerTopicStats.updateBytesOut` method only. So we can also be certain that the stats will be applied correctly. ## B. Make the documentation follow the implementation. This decision impacts the other ecosystem tools, and maybe require a KIP. @mimaison @ijuma How do you think? +1. Long time no see. I have been extremely busy for building our in-house Kafka distro et al. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: jira-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
