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