Hi Chia-Ping, Kirk, Have I answered your questions? Do you have any other feedback?
Thanks, Mickael On Thu, Oct 2, 2025 at 12:14 PM Mickael Maison <[email protected]> wrote: > > Hi Kirk, > > The push interval I propose adding to the new context does not have an > impact on the content, values or interpretation of the telemetry data. > Its role is only to allow reporters to detect when a client stops > emitting metrics and its metrics have become stale. > > Thanks, > Mickael > > On Thu, Oct 2, 2025 at 1:53 AM Kirk True <[email protected]> wrote: > > > > Hi Mickael, > > > > My apologies for wording my thoughts poorly :( > > > > Imagine this sequence of events: > > > > 1. 15:01:21: The client retrieves the telemetry subscription with an > > interval of 600,000 ms > > 2. 15:08:07: The operator updates the subscription's push interval to > > 60,000 ms to more closely watch some metric some issue > > 3. 15:11:23: The client pushes the telemetry data to the server, given its > > 10 minute interval > > 4. 15:11:25: The broker handling the telemetry data executes > > exportMetrics() receives a push interval value of 60,000 ms (current > > setting) along with data collected over the last ~600,000 (historic setting) > > > > So there's a chance that there could be a significant difference between > > what the historic interval value vs. what the current interval value. > > > > But is this likely to cause problems? I'm suspecting not, given the nature > > of the data and the way it's structured. If the telemetry data embeds > > timestamps, then it's probably a moot point. > > > > Thanks, > > Kirk > > > > On Tue, Sep 30, 2025, at 6:09 AM, Mickael Maison wrote: > > > 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 > > > > > > > > > > > > > > > > > > > > > > > >
