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 > >> > > > >> > >> > > > >> > >> > > > > >> > > > >> > > >> > > >
