Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Jun Rao]

Hi, Apoorv,

Thanks for the reply. So it sounds like we could only take further actions
on Measurable. Then the new API makes sense.

Jun

On Wed, Feb 21, 2024 at 4:24 AM Apoorv Mittal 
wrote:

> Hi Jun,
> Thanks for reviewing.
>
> KafkaMetrics uses 2 types of MetricValueProvider as of now i.e. Measurable
> and Gauge. Gauge metrics are generally implemented as anonymous inner
> classes but Measurable has different implementations. KIP-714 (
> KafkaMetricsCollector
> <
> https://github.com/apache/kafka/blob/5cfcc52fb3fce4a43ca77df311382d7a02a40ed2/clients/src/main/java/org/apache/kafka/common/telemetry/internals/KafkaMetricsCollector.java#L218
> >)
> and other collectors (OpenTelemetry Kafka Client
> <
> https://github.com/open-telemetry/opentelemetry-java-instrumentation/blob/7a044f576fc06e041e07b4315a9afb0e1d081d4b/instrumentation/kafka/kafka-clients/kafka-clients-common/library/src/main/java/io/opentelemetry/instrumentation/kafka/internal/KafkaMetricRegistry.java#L36
> >)
> needs to differentiate on type of Measurable to determine the metric types
> (i.e. Counter, Gauge, etc.) for the OpenTelemetry. Though that
> differentiation can happen with already exposed functionality by
> KafkaMetric but that approach is not straightforward and efficient hence
> this KIP adds another method.
>
> As the differentiation is only required for the Measurable value provider
> hence went ahead to expose the measurability check in KafkaMetric. I do not
> think that we should require isGauge check as there doesn't seem to be any
> need for determining Gauge types for external metrics data models. I did
> check the current implementations for Percentiles and Frequencies which
> also emits metrics over Measurable itself. Though I do understand that enum
> might be helpful to translate what metric type the value provider is but as
> we only have support
> <
> https://github.com/apache/kafka/blob/5cfcc52fb3fce4a43ca77df311382d7a02a40ed2/clients/src/main/java/org/apache/kafka/common/metrics/KafkaMetric.java#L36
> >
> for
> Measurable and Gauge hence went ahead only exposing what is currently
> needed.
>
> Regards,
> Apoorv Mittal
> +44 7721681581
>
>
> On Tue, Feb 20, 2024 at 10:58 PM Jun Rao  wrote:
>
> > Hi, Apoorv,
> >
> > Thanks for the KIP.
> >
> > Could we document how we plan to use the new isMeasurable() method? For
> > example, gauge is another type of metric. Do we plan to add an isGauge()
> > method too? If so, is it better to add a separate method for each metric
> > type or is it better to have a single method like metricType() that
> returns
> > an enum?
> >
> > Jun
> >
> >
> > On Wed, Feb 14, 2024 at 6:56 AM Apoorv Mittal 
> > wrote:
> >
> > > Hi,
> > > I would like to start discussion of a small KIP which fills a gap in
> > > determining Kafka Metric measurability.
> > >
> > > KIP-1019: Expose method to determine Metric Measurability
> > > <
> > >
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1019%3A+Expose+method+to+determine+Metric+Measurability
> > > >
> > >
> > > Regards,
> > > Apoorv Mittal
> > >
> >
>

Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Apoorv Mittal]

Hi Jun,
Thanks for reviewing.

KafkaMetrics uses 2 types of MetricValueProvider as of now i.e. Measurable
and Gauge. Gauge metrics are generally implemented as anonymous inner
classes but Measurable has different implementations. KIP-714 (
KafkaMetricsCollector
)
and other collectors (OpenTelemetry Kafka Client
)
needs to differentiate on type of Measurable to determine the metric types
(i.e. Counter, Gauge, etc.) for the OpenTelemetry. Though that
differentiation can happen with already exposed functionality by
KafkaMetric but that approach is not straightforward and efficient hence
this KIP adds another method.

As the differentiation is only required for the Measurable value provider
hence went ahead to expose the measurability check in KafkaMetric. I do not
think that we should require isGauge check as there doesn't seem to be any
need for determining Gauge types for external metrics data models. I did
check the current implementations for Percentiles and Frequencies which
also emits metrics over Measurable itself. Though I do understand that enum
might be helpful to translate what metric type the value provider is but as
we only have support

for
Measurable and Gauge hence went ahead only exposing what is currently
needed.

Regards,
Apoorv Mittal
+44 7721681581


On Tue, Feb 20, 2024 at 10:58 PM Jun Rao  wrote:

> Hi, Apoorv,
>
> Thanks for the KIP.
>
> Could we document how we plan to use the new isMeasurable() method? For
> example, gauge is another type of metric. Do we plan to add an isGauge()
> method too? If so, is it better to add a separate method for each metric
> type or is it better to have a single method like metricType() that returns
> an enum?
>
> Jun
>
>
> On Wed, Feb 14, 2024 at 6:56 AM Apoorv Mittal 
> wrote:
>
> > Hi,
> > I would like to start discussion of a small KIP which fills a gap in
> > determining Kafka Metric measurability.
> >
> > KIP-1019: Expose method to determine Metric Measurability
> > <
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1019%3A+Expose+method+to+determine+Metric+Measurability
> > >
> >
> > Regards,
> > Apoorv Mittal
> >
>

Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Jun Rao]

Hi, Apoorv,

Thanks for the KIP.

Could we document how we plan to use the new isMeasurable() method? For
example, gauge is another type of metric. Do we plan to add an isGauge()
method too? If so, is it better to add a separate method for each metric
type or is it better to have a single method like metricType() that returns
an enum?

Jun


On Wed, Feb 14, 2024 at 6:56 AM Apoorv Mittal 
wrote:

> Hi,
> I would like to start discussion of a small KIP which fills a gap in
> determining Kafka Metric measurability.
>
> KIP-1019: Expose method to determine Metric Measurability
> <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1019%3A+Expose+method+to+determine+Metric+Measurability
> >
>
> Regards,
> Apoorv Mittal
>

Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Matthias J. Sax]

Thanks for the KIP. Seems there is not much we need to discuss about it. 
Feel free to start a VOTE.


-Matthias

On 2/15/24 7:24 AM, Manikumar wrote:

LGTM, Thanks for the KIP.

On Thu, Feb 15, 2024 at 8:50 PM Doğuşcan Namal 
wrote:


LGTM thanks for the KIP.

+1(non-binding)

On Wed, 14 Feb 2024 at 15:22, Andrew Schofield <
andrew_schofield_j...@outlook.com> wrote:


Hi Apoorv,
Thanks for the KIP. Looks like a useful change to tidy up the metrics

code.


Thanks,
Andrew


On 14 Feb 2024, at 14:55, Apoorv Mittal 

wrote:


Hi,
I would like to start discussion of a small KIP which fills a gap in
determining Kafka Metric measurability.

KIP-1019: Expose method to determine Metric Measurability
<



https://cwiki.apache.org/confluence/display/KAFKA/KIP-1019%3A+Expose+method+to+determine+Metric+Measurability



Regards,
Apoorv Mittal

Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Manikumar]

LGTM, Thanks for the KIP.

On Thu, Feb 15, 2024 at 8:50 PM Doğuşcan Namal 
wrote:

> LGTM thanks for the KIP.
>
> +1(non-binding)
>
> On Wed, 14 Feb 2024 at 15:22, Andrew Schofield <
> andrew_schofield_j...@outlook.com> wrote:
>
> > Hi Apoorv,
> > Thanks for the KIP. Looks like a useful change to tidy up the metrics
> code.
> >
> > Thanks,
> > Andrew
> >
> > > On 14 Feb 2024, at 14:55, Apoorv Mittal 
> > wrote:
> > >
> > > Hi,
> > > I would like to start discussion of a small KIP which fills a gap in
> > > determining Kafka Metric measurability.
> > >
> > > KIP-1019: Expose method to determine Metric Measurability
> > > <
> >
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1019%3A+Expose+method+to+determine+Metric+Measurability
> > >
> > >
> > > Regards,
> > > Apoorv Mittal
> >
> >
>

Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Doğuşcan Namal]

LGTM thanks for the KIP.

+1(non-binding)

On Wed, 14 Feb 2024 at 15:22, Andrew Schofield <
andrew_schofield_j...@outlook.com> wrote:

> Hi Apoorv,
> Thanks for the KIP. Looks like a useful change to tidy up the metrics code.
>
> Thanks,
> Andrew
>
> > On 14 Feb 2024, at 14:55, Apoorv Mittal 
> wrote:
> >
> > Hi,
> > I would like to start discussion of a small KIP which fills a gap in
> > determining Kafka Metric measurability.
> >
> > KIP-1019: Expose method to determine Metric Measurability
> > <
> https://cwiki.apache.org/confluence/display/KAFKA/KIP-1019%3A+Expose+method+to+determine+Metric+Measurability
> >
> >
> > Regards,
> > Apoorv Mittal
>
>

Re: [Discuss] KIP-1019: Expose method to determine Metric Measurability

[Andrew Schofield]

Hi Apoorv,
Thanks for the KIP. Looks like a useful change to tidy up the metrics code.

Thanks,
Andrew

> On 14 Feb 2024, at 14:55, Apoorv Mittal  wrote:
>
> Hi,
> I would like to start discussion of a small KIP which fills a gap in
> determining Kafka Metric measurability.
>
> KIP-1019: Expose method to determine Metric Measurability
> 
>
> Regards,
> Apoorv Mittal