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
