Hi Vahid, Thank you so much for the review and voting!
Best, Qichao On Wed, Nov 29, 2023 at 2:55 PM Vahid Hashemian <va...@apache.org> wrote: > Hi Qichao, > > Thanks for answering my questions and updating the KIP accordingly. > > It looks good to me. > > --Vahid > > > On Tue, Nov 28, 2023, 7:22 PM Qichao Chu <qic...@uber.com.invalid> wrote: > > > Hi Vahid, > > > > Thank you for taking the time to review the KIP and asking great > questions. > > > > The execution path mentioned is exactly how this KIP is going to > function. > > We believe this config, compared with many other configurations like > quota, > > will not pose significant alteration to the broker's functionality. Thus > > overwriting the permanent config dynamically is acceptable. After > thinking > > about this topic again, I think your question makes a lot of sense and we > > shouldn't override the permanent config. Permanent config usually > > represents the normal operation condition, so the temporal config should > be > > removed after the cluster is restarted/re-provisioned to be 'clean'. This > > also helps to prevent undesirable behavior and to reduce the risk > involved > > in operation. I have updated the KIP to mention this behavior. > > > > `Medium` level is not mentioned as we don't have any medium-level metrics > > for now. This is for future extension. I have also updated the KIP to > > reflect this information. > > > > Best, > > Qichao > > > > > > > > On Tue, Nov 28, 2023 at 9:22 AM Vahid Hashemian <va...@apache.org> > wrote: > > > > > Hi Qichao, > > > > > > Thanks for proposing this KIP. It'd be super valuable to have the > ability > > > to have those partition level metrics for Kafka topics. > > > > > > Sorry I'm late to the discussion. I just wanted to bring up a point > > > for clarification and one question: > > > > > > Let's assume that a production cluster cannot afford to enable high > > > verbosity on a permanent basis (at least not for all topics) due to > > > performance concerns. > > > > > > Since this new config can be set dynamically, in case of an issue or > > > investigation that warrants obtaining partition level metrics, one can > > > simply enable high verbosity for select topic(s), temporarily collect > > > metrics at partition level, and then change the config back to the > > previous > > > setting. Since the config values are not set incrementally, the > operator > > > would need to run a `describe` to get the existing config first, and > then > > > amend it to enable high verbosity for the topic(s) of interest. > Finally, > > > when the investigation concludes, the config has to be reverted to its > > > permanent setting. > > > > > > If the above execution path makes sense, in case the operator forgets > to > > > take an inventory of the existing (permanent) config and simply > > overwrites > > > it, then that permanent config will be gone and not retrievable. Is > this > > > correct? > > > > > > We usually don't need to temporarily change broker configs and I see > this > > > config as one that can be temporarily changed. So keeping track of what > > the > > > value was before the change is rather important. > > > > > > Aside from this point, my question is: What's the impact of `medium` > > > setting for `level`? I couldn't find it described in the KIP. > > > > > > Thanks! > > > --Vahid > > > > > > > > > > > > On Mon, Nov 13, 2023 at 5:34 AM Divij Vaidya <divijvaidy...@gmail.com> > > > wrote: > > > > > > > Thank you for updating the KIP Qichao. > > > > > > > > I don't have any more questions or suggestions. Looks good to move > > > forward > > > > from my perspective. > > > > > > > > > > > > > > > > -- > > > > Divij Vaidya > > > > > > > > > > > > > > > > On Fri, Nov 10, 2023 at 2:25 PM Qichao Chu <qic...@uber.com.invalid> > > > > wrote: > > > > > > > > > Thank you again for the nice suggestions, Jorge! > > > > > I will wait for Divij's response and move it to the vote stage once > > the > > > > > generic filter part reached concensus. > > > > > > > > > > Qichao Chu > > > > > Software Engineer | Data - Kafka > > > > > [image: Uber] <https://uber.com/> > > > > > > > > > > > > > > > On Fri, Nov 10, 2023 at 6:49 AM Jorge Esteban Quilcate Otoya < > > > > > quilcate.jo...@gmail.com> wrote: > > > > > > > > > > > Hi Qichao, > > > > > > > > > > > > Thanks for updating the KIP, all updates look good to me. > > > > > > > > > > > > Looking forward to see this KIP moving forward! > > > > > > > > > > > > Cheers, > > > > > > Jorge. > > > > > > > > > > > > > > > > > > > > > > > > On Wed, 8 Nov 2023 at 08:55, Qichao Chu <qic...@uber.com.invalid > > > > > > wrote: > > > > > > > > > > > > > Hi Divij, > > > > > > > > > > > > > > Thank you for the feedback. I updated the KIP to make it a > little > > > bit > > > > > > more > > > > > > > generic: filters will stay in an array instead of different > > > top-level > > > > > > > objects. In this way, if we need language filters in the > future. > > > The > > > > > > logic > > > > > > > relationship of filters is also added. > > > > > > > > > > > > > > Hi Jorge, > > > > > > > > > > > > > > Thank you for the review and great comments. Here is the reply > > for > > > > each > > > > > > of > > > > > > > the suggestions: > > > > > > > > > > > > > > 1) The words describing the property are now updated to include > > > more > > > > > > > details of the keys in the JSON. It also explicitly mentions > the > > > JSON > > > > > > > nature of the config now. > > > > > > > 2) The JSON entries should be non-conflict so the order is not > > > > > relevant. > > > > > > If > > > > > > > there's conflict, the conflict resolution rules are stated in > the > > > > KIP. > > > > > To > > > > > > > make it more clear, ordering and duplication rules are updated > in > > > the > > > > > > > Restrictions section of the *level* property. > > > > > > > 3) Yeah we did take a look at the RecordingLevel config and it > > does > > > > not > > > > > > > work for this case. The RecodingLevel config does not offer the > > > > > > capability > > > > > > > of filtering and it has a drawback of needing to be added to > all > > > the > > > > > > future > > > > > > > sensors. To reduce the duplication, I propose we merge the > > > > > RecordingLevel > > > > > > > to this more generic config in the future. Please take a look > > into > > > > the > > > > > > > *Using > > > > > > > the Existing RecordingLevel Config* section under *Rejected > > > > > Alternatives* > > > > > > > for more details. > > > > > > > 4) This suggestion makes a lot of sense. My idea is to create a > > > > > > > table/form/doc in the documentation for the verbosity levels of > > all > > > > > > metric > > > > > > > series. If it's too verbose to be in the docs, I will update > the > > > KIP > > > > to > > > > > > > include this info. I will create a JIRA for this effort once > the > > > KIP > > > > is > > > > > > > approved. > > > > > > > 5) Sure we can expand to all other series, added to the KIP. > > > > > > > 6) Added a new section(*Working with the Configuration via > CLI)* > > > with > > > > > the > > > > > > > user experience details > > > > > > > 7) Links are updated. > > > > > > > > > > > > > > Please take another look and let me know if you have any more > > > > concerns. > > > > > > > > > > > > > > Best, > > > > > > > Qichao Chu > > > > > > > Software Engineer | Data - Kafka > > > > > > > [image: Uber] <https://uber.com/> > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 8, 2023 at 6:29 AM Jorge Esteban Quilcate Otoya < > > > > > > > quilcate.jo...@gmail.com> wrote: > > > > > > > > > > > > > > > Hi Qichao, > > > > > > > > > > > > > > > > Thanks for the KIP! This will be a valuable contribution and > > > > improve > > > > > > the > > > > > > > > tooling for troubleshooting. > > > > > > > > > > > > > > > > I have a couple of comments: > > > > > > > > > > > > > > > > 1. It's unclear from the `metrics.verbosity` description what > > the > > > > > > > supported > > > > > > > > values are. In the description mentions "If the value is high > > ... > > > > In > > > > > > the > > > > > > > > low settings" but I think it's referring to the `level` > > property > > > > > > > > specifically instead of the whole value that is now JSON. > Could > > > you > > > > > > > clarify > > > > > > > > this? > > > > > > > > > > > > > > > > 2. Could we state in which order the JSON entries are going > to > > be > > > > > > > > evaluated? I guess the last entry wins if it overlaps > previous > > > > > values, > > > > > > > but > > > > > > > > better to make this explicit. > > > > > > > > > > > > > > > > 3. Kafka metrics library has a `RecordingLevel` configuration > > -- > > > > have > > > > > > we > > > > > > > > considered aligning these concepts and maybe reuse it instead > > of > > > > > > > > `verbosityLevel`? Then we can reuse the levels: INFO, DEBUG, > > > TRACE. > > > > > > > > > > > > > > > > 4. Not sure if within the scope of the KIP, but would be > > helpful > > > to > > > > > > > > document the metrics with the verbosity level attached to the > > > > > metrics. > > > > > > > > Maybe creating a JIRA ticket to track this would be enough if > > we > > > > > can't > > > > > > > > cover it as part of the KIP. > > > > > > > > > > > > > > > > 5. Could we consider the following client-related metrics as > > > well: > > > > > > > > - BytesRejectedPerSec > > > > > > > > - TotalProduceRequestsPerSec > > > > > > > > - TotalFetchRequestsPerSec > > > > > > > > - FailedProduceRequestsPerSec > > > > > > > > - FailedFetchRequestsPerSec > > > > > > > > - FetchMessageConversionsPerSec > > > > > > > > - ProduceMessageConversionsPerSec > > > > > > > > Would be great to have these from day 1 instead of requiring > a > > > > > > following > > > > > > > > KIP to extend this. Could be implemented in separate PRs if > > > needed. > > > > > > > > > > > > > > > > 6. To make it clearer how the user experience would be, could > > we > > > > > > provide > > > > > > > an > > > > > > > > example of: > > > > > > > > - how the broker configuration will be provided by default, > and > > > > > > > > - how the CLI tooling would be used to change the > > configuration? > > > > > > > > - Maybe a couple of scenarios: adding a new metric config, a > > > second > > > > > one > > > > > > > > with overlapping values, and > > > > > > > > - describing the expected metrics to be mapped > > > > > > > > > > > > > > > > A couple of nits: > > > > > > > > - The first link "MessagesInPerSec metrics" is pointing to > > > > > > > > https://kafka.apache.org/documentation/#uses_metrics -- is > > this > > > > the > > > > > > > > correct > > > > > > > > reference? It doesn't seem too relevant. > > > > > > > > - Also, the link to ReplicaManager points to a line that has > > > change > > > > > > > > already; better to have a permalink to a specific commit: > e.g. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://github.com/apache/kafka/blob/edc7e10a745c350ad1efa9e4866370dc8ea0e034/core/src/main/scala/kafka/server/ReplicaManager.scala#L1218 > > > > > > > > > > > > > > > > Cheers, > > > > > > > > Jorge. > > > > > > > > > > > > > > > > On Tue, 7 Nov 2023 at 17:06, Qichao Chu > > <qic...@uber.com.invalid > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Hi Divij, > > > > > > > > > > > > > > > > > > It would be very nice if you could take a look at the > recent > > > > > changes, > > > > > > > > thank > > > > > > > > > you! > > > > > > > > > If there's no more required changes, shall we move to vote > > > stage? > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > Qichao Chu > > > > > > > > > Software Engineer | Data - Kafka > > > > > > > > > [image: Uber] <https://uber.com/> > > > > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Nov 2, 2023 at 12:06 AM Qichao Chu < > qic...@uber.com> > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Divij, > > > > > > > > > > > > > > > > > > > > Thank you for the very quick response and the nice > > > > suggestions. I > > > > > > > have > > > > > > > > > > updated the KIP with the following thoughts. > > > > > > > > > > > > > > > > > > > > 1. I checked the Java documentation and it seems the > regex > > > > engine > > > > > > in > > > > > > > > > utils > > > > > > > > > > < > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/regex/Pattern.html > > > > > > > > > > > > > > > > > > is > > > > > > > > > > not 100% compatible with PCRE, though it is very close. I > > > > stated > > > > > > > > > > the Java implementation as the requirement since we are > > most > > > > > likely > > > > > > > to > > > > > > > > > > target a JVM language. > > > > > > > > > > 2. Agreed with the filter limitation. For now, let's keep > > it > > > > > topic > > > > > > > > only. > > > > > > > > > > With that in mind, I feel we do have cases where a user > > wants > > > > to > > > > > > list > > > > > > > > > many > > > > > > > > > > topics. Although regex is also possible, an array will > make > > > > > things > > > > > > > > > faster. > > > > > > > > > > This makes me add two options for the topic filter. > > > > > > > > > > 3. It seems not many configs are using JSON, this was the > > > > > intention > > > > > > > for > > > > > > > > > me > > > > > > > > > > to use a compound string. However since JSON is used > widely > > > in > > > > > the > > > > > > > > > project, > > > > > > > > > > and given the benefits you mentioned earlier, I tried to > > make > > > > the > > > > > > > > config > > > > > > > > > a > > > > > > > > > > JSON array. The change is to make it compatible with > > > > multi-level > > > > > > > > > settings. > > > > > > > > > > > > > > > > > > > > Let me know what you think. Many thanks! > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > > > > Qichao Chu > > > > > > > > > > Software Engineer | Data - Kafka > > > > > > > > > > [image: Uber] <https://uber.com/> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, Nov 1, 2023 at 9:43 PM Divij Vaidya < > > > > > > divijvaidy...@gmail.com > > > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > >> Thank you for making the changes Qichao. > > > > > > > > > >> > > > > > > > > > >> We are now entering in the territory of defining a > > > declarative > > > > > > > schema > > > > > > > > > for > > > > > > > > > >> filters. In the new input format, the type is string but > > we > > > > are > > > > > > > > > imposing a > > > > > > > > > >> schema for the string and we should clearly call out the > > > > schema. > > > > > > You > > > > > > > > can > > > > > > > > > >> perhaps choose to adopt a schema such as below: > > > > > > > > > >> > > > > > > > > > >> metricLevel = High | Low (default: Low) > > > > > > > > > >> metricNameRegEx = regEx (default: .*) > > > > > > > > > >> nameOfDimension = string > > > > > > > > > >> dimensionRegEx = regEx > > > > > > > > > >> dimensionFilter = [<nameOfDimension>=<dimensionRegEx>] > > > > (default: > > > > > > []) > > > > > > > > > >> > > > > > > > > > >> Final Value schema = "level"=$metricLevel, > > > > > > "name"=$metricNameRegEx, > > > > > > > > > >> $dimensionFilter > > > > > > > > > >> > > > > > > > > > >> Further we need to answer questions such as : > > > > > > > > > >> 1. which regEx format do we support (it should probably > be > > > > > > > > > Perl-compatible > > > > > > > > > >> regular expressions (PCRE) because Java's regEx is > > > compatible > > > > > with > > > > > > > it) > > > > > > > > > >> 2. should we restrict the dimensionFilter to at max > > length 1 > > > > and > > > > > > > value > > > > > > > > > >> "topic" only for now. Later when we want to expand, we > can > > > > > expand > > > > > > > > > filters > > > > > > > > > >> for other dimensions as well such as partitions. > > > > > > > > > >> 3. if we are coming up with our stringified-schema, why > > not > > > > use > > > > > > > json? > > > > > > > > It > > > > > > > > > >> would save us from building a parsing utility for the > > > schema. > > > > (I > > > > > > > like > > > > > > > > it > > > > > > > > > >> in > > > > > > > > > >> its current format but there is a case to be made for > json > > > as > > > > > > well) > > > > > > > > > >> 4. what happens when there are contradictory regEx > rules, > > > > e.g. a > > > > > > > topic > > > > > > > > > >> defined in high as well as low. It is generally solved > by > > > > > defining > > > > > > > > > >> precedence. In our case, we can choose that high has > more > > > > > > precedence > > > > > > > > > than > > > > > > > > > >> low. > > > > > > > > > >> > > > > > > > > > >> What do you think? > > > > > > > > > >> > > > > > > > > > >> -- > > > > > > > > > >> Divij Vaidya > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> > > > > > > > > > >> On Wed, Nov 1, 2023 at 2:07 PM Qichao Chu > > > > > <qic...@uber.com.invalid > > > > > > > > > > > > > > > > >> wrote: > > > > > > > > > >> > > > > > > > > > >> > Hi Divij, > > > > > > > > > >> > > > > > > > > > > >> > Thank you for the review and the great suggestions, > > > again. I > > > > > > have > > > > > > > > > >> updated > > > > > > > > > >> > the corresponding content, can you take another look? > > > > > > > > > >> > Regarding the KIP-544 style regex, I have added it to > > the > > > > new > > > > > > > > property > > > > > > > > > >> too. > > > > > > > > > >> > It's expanded to include multiple sections for better > > > future > > > > > > > > > extension. > > > > > > > > > >> > > > > > > > > > > >> > Best, > > > > > > > > > >> > Qichao Chu > > > > > > > > > >> > Software Engineer | Data - Kafka > > > > > > > > > >> > [image: Uber] <https://uber.com/> > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > >> > On Mon, Oct 30, 2023 at 6:26 PM Divij Vaidya < > > > > > > > > divijvaidy...@gmail.com > > > > > > > > > > > > > > > > > > > >> > wrote: > > > > > > > > > >> > > > > > > > > > > >> > > Hey *Qichao* > > > > > > > > > >> > > > > > > > > > > > >> > > Thank you for the update on the KIP. I like the idea > > of > > > > > > > > incremental > > > > > > > > > >> > > delivery and adding which metrics support this > > verbosity > > > > as > > > > > a > > > > > > > > later > > > > > > > > > >> KIP. > > > > > > > > > >> > > But I also want to ensure that we wouldn't have to > > > change > > > > > the > > > > > > > > > current > > > > > > > > > >> > > config when adding that in future. Hence, we need > some > > > > > > > discussion > > > > > > > > on > > > > > > > > > >> it > > > > > > > > > >> > in > > > > > > > > > >> > > the scope of the KIP. > > > > > > > > > >> > > > > > > > > > > > >> > > About the dynamic configuration: > > > > > > > > > >> > > Do we need to add the "default" mode? I am asking > > > because > > > > it > > > > > > may > > > > > > > > > >> inhibit > > > > > > > > > >> > us > > > > > > > > > >> > > from adding the allowList option in future. Instead > if > > > we > > > > > > could > > > > > > > > > >> rephrase > > > > > > > > > >> > > the config as: "metric.verbosity.high" which takes > > > values > > > > > as a > > > > > > > > regEx > > > > > > > > > >> > > (default will be empty), then we wouldn't have to > > worry > > > > > about > > > > > > > > > >> > > future-proofness of this KIP. Notably this is an > > > existing > > > > > > > pattern > > > > > > > > > >> used by > > > > > > > > > >> > > KIP-544. > > > > > > > > > >> > > Alternatively, if you choose to stick to the current > > > > > > > configuration > > > > > > > > > >> > pattern, > > > > > > > > > >> > > please provide information on how this config will > > look > > > > like > > > > > > > when > > > > > > > > we > > > > > > > > > >> add > > > > > > > > > >> > > allow listing in future. > > > > > > > > > >> > > > > > > > > > > > >> > > About the perf test: > > > > > > > > > >> > > Motivation - The motivation of perf test is to > provide > > > > users > > > > > > > with > > > > > > > > a > > > > > > > > > >> hint > > > > > > > > > >> > on > > > > > > > > > >> > > what perf penalty they can expect and whether > default > > > has > > > > > > > degraded > > > > > > > > > >> perf > > > > > > > > > >> > > (due to additional "empty" labels). > > > > > > > > > >> > > Dimensions of the test could be - scrape interval, > > > > > utilization > > > > > > > of > > > > > > > > > >> broker > > > > > > > > > >> > > (no traffic vs. heavy traffic), number of partitions > > > > > > (small/200 > > > > > > > to > > > > > > > > > >> > > large/2k). > > > > > > > > > >> > > Things to collect during perf test - number of > mbeans > > > > > > registered > > > > > > > > > with > > > > > > > > > >> > JMX, > > > > > > > > > >> > > CPU, heap utilization > > > > > > > > > >> > > Expected results - As long as we can prove that > there > > is > > > > no > > > > > > > > > additional > > > > > > > > > >> > > usage (significant) of CPU or heap after this change > > for > > > > the > > > > > > > > > "default > > > > > > > > > >> > > mode", we should be good. For the "high" mode, we > > should > > > > > > > document > > > > > > > > > the > > > > > > > > > >> > > expected increase for users but it is not a blocker > to > > > > > > implement > > > > > > > > > this > > > > > > > > > >> > KIP. > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > *Kirk*, I have tried to clarify the expectation on > > > > > > performance, > > > > > > > > does > > > > > > > > > >> that > > > > > > > > > >> > > address your question earlier? Also, I am happy with > > > > having > > > > > a > > > > > > > > Kafka > > > > > > > > > >> level > > > > > > > > > >> > > dynamic config that we can use to filter our > > > > > > > metric/dimensionality > > > > > > > > > >> since > > > > > > > > > >> > we > > > > > > > > > >> > > have a precedence at KIP-544. Hence, my suggestion > to > > > push > > > > > > this > > > > > > > > > >> filtering > > > > > > > > > >> > > to metric library can be ignored. > > > > > > > > > >> > > > > > > > > > > > >> > > -- > > > > > > > > > >> > > Divij Vaidya > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > > >> > > On Sat, Oct 28, 2023 at 11:37 AM Qichao Chu > > > > > > > > <qic...@uber.com.invalid > > > > > > > > > > > > > > > > > > > >> > > wrote: > > > > > > > > > >> > > > > > > > > > > > >> > > > Hello Everyone, > > > > > > > > > >> > > > > > > > > > > > > >> > > > Can I ask for some feedback regarding KIP-977 > > > > > > > > > >> > > > < > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-977%3A+Partition-Level+Throughput+Metrics > > > > > > > > > >> > > > > > > > > > > > > > >> > > > ? > > > > > > > > > >> > > > > > > > > > > > > >> > > > Best, > > > > > > > > > >> > > > Qichao Chu > > > > > > > > > >> > > > Software Engineer | Data - Kafka > > > > > > > > > >> > > > [image: Uber] <https://uber.com/> > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > > >> > > > On Mon, Oct 16, 2023 at 7:34 PM Qichao Chu < > > > > > qic...@uber.com > > > > > > > > > > > > > > > > wrote: > > > > > > > > > >> > > > > > > > > > > > > >> > > > > Hi Divij and Kirk, > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > Thank you both for providing the valuable > feedback > > > and > > > > > > sorry > > > > > > > > for > > > > > > > > > >> the > > > > > > > > > >> > > > > delay. I have just updated the KIP to address > the > > > > > > comments. > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > 1. Instead of using a topic-level control, > > global > > > > > > > verbosity > > > > > > > > > >> > control > > > > > > > > > >> > > > > makes more sense if we want to extend it in > the > > > > > future. > > > > > > > It > > > > > > > > > >> would > > > > > > > > > >> > be > > > > > > > > > >> > > > very > > > > > > > > > >> > > > > difficult if we want to apply the topic > > allowlist > > > > > > > > everywhere > > > > > > > > > >> > > > > 2. Also, the topic allowlist was not dynamic > > > which > > > > > > makes > > > > > > > > > >> > everything > > > > > > > > > >> > > > > quite complex, especially for the topic > > lifecycle > > > > > > > > management. > > > > > > > > > >> By > > > > > > > > > >> > > > using the > > > > > > > > > >> > > > > dynamic global config, debugging could be > > easier, > > > > and > > > > > > > > > >> management > > > > > > > > > >> > of > > > > > > > > > >> > > > the > > > > > > > > > >> > > > > config is also made easier. > > > > > > > > > >> > > > > 3. More details are included in the test > > section. > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > One thing that still misses is the performance > > > > numbers. > > > > > I > > > > > > > will > > > > > > > > > >> get it > > > > > > > > > >> > > > > ready with our internal clusters and share out > > soon. > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > Many thanks for the review! > > > > > > > > > >> > > > > Qichao > > > > > > > > > >> > > > > > > > > > > > > > >> > > > > On Tue, Sep 12, 2023 at 8:31 AM Kirk True < > > > > > > > k...@kirktrue.pro> > > > > > > > > > >> wrote: > > > > > > > > > >> > > > > > > > > > > > > > >> > > > >> Oh, and does > > > metrics.partition.level.reporting.topics > > > > > > allow > > > > > > > > for > > > > > > > > > >> > regex? > > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > On Sep 12, 2023, at 8:26 AM, Kirk True < > > > > > > > k...@kirktrue.pro> > > > > > > > > > >> wrote: > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > Hi Qichao, > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > Thanks for the KIP! > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > Divij—questions/comments inline... > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> >> On Sep 11, 2023, at 4:32 AM, Divij Vaidya < > > > > > > > > > >> > divijvaidy...@gmail.com > > > > > > > > > >> > > > > > > > > > > > > >> > > > >> wrote: > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> Thank you for the proposal Qichao. > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> I agree with the motivation here and > > understand > > > > the > > > > > > > > tradeoff > > > > > > > > > >> here > > > > > > > > > >> > > > >> >> between observability vs. increased metric > > > > > dimensions > > > > > > > > > (metric > > > > > > > > > >> > > fan-out > > > > > > > > > >> > > > >> >> as you say in the KIP). > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> High level comments: > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> 1. I would urge you to consider the > > > extensibility > > > > of > > > > > > the > > > > > > > > > >> proposal > > > > > > > > > >> > > for > > > > > > > > > >> > > > >> >> other types of metrics. Tomorrow, if we want > > to > > > > > > > > selectively > > > > > > > > > >> add > > > > > > > > > >> > > > >> >> "partition" dimension to another metric, > would > > > we > > > > > have > > > > > > > to > > > > > > > > > >> modify > > > > > > > > > >> > > the > > > > > > > > > >> > > > >> >> code where each metric is emitted? > > > Alternatively, > > > > > > could > > > > > > > we > > > > > > > > > >> > abstract > > > > > > > > > >> > > > >> >> out this config in a "Kafka Metrics" > library. > > > The > > > > > code > > > > > > > > > >> provides > > > > > > > > > >> > all > > > > > > > > > >> > > > >> >> information about this library and this > > library > > > > can > > > > > > > choose > > > > > > > > > >> which > > > > > > > > > >> > > > >> >> dimensions it wants to add to the final > > metrics > > > > that > > > > > > are > > > > > > > > > >> emitted > > > > > > > > > >> > > > based > > > > > > > > > >> > > > >> >> on declarative configuration. > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > I’d agree with this if it doesn’t place a > > burden > > > on > > > > > the > > > > > > > > > >> callers. > > > > > > > > > >> > Are > > > > > > > > > >> > > > >> there any potential call sites that don’t have > > the > > > > > > > partition > > > > > > > > > >> > > information > > > > > > > > > >> > > > >> readily available? > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> >> 2. Can we offload the handling of this > > dimension > > > > > > > filtering > > > > > > > > > to > > > > > > > > > >> the > > > > > > > > > >> > > > >> >> metric framework? Have you explored whether > > > > > prometheus > > > > > > > or > > > > > > > > > >> other > > > > > > > > > >> > > > >> >> libraries provide the ability to dynamically > > > > change > > > > > > > > > dimensions > > > > > > > > > >> > > > >> >> associated with metrics? > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > I’m not familiar with the downstream metrics > > > > > providers’ > > > > > > > > > >> > > capabilities. > > > > > > > > > >> > > > >> This is a greatest common denominator scenario, > > > > right? > > > > > > We’d > > > > > > > > > have > > > > > > > > > >> to > > > > > > > > > >> > be > > > > > > > > > >> > > > >> reasonable sure that the heavily used providers > > > *all* > > > > > > > support > > > > > > > > > >> such > > > > > > > > > >> > > > dynamic > > > > > > > > > >> > > > >> filtering. > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > Also—and correct me as needed as I’m not > > familiar > > > > > with > > > > > > > the > > > > > > > > > >> area—if > > > > > > > > > >> > > we > > > > > > > > > >> > > > >> relegate partition filtering to a lower layer, > > we’d > > > > > still > > > > > > > > need > > > > > > > > > to > > > > > > > > > >> > > store > > > > > > > > > >> > > > the > > > > > > > > > >> > > > >> metric data in memory until it’s flushed, yes? > If > > > so, > > > > > is > > > > > > > that > > > > > > > > > >> > overhead > > > > > > > > > >> > > > of > > > > > > > > > >> > > > >> any concern? > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> >> Implementation level comments: > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> 1. In the test plan section, please mention > > what > > > > > kind > > > > > > of > > > > > > > > > integ > > > > > > > > > >> > > and/or > > > > > > > > > >> > > > >> >> unit tests will be added and what they will > > > > assert. > > > > > As > > > > > > > an > > > > > > > > > >> > example, > > > > > > > > > >> > > > you > > > > > > > > > >> > > > >> >> can add a section, "functionality tests", > > which > > > > > would > > > > > > > > assert > > > > > > > > > >> that > > > > > > > > > >> > > new > > > > > > > > > >> > > > >> >> metric config is being respected and another > > > > > section, > > > > > > > > > >> > "performance > > > > > > > > > >> > > > >> >> tests", which could be a system test and > > assert > > > > that > > > > > > no > > > > > > > > > >> > regression > > > > > > > > > >> > > > >> >> caused wrt resources occupied by metrics > from > > > one > > > > > > > version > > > > > > > > to > > > > > > > > > >> > > another. > > > > > > > > > >> > > > >> >> 2. Please mention why or why not are we > > > > considering > > > > > > > > > >> dynamically > > > > > > > > > >> > > > >> >> setting the configuration (i.e. without a > > broker > > > > > > > > restart)? I > > > > > > > > > >> > would > > > > > > > > > >> > > > >> >> imagine that the ability to dynamically > > > configure > > > > > for > > > > > > a > > > > > > > > > >> specific > > > > > > > > > >> > > > topic > > > > > > > > > >> > > > >> >> will be very useful especially to debug > > > production > > > > > > > > > situations > > > > > > > > > >> > that > > > > > > > > > >> > > > you > > > > > > > > > >> > > > >> >> mention in the motivation. > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > +1 > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> >> 3. You mention that we want to start with > > > metrics > > > > > > > closely > > > > > > > > > >> related > > > > > > > > > >> > > to > > > > > > > > > >> > > > >> >> producer & consumers first, which is fair. > > Could > > > > you > > > > > > > > please > > > > > > > > > >> add a > > > > > > > > > >> > > > >> >> statement on the work required to extend > this > > to > > > > > other > > > > > > > > > >> metrics in > > > > > > > > > >> > > > >> >> future? > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > +1 > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> >> 4. In the compatibility section, you mention > > > that > > > > > this > > > > > > > > > change > > > > > > > > > >> is > > > > > > > > > >> > > > >> >> backward compatible. I don't fully > understand > > > > that. > > > > > > > > During a > > > > > > > > > >> > > version > > > > > > > > > >> > > > >> >> upgrade, we will start with an empty list of > > > > topics > > > > > to > > > > > > > > > >> maintain > > > > > > > > > >> > > > >> >> backward compatibility. I assume after the > > > > upgrade, > > > > > we > > > > > > > > will > > > > > > > > > >> > update > > > > > > > > > >> > > > the > > > > > > > > > >> > > > >> >> new config with topic names that we desire > to > > > > > monitor. > > > > > > > But > > > > > > > > > >> > updating > > > > > > > > > >> > > > >> >> the config will require a broker restart (a > > > > rolling > > > > > > > > restart > > > > > > > > > >> since > > > > > > > > > >> > > > >> >> config is read-only). We will be in a > > situation > > > > > where > > > > > > > some > > > > > > > > > >> > brokers > > > > > > > > > >> > > > are > > > > > > > > > >> > > > >> >> sending metrics with a new "partition" > > dimension > > > > and > > > > > > > some > > > > > > > > > >> brokers > > > > > > > > > >> > > are > > > > > > > > > >> > > > >> >> sending metrics with no partition dimension. > > Is > > > > that > > > > > > > > > >> acceptable > > > > > > > > > >> > to > > > > > > > > > >> > > > JMX > > > > > > > > > >> > > > >> >> / prometheus collectors? Would it break > them? > > > > Please > > > > > > > > clarify > > > > > > > > > >> how > > > > > > > > > >> > > > >> >> upgrades will work in the compatibility > > section. > > > > > > > > > >> > > > >> >> 5. Could you please quantify (with an > > > experiment) > > > > > the > > > > > > > > > expected > > > > > > > > > >> > perf > > > > > > > > > >> > > > >> >> impact of adding the partition dimension? > This > > > > could > > > > > > be > > > > > > > > done > > > > > > > > > >> as > > > > > > > > > >> > > part > > > > > > > > > >> > > > >> >> of "test plan" section and would serve as a > > data > > > > > point > > > > > > > for > > > > > > > > > >> users > > > > > > > > > >> > to > > > > > > > > > >> > > > >> >> understand the potential impact if they > decide > > > to > > > > > turn > > > > > > > it > > > > > > > > > on. > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > Is there some guidance on the level of > > precision > > > > and > > > > > > > detail > > > > > > > > > >> > expected > > > > > > > > > >> > > > >> when providing the performance numbers in the > > KIP? > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > This notion of proving out the performance > > impact > > > > is > > > > > > > > > >> important, I > > > > > > > > > >> > > > >> agree. Anecdotally, there was another KIP I was > > > > > following > > > > > > > for > > > > > > > > > >> which > > > > > > > > > >> > > > >> performance numbers were requested, as is > > > reasonable. > > > > > But > > > > > > > > that > > > > > > > > > >> > caused > > > > > > > > > >> > > > the > > > > > > > > > >> > > > >> KIP to go a bit sideways as a result because it > > > > wasn’t > > > > > > able > > > > > > > > to > > > > > > > > > >> get > > > > > > > > > >> > > > >> consensus on a) the different scenarios to > test, > > > and > > > > b) > > > > > > the > > > > > > > > > >> > > quantitative > > > > > > > > > >> > > > >> goal for each. I’m not really sure the rigo(u)r > > > > that’s > > > > > > > > expected > > > > > > > > > >> at > > > > > > > > > >> > > this > > > > > > > > > >> > > > >> stage in the development of a new feature. > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> > Thanks, > > > > > > > > > >> > > > >> > Kirk > > > > > > > > > >> > > > >> > > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> -- > > > > > > > > > >> > > > >> >> Divij Vaidya > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> > > > > > > > > > >> > > > >> >> On Sat, Sep 9, 2023 at 8:18 PM Qichao Chu > > > > > > > > > >> > <qic...@uber.com.invalid > > > > > > > > > >> > > > > > > > > > > > > >> > > > >> wrote: > > > > > > > > > >> > > > >> >>> > > > > > > > > > >> > > > >> >>> Hi All, > > > > > > > > > >> > > > >> >>> > > > > > > > > > >> > > > >> >>> Although this has been discussed many > times, > > I > > > > > would > > > > > > > like > > > > > > > > > to > > > > > > > > > >> > > start a > > > > > > > > > >> > > > >> new > > > > > > > > > >> > > > >> >>> discussion regarding the introduction of > > > > > > > partition-level > > > > > > > > > >> > > throughput > > > > > > > > > >> > > > >> >>> metrics. Please review the KIP and I'm > eager > > to > > > > > know > > > > > > > > > >> everyone's > > > > > > > > > >> > > > >> thoughts: > > > > > > > > > >> > > > >> >>> > > > > > > > > > >> > > > >> > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-977%3A+Partition-Level+Throughput+Metrics > > > > > > > > > >> > > > >> >>> > > > > > > > > > >> > > > >> >>> TL;DR: The KIP proposes to add > > partition-level > > > > > > > throughput > > > > > > > > > >> > metrics > > > > > > > > > >> > > > and > > > > > > > > > >> > > > >> a new > > > > > > > > > >> > > > >> >>> configuration to control the fan-out rate. > > > > > > > > > >> > > > >> >>> > > > > > > > > > >> > > > >> >>> Thank you all for the review and have a > nice > > > > > weekend! > > > > > > > > > >> > > > >> >>> > > > > > > > > > >> > > > >> >>> Best, > > > > > > > > > >> > > > >> >>> Qichao > > > > > > > > > >> > > > >> > > > > > > > > > >> > > > >> > > > > > > > > > >> > > > > > > > > > > > > >> > > > > > > > > > > > >> > > > > > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
