Thanks for commenting, Matthias. Why did you switch from passing in a `Collection` to a single metric?
With the changes in the implementation it became obvious that was the necessary approach. Since it seems the discussion has concluded, I'll start a vote now. Thanks to all for the comments and discussion. -Bill On Fri, Aug 30, 2024 at 10:31 PM Matthias J. Sax <mj...@apache.org> wrote: > Thanks Bill. The KIP reads great. Overall LGTM. > > Just one question out of curiosity. Why did you switch from passing in a > `Collection` to a single metric? > > > -Matthias > > > On 8/26/24 3:22 PM, Bill Bejeck wrote: > > All, > > > > I had originally planned to call for a vote, but during some early > > implementation work, I've discovered some changes are necessary. > > First, we need a reciprocal method to remove a metric from subscriptions, > > and I've updated the register method to take a single metric. > > > > Thanks in advance for taking another look at the updated KIP, and > hopefully > > we'll get to a vote this week. > > > > -Bill > > > > On Thu, Aug 22, 2024 at 3:18 PM Bill Bejeck <bbej...@gmail.com> wrote: > > > >> Matthias and Andrew, > >> > >> I've updated the KIP. I decided to not list the Kafka Stream metrics > >> supported as it would create a large amount of redundant information. > >> The types of supported metrics and behavior for unsupported metrics is > >> clearly defined. > >> > >> Thanks, > >> Bill > >> > >> On Thu, Aug 22, 2024 at 12:10 PM Bill Bejeck <bbej...@gmail.com> wrote: > >> > >>> Hi Mathias and Andrew, > >>> > >>> Thanks for taking the time to read and comment on the KIP. I'll > address > >>> each comment below: > >>> > >>> MS1 - I agree that logging a WARN here should be sufficient. I'll > update > >>> the interface in KIP to the original `void` return type. > >>> AS1 - I was considering throwing an exception, but you make a > compelling > >>> argument about the expected behavior and external dynamic factors. > >>> So as stated above, the KIP will specify a `void` return type. > >>> > >>> MS2 - Good point. I'll update the KIP with a brief statement on > supported > >>> types and a table of Kafka Streams metrics displaying what's supported > and > >>> which ones aren't. > >>> AS2 - I think the above addresses your second comment. > >>> > >>> MS3 - We could explore adding new metrics, but I wouldn't want that to > >>> block the progress of this KIP. > >>> So I'd prefer to defer the discussion of new metrics to a follow-on > KIP. > >>> Having said that, if you (or anyone else!) has an idea of > >>> a specific metric to add, we can consider it now. > >>> > >>> MS4 - Unsupported metrics are silently filtered out. Although it's an > >>> implementation detail, I propose to log a WARN statement for each > filtered > >>> metric passed to `registerMetricsForSubscription`. > >>> I also update the KIP with the behavior of unsupported metric types. > >>> > >>> MS5 - Setting `enable.metric.push = false` completely disables the > >>> telemetry pipeline and any provided Kafka Streams metrics are ignored > (I > >>> have this in the KIP). > >>> Although this is implementation detail, this is a valid condition to > >>> throw a `ConfigException` on Kafka Streams startup should this > >>> inconsistency in telemetry configs exist (at least in the main and > admin > >>> consumer). > >>> I'll update the KIP to be more clear on this point as well. > >>> > >>> Thanks, > >>> Bill > >>> > >>> On Thu, Aug 22, 2024 at 10:54 AM Andrew Schofield < > >>> andrew_schofi...@live.com> wrote: > >>> > >>>> Hi Bill (and Matthias), > >>>> Thanks for the KIP. This looks like a valuable extension to KIP-714. > >>>> > >>>> AS1: Personally, I think that registerMetricsForSubscription should > >>>> return > >>>> void and not throw an exception if metrics push is not enabled. > >>>> Otherwise, > >>>> you end up with an application whose behaviour is markedly different > >>>> depending > >>>> on external factors. For example, if you connect to a broker without a > >>>> plugin > >>>> that supports the ClientTelemetry interface, no metrics are going to > be > >>>> pushed. > >>>> Similarly, if the enable.metrics.push is false, no metrics are going > to > >>>> be pushed. > >>>> Neither of those seem to me to have implications on whether this > method > >>>> executes successfully. Even if everything is enabled, the metrics will > >>>> only flow > >>>> if there’s a telemetry subscription which matches the metric names. > >>>> Those are > >>>> dynamic things which are intended to be modified by operators seeking > to > >>>> diagnose problems. We certainly don’t want the > >>>> registerMetricsForSubscription > >>>> method to behave differently depending on the current state of its > >>>> telemetry > >>>> subscriptions. > >>>> > >>>> AS2: Good catch from Matthias for metrics whose type is incompatible > >>>> with OTLP. We need to define the behaviour here. > >>>> > >>>> Thanks, > >>>> Andrew > >>>> > >>>> > >>>>> On 22 Aug 2024, at 05:35, Matthias J. Sax <mj...@apache.org> wrote: > >>>>> > >>>>> Thanks for the KIP Bill. > >>>>> > >>>>> Just for brainstorming: the newly added methods have `boolean` return > >>>> type. Is this necessary or would logging a WARN be sufficient? Or > maybe > >>>> throw an exception if reporting is disabled? > >>>>> > >>>>> As we intend to report KS metrics, I am wondering if we should talk > >>>> about this in some more detail? As discussed offline, KIP-714 is > based on > >>>> open-telemetry and supports only certain metric types, and not all KS > >>>> metrics can be reported. > >>>>> > >>>>> Should we maybe even add new metrics to KS to close this gap (could > >>>> also be a follow up KIP). > >>>>> > >>>>> What happens if a registered metric is of an unsupported type? > >>>>> > >>>>> What happens if users set KS level `enable.metric.push = true` but a > >>>> client level config `enable.maetric.push = false`? Should we throw a > >>>> ConfigException or similar for this case (at least for admin and > >>>> main.consumer -- as we don't intent to use the producer, it would not > >>>> matter if producer metric push is enabled or not). To be fair, this > might > >>>> be more of an impl detail? Just curious. > >>>>> > >>>>> > >>>>> -Matthias > >>>>> > >>>>> > >>>>> On 8/21/24 1:03 PM, Apoorv Mittal wrote: > >>>>>> Hi Bill, > >>>>>> Thanks for addressing, looks good to me. Looking forward to > >>>> implementation. > >>>>>> Regards, > >>>>>> Apoorv Mittal > >>>>>> On Wed, Aug 21, 2024 at 8:14 PM Bill Bejeck <bbej...@gmail.com> > >>>> wrote: > >>>>>>> Hi Apoorv, > >>>>>>> > >>>>>>> Good call, I've updated the compatibility section with text about > >>>> using > >>>>>>> older brokers. > >>>>>>> > >>>>>>> -Bill > >>>>>>> > >>>>>>> On Wed, Aug 21, 2024 at 1:30 PM Apoorv Mittal < > >>>> apoorvmitta...@gmail.com> > >>>>>>> wrote: > >>>>>>> > >>>>>>>> Hi Bill, > >>>>>>>> Thanks for the details. Sounds good to me. > >>>>>>>> > >>>>>>>> One last question: > >>>>>>>> > >>>>>>>> AM5: Do we need to capture any details around compatibility with > >>>> older > >>>>>>>> brokers which don't support KIP-714 or when telemetry APIs are not > >>>>>>> enabled > >>>>>>>> on brokers (no configured receiver plugin)? > >>>>>>>> > >>>>>>>> Regards, > >>>>>>>> Apoorv Mittal > >>>>>>>> > >>>>>>>> > >>>>>>>> On Tue, Aug 20, 2024 at 10:22 PM Bill Bejeck <bbej...@apache.org> > >>>> wrote: > >>>>>>>> > >>>>>>>>> Hi Apoorv, > >>>>>>>>> > >>>>>>>>> Thank you for the discussion! Here are my responses to your > >>>> follow-up > >>>>>>>>> questions: > >>>>>>>>> > >>>>>>>>> AM2: My apologies for the previous response; I misunderstood the > >>>>>>>> question. > >>>>>>>>> When executing the `registerMetricsForSubscription,` it is up to > >>>> the > >>>>>>>>> developer to provide all metrics that could possibly be in a > >>>>>>> subscription > >>>>>>>>> request. For example, Kafka Streams will provide all stream > >>>> metrics, > >>>>>>> thus > >>>>>>>>> supporting any subscription changes at runtime. I'll clarify this > >>>> in > >>>>>>> the > >>>>>>>>> KIP. > >>>>>>>>> > >>>>>>>>> AM3: I understand your point, but I'm not convinced this will be > an > >>>>>>> issue > >>>>>>>>> right now. But given it is an implementation detail, I'd like > to > >>>>>>> defer > >>>>>>>>> addressing this issue in the PR phase, where we can thoroughly > >>>> test and > >>>>>>>>> adjust accordingly. Is that acceptable for you? > >>>>>>>>> > >>>>>>>>> Thanks, > >>>>>>>>> Bill > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> On Tue, Aug 20, 2024 at 3:34 PM Apoorv Mittal < > >>>>>>> apoorvmitta...@gmail.com> > >>>>>>>>> wrote: > >>>>>>>>> > >>>>>>>>>> Hi Bill, > >>>>>>>>>> Thanks for the answers, some follow ups. > >>>>>>>>>> > >>>>>>>>>> AM2: If we will only register subscribed metrics, as per the > >>>>>>>> subscription > >>>>>>>>>> received from the broker, then are we going to call the > >>>>>>>>>> `registerMetricsForSubscription` post to > GetTelemetrySubscription > >>>>>>>>> response > >>>>>>>>>> from the broker? Also how do we manage the dynamic update of > >>>>>>> subscribed > >>>>>>>>>> metrics with this approach i.e. subscribed metrics can change at > >>>>>>>> runtime > >>>>>>>>>> then are we going to re-register additional metrics from > external > >>>>>>>>>> applications? > >>>>>>>>>> > >>>>>>>>>> AM3: Sounds good. I understand that Streams JMXReporter will > emit > >>>>>>>> Streams > >>>>>>>>>> metrics and individual client JMXReporter will emit respective > >>>> client > >>>>>>>>>> metrics. But currently in the clients, all the metrics in mbean > >>>> are > >>>>>>>>>> evaluated by JMXReporter and KafkaMetricsCollector, though it's > an > >>>>>>>>>> implementation detail but I am not sure if we register external > >>>>>>>>> application > >>>>>>>>>> metrics in client, by registerMetricsForSubscription, then how > we > >>>>>>> will > >>>>>>>>>> differentiate with external application metrics while emitting > >>>>>>> through > >>>>>>>>>> JMXReporter of clients. > >>>>>>>>>> > >>>>>>>>>> Regards, > >>>>>>>>>> Apoorv Mittal > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> On Tue, Aug 20, 2024 at 8:09 PM Bill Bejeck <bbej...@apache.org > > > >>>>>>>> wrote: > >>>>>>>>>> > >>>>>>>>>>> Hi Apoorv, > >>>>>>>>>>> > >>>>>>>>>>> Thanks for reading the KIP and commenting. Here are my answers > >>>> to > >>>>>>>> your > >>>>>>>>>>> comments/questions: > >>>>>>>>>>> > >>>>>>>>>>> AM1: Good point; I'll update the Javadoc in the KIP to be more > >>>>>>>>> specific. > >>>>>>>>>>> > >>>>>>>>>>> AM2: I'm not sure I follow, but IIUC, then yes, this method > will > >>>>>>> only > >>>>>>>>>>> subscribe telemetry metrics. > >>>>>>>>>>> > >>>>>>>>>>> AM3: The `registerMetricsForSubscription` method only > subscribes > >>>>>>>>> metrics > >>>>>>>>>>> for telemetry reporting; it does not give them to the JMX > >>>> reporter. > >>>>>>>>> The > >>>>>>>>>>> application implementer is responsible for providing metrics to > >>>> any > >>>>>>>>> other > >>>>>>>>>>> reporter. For example, when starting an application, Kafka > >>>> Streams > >>>>>>>>>>> separately exposes all its metrics to a JMX reporter. > >>>>>>>>>>> > >>>>>>>>>>> AM4: Great question. I'll assert we still need the > >>>>>>>>> `enable.metrics.push` > >>>>>>>>>>> configuration to toggle pushing Kafka Streams telemetry > metrics. > >>>>>>> To > >>>>>>>>>> change > >>>>>>>>>>> the metrics of the embedded Kafka clients within Kafka Streams, > >>>>>>> users > >>>>>>>>>> will > >>>>>>>>>>> use the current mechanism to update client configuration > >>>> settings. > >>>>>>>>> I'll > >>>>>>>>>>> update the KIP to clarify these details. > >>>>>>>>>>> > >>>>>>>>>>> Thanks, > >>>>>>>>>>> Bill > >>>>>>>>>>> > >>>>>>>>>>> On Tue, Aug 20, 2024 at 6:30 AM Apoorv Mittal < > >>>>>>>>> apoorvmitta...@gmail.com> > >>>>>>>>>>> wrote: > >>>>>>>>>>> > >>>>>>>>>>>> Hi Bill, > >>>>>>>>>>>> Thanks for the KIP and it's great to see a way applications > can > >>>>>>>> also > >>>>>>>>>> send > >>>>>>>>>>>> telemetry metrics over the KIP-714 pipeline. > >>>>>>>>>>>> > >>>>>>>>>>>> A couple of questions: > >>>>>>>>>>>> AM1: The Javadoc for the method seems to be a bit vague, it > >>>> talks > >>>>>>>>> about > >>>>>>>>>>>> "subscription" but do we need to be a bit more detailed > >>>> regarding > >>>>>>>>> which > >>>>>>>>>>>> subscription we refer to? > >>>>>>>>>>>> > >>>>>>>>>>>> AM2: Are we going to only register KIP-714 subscribed metrics > >>>>>>> with > >>>>>>>>> this > >>>>>>>>>>>> method? > >>>>>>>>>>>> > >>>>>>>>>>>> AM3: Are the additional registered metrics available on JMX > >>>>>>>> reporter > >>>>>>>>> as > >>>>>>>>>>>> well or are we going to only register additional metrics for > >>>>>>>> KIP-714 > >>>>>>>>>>>> telemetry pipeline? > >>>>>>>>>>>> > >>>>>>>>>>>> AM4: StreamsConfig also defines `enable.metrics.push`, do we > >>>> need > >>>>>>>>> that > >>>>>>>>>>>> config or rely on client metric push config? Or all the > clients > >>>>>>>> will > >>>>>>>>>>>> inherit the `enable.metrics.push` defined for Streams? > >>>>>>>>>>>> > >>>>>>>>>>>> Regards, > >>>>>>>>>>>> Apoorv Mittal > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> On Tue, Aug 20, 2024 at 10:21 AM Bruno Cadonna < > >>>>>>> cado...@apache.org > >>>>>>>>> > >>>>>>>>>>> wrote: > >>>>>>>>>>>> > >>>>>>>>>>>>> Hi Bill, > >>>>>>>>>>>>> > >>>>>>>>>>>>> Thanks for the KIP! > >>>>>>>>>>>>> > >>>>>>>>>>>>> BC1 > >>>>>>>>>>>>> I find the name registerAdditionalMetrics() not specific > >>>>>>> enough. > >>>>>>>> It > >>>>>>>>>> is > >>>>>>>>>>>>> not clear that the passed-in metrics are made available for > >>>>>>>>>>>>> subscription. What about registerMetricsForSubscription()? > >>>>>>>>>>>>> The Additional part is confusing in my opinion: additional to > >>>>>>>> what? > >>>>>>>>>>>>> > >>>>>>>>>>>>> BC2 > >>>>>>>>>>>>> Does the KIP need a transformation rule from MetricName to > >>>>>>>>>>>>> OpenTelemetry-compatible names that are exposed at the broker > >>>>>>> for > >>>>>>>>>>>>> subscription and retrieval? > >>>>>>>>>>>>> > >>>>>>>>>>>>> Best, > >>>>>>>>>>>>> Bruno > >>>>>>>>>>>>> > >>>>>>>>>>>>> On 8/20/24 12:43 AM, Bill Bejeck wrote: > >>>>>>>>>>>>>> Hi Lucas, thanks for the comments. > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> LB1 > >>>>>>>>>>>>>> Yes, I'll update the KIP signature to `? extends Metric` > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> LB2 > >>>>>>>>>>>>>> If `enable.metrics.push` is disabled, the metrics passed in > >>>>>>> are > >>>>>>>>>>>> ignored. > >>>>>>>>>>>>>> Thinking about this now, I'm considering updating the return > >>>>>>>> type > >>>>>>>>>> of > >>>>>>>>>>>>>> `registerAdditionalMetrics` to a `boolean` from `void`, true > >>>>>>> if > >>>>>>>>> the > >>>>>>>>>>>>> metrics > >>>>>>>>>>>>>> have been successfully applied and false otherwise. > >>>>>>>>>>>>>> WDYT? > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> Cheers, > >>>>>>>>>>>>>> Bill > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>>> On Mon, Aug 12, 2024 at 11:33 AM Lucas Brutschy > >>>>>>>>>>>>>> <lbruts...@confluent.io.invalid> wrote: > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Hi Bill! > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Thanks for the KIP! I like this solution, it's general > >>>>>>> enough > >>>>>>>>> for > >>>>>>>>>>> many > >>>>>>>>>>>>>>> kinds of application. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> LB1: Could we accept an instance of the Metric interface > >>>>>>>> instead > >>>>>>>>>> of > >>>>>>>>>>>>>>> KafkaMetric? No strong reason to do it, it would just make > >>>>>>> the > >>>>>>>>>>>>>>> interface slightly more generic. > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> LB2: What is the behavior is `enable.metrics.push` is > >>>>>>>> disabled? > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> Cheers, > >>>>>>>>>>>>>>> Lucas > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> On Fri, Aug 2, 2024 at 9:30 PM Bill Bejeck < > >>>>>>>> bbej...@apache.org> > >>>>>>>>>>>> wrote: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Hi all, > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> I would like to start a discussion thread on KIP-1076 to > >>>>>>>> enable > >>>>>>>>>>>> metrics > >>>>>>>>>>>>>>>> collection for applications that embed a Kafka client. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> The KIP can be found here: > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>> > >>>>>>>> > >>>>>>> > >>>> > https://cwiki.apache.org/confluence/display/KAFKA/KIP-1076%3A++Metrics+for+client+applications+KIP-714+extension > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> I look forward to the discussion and your feedback. > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> Thanks, > >>>>>>>>>>>>>>>> Bill > >>>> > >>>> > >>>> > > >
