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