Hi Bruno, Thanks for the comments! Here are my responses:
BC1: Yes, I agree the name could use some work. My original intent was that the user is adding additional metrics to the metrics already reported by the given client. But the name you provided here works, so I'll update the KIP accordingly. BC2: This KIP will leverage the naming rules and transformation specified in KIP-714 <https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability#KIP714:Clientmetricsandobservability-Naming>. I'll add a section on naming to the KIP; while I won't repeat the text from KIP-714, I'll provide a link and a table with a couple of examples of how Kafka Streams metric names will be transformed to OpenTelemetry-compatible names. On Tue, Aug 20, 2024 at 5: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 > >> > > >
