Hi All, Please let me know if there is any other feedback or suggestions. Regards, Apoorv Mittal
On Fri, Nov 1, 2024 at 5:30 PM Apoorv Mittal <apoorvmitta...@gmail.com> wrote: > Thanks Jun and Andrew for reviewing and feedback. > > J1: I was thinking of having metrics as per the APIs in > SharePartitionManager but I think you are right. This is something not > beneficial to track as the release should only happen when a client sends a > leave group request. I have removed this metric. > > J2: Moved to Histogram. > > J3: Moved to Meter. I have removed RequestTopicPartitionsFetchEmptyCount > from KIP (as mentioned by Andrew - AS8) as RequestTopicPartitionsFetchRatio > moved to Histogram. Also I have marked InFlightMessageCount as Meter, so > shall capture the rate better. > > AS0: Done, specified specifically in the Motivation section. > > AS1: Hmmm, I thought about that earlier and was thinking about keeping the > abstracted share fetch APIs metrics in ShareGroupMetrics itself. However I > think the Share Fetch and Share Acknowledge can be part of BrokerTopicMetrics > as these operations happen by Share Consumer. Do you think we should also > move the RequestTopicPartitionsFetchRatio metric? I think we shouldn't and > it should be in ShareGroupMetrics itself, please let me know what you think. > > AS2: Yes you are right, it can support all topic rate as well. I have > added the suggestion. > > AS3: I have removed this metric as part of J1 comment. > > AS4,AS6: We already have a share-acknowledgement Meter metric defined in > KIP-932. I am of the opinion to move the KIP-932 share-acknowledgement > metric to TotalShareAcknowledgementRequestsPerSec. Hence I have added > corresponding FailedShareAcknowledgementRequestsPerSec in this KIP. > > AS5: It counts the failure of a share acknowledge request. Similar to > multi topic partition fetch, acknowledge can also be multi topic partition > but a single acknowledge request might either fail completely or at topic > partition level. Hence this metric tracks the failure as similar to > FailedFetchRequestsPerSec or FailedShareFetchRequestsPerSec. > > AS7: I have added some examples and another metric FetchLockTimeMs, which > should help in optimizing behaviour. > > AS8: Yes, we might be able to work with low percentiles to investigate > further hence I have removed RequestTopicPartitionsFetchEmptyCount. > However Histogram will give us percentiles but not the raw bucketed > value, in case we think we need such data explicitly then might consider > adding it but for now it's removed. > > > Regards, > Apoorv Mittal > > > On Thu, Oct 31, 2024 at 1:03 PM Andrew Schofield < > andrew_schofield_j...@outlook.com> wrote: > >> Hi Apoorv, >> Thanks for the KIP. I have some comments. >> >> AS0: These are broker metrics and not client metrics. I don't think the >> KIP states that and it probably should. >> >> AS1: TotalShareFetchRequestsPerSec is inspired by the existing metric >> TotalFetchRequestsPerSec. That one is in the BrokerTopicMetrics group >> so I would expect the new metric to be in the same group. This comment >> applies to all of the ShareGroupMetrics broker metrics scoped to a topic. >> >> AS2: Similarly to TotalFetchRequestPerSec, does the new metric support >> omitting the topic tag in order to obtain the all-topic rate? I believe so >> because it follows the existing metrics. I guess the tag is actually >> `topic=([-.\w]+)` to copy the existing notation. >> >> AS3: What's a share release? Does it count releases which are made >> by ShareFetch/Acknowledge requests as well as implicit ones as a >> result of share session expiration or closure? >> >> AS4: I see there is FailedShareAcknowledgementRequestsPerSec. >> If this corresponds specifically to the ShareAcknowledge API, then >> the name should be FailedShareAcknowledgeRequestsPerSec. >> >> AS5: What does FailedShareAcknowledgementRequestsPerSec >> actually count? This is a per-topic metric and a ShareAcknowledge >> can mention several topics, but actually it can also operate on many >> acquired records specifying any of the different acknowledgement >> types that KIP-932 defines. >> >> AS6: Why no TotalShareAcknowledgementRequestsPerSec? >> >> AS7: I think I like the concept behind RequestTopicPartitionsFetchRatio, >> but it would really help to include some examples of how it works. You >> mention 4 potential causes for low ratios, so it would be nice to see how >> each of them would be evident from the metrics and configurations. >> This is going to be valuable information for operating this in production. >> >> AS8: As Jun mentioned, maybe a histogram would be appropriate >> for RequestTopicPartitionsFetchRatio. Then I wonder whether it's >> worth having a separate RequestTopicPartitionsFetchEmptyCount since >> those are requests for which the fetch ratio was 0. >> >> Thanks, >> Andrew >> ________________________________________ >> From: Jun Rao <j...@confluent.io.INVALID> >> Sent: 30 October 2024 23:07 >> To: dev@kafka.apache.org <dev@kafka.apache.org> >> Subject: Re: [DISCUSS] KIP-1103: Additional metrics for cooperative >> consumption >> >> Hi, Apoorv, >> >> Thanks for the KIP. A few comments. >> >> 1. TotalShareReleaseRequestsPerSec: This is a bit weird since there is >> no ShareReleaseRequest. >> >> 2. RequestTopicPartitionsFetchRatio and InFlightBatchMessageCount: It >> seems >> Histogram is more appropriate for them. >> >> 3. RequestTopicPartitionsFetchEmptyCount and AcquisitionLockTimeoutCount: >> The problem with using gauge is that if the value changes quickly, we may >> not be able to capture the occurrence of empty fetch or lock timeout. >> Meter >> captures those rare events better. >> >> Jun >> >> >> On Tue, Oct 29, 2024 at 8:48 AM Apoorv Mittal <apoorvmitta...@gmail.com> >> wrote: >> >> > Hi Everyone, >> > I would like to start a discussion on KIP-1103: >> > >> > >> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1103%3A+Additional+metrics+for+cooperative+consumption >> > >> > This KIP extends KIP-932 to provide additional metrics for >> > Queues/Cooperative consumption. >> > >> > Regards, >> > Apoorv Mittal >> > >> >
