Hi Kirk,

The push interval value passed in the new context will be the interval
configured in the metrics subscription via the interval.ms field.

For example if the administrator sets the following subscription:
bin/kafka-client-metrics.sh --bootstrap-server $BROKERS --alter
--generate-name \
  --metrics 
org.apache.kafka.producer.node.request.latency.,org.apache.kafka.consumer.node.request.latency.
\
  --interval 60000
then the pushIntervalMs() method will return 60000 when handling
metrics that match this subscription.

I'm not sure I fully understand what you mean, but I wonder if you're
commenting on the fact that clients are due to send metrics within the
(interval x 0.5) and (interval x 1.5) window.
If so do you think a note in the javadoc would help? or would you
rather advertise the upper bound to (interval x 1.5) to avoid reporter
implementations clearing client metrics too early?

Thanks,
Mickael

On Tue, Sep 30, 2025 at 2:28 AM Kirk True <[email protected]> wrote:
>
> Hi Mickael,
>
> Thanks for the KIP.
>
> IIUC, the pushIntervalMs value passed via the context to exportMetrics() is 
> the _current_ push interval, right? OTOH, the payload parameter represents 
> the data collected in the _previous_ push interval, right?
>
> I'm assuming this won't matter in practice, but I wanted to highlight it.
>
> Thanks,
> Kirk
>
> On Mon, Sep 29, 2025, at 10:14 AM, Mickael Maison wrote:
> > Hi Chia-Ping,
> >
> > chia_00: While it initially looks like a simpler option, it turns out
> > the exposed API isn't nice.
> > This was my original approach, hence why Andrew mentions an earlier
> > version, but after implementing it I really disliked the user
> > experience.
> > With this alternative if you want to implement the new method you
> > would still need to implement the older method as it does not have a
> > default in the ClientTelemetryReceiver interface. Not only this is
> > counter intuitive but it also means that both older and newer
> > implementations would need to change if we ever remove the older
> > method. With the proposed API, new implementations can directly
> > implement the new interface and won't need changing in the future.
> > Obviously existing implementations wanting to migrate will require
> > code changes and thus recompiling.
> >
> > Thanks,
> > Mickael
> >
> > On Mon, Sep 29, 2025 at 6:13 PM Chia-Ping Tsai <[email protected]> wrote:
> > >
> > > hi Mickael
> > >
> > > Apologies for the delayed review. I have a quick question.
> > >
> > > chia_00: The rejected alternative is a simpler approach, and the need for 
> > > recompilation is an acceptable drawback. The method is slated for removal 
> > > in a major release, which requires recompilation anyway I think.
> > >
> > > Best,
> > > Chia-Ping
> > >
> > > On 2025/09/24 15:56:03 Mickael Maison wrote:
> > > > Hi,
> > > >
> > > > Thanks for the feedback.
> > > > Since it's a relatively small KIP, if I don't see any more feedback in
> > > > the next few days, I'll start a vote.
> > > >
> > > > Thanks,
> > > > Mickael
> > > >
> > > > On Wed, Sep 17, 2025 at 5:44 AM Luke Chen <[email protected]> wrote:
> > > > >
> > > > > Hi Mickael,
> > > > >
> > > > > From the KIP-714, we annotated the ClientTelemetry and 
> > > > > ClientTelemetryReceiver
> > > > > as `@InterfaceStability.Evolving`.
> > > > > I was thinking that we are safe to break the compatibility in the next
> > > > > minor release, but it is removed earlier in this PR:
> > > > > https://github.com/apache/kafka/pull/19036...
> > > > >
> > > > > So, I'm +1 on this.
> > > > >
> > > > > Thanks.
> > > > > Luke
> > > > >
> > > > > On Wed, Sep 10, 2025 at 11:44 PM Mickael Maison 
> > > > > <[email protected]>
> > > > > wrote:
> > > > >
> > > > > > Hi Andrew,
> > > > > >
> > > > > > Thanks for taking a look!
> > > > > > Yeah after experimenting with the approach now listed in the 
> > > > > > rejected
> > > > > > alternatives, I thought it was better to just bring whole new
> > > > > > interfaces.
> > > > > >
> > > > > > AS1: I was following the naming from ClientTelemetry, but I agree 
> > > > > > it's
> > > > > > probably simpler to name that method clientTelemetryExporter().
> > > > > > I've updated the KIP.
> > > > > >
> > > > > > Thanks,
> > > > > > Mickael
> > > > > >
> > > > > > On Wed, Sep 10, 2025 at 4:26 PM Andrew Schofield
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi Mickael,
> > > > > > > Thanks for the KIP. It looks like a nice improvement.
> > > > > > >
> > > > > > > I previously read an earlier version and had some comments, but
> > > > > > > you've already addressed them and added to the rejected 
> > > > > > > alternatives :)
> > > > > > > Just one additional comment.
> > > > > > >
> > > > > > > AS1: I suggest the method in the ClientTelemetryExporterProvider
> > > > > > > interface would be better named clientTelemetryExporter.
> > > > > > >
> > > > > > > Thanks,
> > > > > > > Andrew
> > > > > > >
> > > > > > > ________________________________________
> > > > > > > From: Mickael Maison <[email protected]>
> > > > > > > Sent: 09 September 2025 16:56
> > > > > > > To: dev <[email protected]>
> > > > > > > Subject: [DISCUSS] KIP-1217: Include push interval in
> > > > > > ClientTelemetryReceiver context
> > > > > > >
> > > > > > > Apologies, the KIP number is 1217:
> > > > > > > KIP-1217: Include push interval in ClientTelemetryReceiver 
> > > > > > > context:
> > > > > > > https://cwiki.apache.org/confluence/x/6QnxFg
> > > > > > >
> > > > > > > On Tue, Sep 9, 2025 at 5:52 PM Mickael Maison 
> > > > > > > <[email protected]>
> > > > > > wrote:
> > > > > > > >
> > > > > > > > Hi,
> > > > > > > >
> > > > > > > > I'd like to start a discussion on KIP-1127:
> > > > > > > > https://cwiki.apache.org/confluence/x/6QnxFg
> > > > > > > >
> > > > > > > > This small KIP proposes providing the push interval to
> > > > > > > > ClientTelemetryReceiver implementations to improve the 
> > > > > > > > management of
> > > > > > > > client telemetry metrics.
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Mickael
> > > > > >
> > > >
> >

Reply via email to