You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "bachmanity1 (via GitHub)" <gi...@apache.org> on 2023/08/16 18:26:10 UTC
[GitHub] [kafka] bachmanity1 opened a new pull request, #14228: MINOR: Fix ReplicationBytes(Out|In)PerSec metrics' docs
bachmanity1 opened a new pull request, #14228:
URL: https://github.com/apache/kafka/pull/14228
Based on the code shown below it looks like `ReplicationBytes(Out|In)PerSec` metrics are reported only across all topics and NOT for each individual topic. https://github.com/apache/kafka/blob/a9efca0bf63110d68f84fc9841d8a31f245e10e0/core/src/main/scala/kafka/server/KafkaRequestHandler.scala#L405-L416
### Committer Checklist (excluded from commit message)
- [ ] Verify design and implementation
- [ ] Verify test coverage and CI build status
- [ ] Verify documentation (including upgrade notes)
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] divijvaidya commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "divijvaidya (via GitHub)" <gi...@apache.org>.
divijvaidya commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1692083582
> @divijvaidya @showuon can you have a look, please? Thanks!
Sorry I won't reach to reviewing this PR any time soon. Already have a big list of pending PRs on my plate.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1764507081
Thanks, merged!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1764464013
We only sync docs from the kafka repo to the website when we do a release. Can you open a PR against [kafka-site](https://github.com/apache/kafka-site) to update the website directly?
You don't need to update all versions, I guess only updating https://github.com/apache/kafka-site/blob/asf-site/36/ops.html should be enough. Thanks
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1764456839
Sure, changing the metrics is also an option, thanks for starting a KIP.
I'll still merge this so the docs represent the current state.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1754554650
@mimaison @ijuma kind reminder
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1764500590
@mimaison I've opened [PR](https://github.com/apache/kafka-site/pull/561) in the kafka-site repo.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison merged PR #14228:
URL: https://github.com/apache/kafka/pull/14228
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "dongjinleekr (via GitHub)" <gi...@apache.org>.
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] dongjinleekr commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "dongjinleekr (via GitHub)" <gi...@apache.org>.
dongjinleekr commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1681863888
@bachmanity1
> I think that the documentation for the `ReassignmentBytes(In|Out)PerSec` metrics doesn't say that these metrics are reported per topic and is actually correct.
OK. Then, let's focus on `ReplicatiationBytes(In|Out)PerSec` (1 and 2) only. It seems like we have little reason to update the contract on `ReassignmentBytes(In|Out)PerSec` (3 and 4).
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bachmanity1 commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1681849815
> 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.
>kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesInPerSec (BrokerTopicStats.ReplicationBytesInPerSec)
kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesOutPerSec (BrokerTopicStats.ReplicationBytesOutPerSec)
kafka.server:type=BrokerTopicMetrics,name=ReassignmentBytesInPerSec (BrokerTopicStats.ReassignmentBytesInPerSec)
kafka.server:type=BrokerTopicMetrics,name=ReassignmentBytesOutPerSec (BrokerTopicStats.ReassignmentBytesOutPerSec)
I think that the documentation for the `ReassignmentBytes(In|Out)PerSec` metrics doesn't say that these metrics are reported per topic and is actually correct.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bachmanity1 commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1691612191
@divijvaidya @showuon can you have a look, please? Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1764451071
Hi @mimaison, thanks a lot for the review! I've removed the extra closing parenthesis.
Alternatively, I've thought that maybe current documentation actually makes sense and we should instead patch current code to expose replication metrics for each topic. I've authored a [KIP](https://cwiki.apache.org/confluence/display/KAFKA/KIP-973%3A+Expose+per+topic+replication+rate+metrics) that suggests this change. If it makes sense to you, I'd appreciate it if you could join the discussion thread related to this proposal.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bachmanity1 commented on pull request #14228: MINOR: Fix ReplicationBytes(Out|In)PerSec metrics' docs
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1681087535
@mimaison @ijuma @dongjinleekr can you please review this? Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bachmanity1 commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1725231949
Hi @nizhikov @tledkov,
I've seen your kind offer to help the community with reviews.
Can you please have a look at this PR? I've also opened a [KIP](https://cwiki.apache.org/confluence/display/KAFKA/KIP-973%3A+Expose+per+topic+replication+rate+metrics) that offers an alternative solution by aligning implementation with current docs. If you have time, can you have a look at it too? Thanks!
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics [kafka]
Posted by "mimaison (via GitHub)" <gi...@apache.org>.
mimaison commented on code in PR #14228:
URL: https://github.com/apache/kafka/pull/14228#discussion_r1360428852
##########
docs/ops.html:
##########
@@ -1537,8 +1537,8 @@ <h4 class="anchor-heading"><a id="remote_jmx" class="anchor-link"></a><a href="#
</tr>
<tr>
<td>Byte out rate to other brokers</td>
- <td>kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesOutPerSec,topic=([-.\w]+)</td>
- <td>Byte out (to the other brokers) rate per topic. Omitting 'topic=(...)' will yield the all-topic rate.</td>
+ <td>kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesOutPerSec)</td>
Review Comment:
We don't need the extra closing parenthesis at the end
##########
docs/ops.html:
##########
@@ -1453,8 +1453,8 @@ <h4 class="anchor-heading"><a id="remote_jmx" class="anchor-link"></a><a href="#
</tr>
<tr>
<td>Byte in rate from other brokers</td>
- <td>kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesInPerSec,topic=([-.\w]+)</td>
- <td>Byte in (from the other brokers) rate per topic. Omitting 'topic=(...)' will yield the all-topic rate.</td>
+ <td>kafka.server:type=BrokerTopicMetrics,name=ReplicationBytesInPerSec)</td>
Review Comment:
We don't need the extra closing parenthesis at the end
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [kafka] bachmanity1 commented on pull request #14228: MINOR: Fix docs for ReplicationBytes(Out|In)PerSec metrics
Posted by "bachmanity1 (via GitHub)" <gi...@apache.org>.
bachmanity1 commented on PR #14228:
URL: https://github.com/apache/kafka/pull/14228#issuecomment-1715743687
@showuon @ijuma @mimaison
Just a kind reminder.
I've also opened KIP related to this issue https://cwiki.apache.org/confluence/display/KAFKA/KIP-973%3A+Expose+per+topic+replication+rate+metrics.
--
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-unsubscribe@kafka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org