Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-13 Thread Andrew Schofield
Hi Jun, Thanks for the clarifications. 131. The client instance ids returned from KafkaStreams.clientInstanceIds(Duration) correspond to the client_instance_id labels added by the broker to the metrics pushed from the clients. This should be sufficient information to enable correlation between

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Matthias J. Sax
Thanks Andrew. Makes sense to me. Adding the parameter-less overload was just a random idea. No need to extend the KIP. -Matthias On 10/12/23 12:12 PM, Jun Rao wrote: Hi, Andrew, Thanks for the reply. 131. Could we also document how one could correlate each client instance in KStreams with

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Jun Rao
Hi, Andrew, Thanks for the reply. 131. Could we also document how one could correlate each client instance in KStreams with the labels for the metrics received by the brokers? 132. The documentation for RequestsPerSec is not complete. If you trace through how

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Andrew Schofield
Hi Matthias, I’ll answer (1) to (3). (1) The KIP uses the phrase “client instance id” and the method name mirrors that. Personally, I’m comfortable with the current name. (2) That’s a good point. I’ll update it to use a Kafka Uuid instead. (3) Although it’s a trivial thing to add an overload

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Andrew Schofield
Hi Jun, Thanks for your comments. 130. As Matthias described, and I am adding to the KIP, the `KafkaStreams#clientInstanceIds` method is only permitted when the state is RUNNING or REBALANCING. Also, clients can be added dynamically so the maps might change over time. If it’s in a permitted

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Matthias J. Sax
Seems both Andrew and Jun prefer to merge the consumers. I am ok with this. I'll leave it to Andrew to update the KIP accordingly, including adding `throws TimeoutException`. -Matthias On 10/12/23 10:07 AM, Jun Rao wrote: Hi, Matthias, 130. Yes, throwing an exception sounds reasonable. It

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Jun Rao
Hi, Matthias, 130. Yes, throwing an exception sounds reasonable. It would be useful to document this. 131. I was thinking that we could just return all consumers (including the global consumer) through Map consumerInstanceIds() and use keys to identify each consumer instance. The benefit is that

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Andrew Schofield
Hi Matthias, 131) I also think that separating the main and restore consumers is excessively specific about the current implementation. So, I think I’d prefer: public class ClientInstanceIds { String adminInstanceId(); Optional globalConsumerInstanceId(); Map consumerInstanceIds();

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Matthias J. Sax
Thanks Sophie and Jun. `clientInstanceIds()` is fine with me -- was not sure about the double plural myself. Sorry if my comments was confusing. I was trying to say, that adding a overload to `KafkaStreams` that does not take a timeout parameter does not make sense, because there is no

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Jun Rao
Hi, Matthias, Thanks for the reply. 130. What would be the semantic? If the timeout has expired and only some of the client instances' id have been retrieved, does the call return the partial result or throw an exception? 131. Could we group all consumer instances in a single method since we

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-12 Thread Sophie Blee-Goldman
Regarding the naming, I personally think `clientInstanceId` makes sense for the plain clients -- especially if we might later introduce the notion of an `applicationInstanceId`. I'm not a huge fan of `clientsInstanceIds` for the Kafka Streams API, though, can we use `clientInstanceIds` instead?

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-11 Thread Matthias J. Sax
In can answer 130 and 131. 130) We cannot guarantee that all clients are already initialized due to race conditions. We plan to not allow calling `KafkaStreams#clientsInstanceIds()` when the state is not RUNNING (or REBALANCING) though -- guess this slipped on the KIP and should be added?

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-11 Thread Jun Rao
Hi, Andrew, Thanks for the updated KIP. Just a few more minor comments. 130. KafkaStreams.clientsInstanceId(Duration timeout): Does it wait for all consumer/producer/adminClient instances to be initialized? Are all those instances created during KafkaStreams initialization? 131. Why does

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-11 Thread Matthias J. Sax
Thanks! On 10/10/23 11:31 PM, Andrew Schofield wrote: Matthias, Yes, I think that’s a sensible way forward and the interface you propose looks good. I’ll update the KIP accordingly. Thanks, Andrew On 10 Oct 2023, at 23:01, Matthias J. Sax wrote: Andrew, yes I would like to get this

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-11 Thread Andrew Schofield
Matthias, Yes, I think that’s a sensible way forward and the interface you propose looks good. I’ll update the KIP accordingly. Thanks, Andrew > On 10 Oct 2023, at 23:01, Matthias J. Sax wrote: > > Andrew, > > yes I would like to get this change into KIP-714 right way. Seems to be >

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-10 Thread Matthias J. Sax
Andrew, yes I would like to get this change into KIP-714 right way. Seems to be important, as we don't know if/when a follow-up KIP for Kafka Streams would land. I was also thinking (and discussed with a few others) how to expose it, and we would propose the following: We add a new method

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-09 Thread Andrew Schofield
Hi Matthias, Good point. Makes sense to me. Is this something that can also be included in the proposed Kafka Streams follow-on KIP, or would you prefer that I add it to KIP-714? I have a slight preference for the former to put all of the KS enhancements into a separate KIP. Thanks, Andrew >

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-06 Thread Matthias J. Sax
Thanks Andrew. SGTM. One point you did not address is the idea to add a method to `KafkaStreams` similar to the proposed `clientInstanceId()` that will be added to consumer/producer/admin clients. Without addressing this, Kafka Streams users won't have a way to get the assigned `instanceId`

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-06 Thread Andrew Schofield
Hi Matthias, Thanks for your comments. I agree that a follow-up KIP for Kafka Streams makes sense. This KIP currently has made a bit of an effort to embrace KS, but it’s not enough by a long way. I have removed `application.id `. This should be done properly in the

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-03 Thread Andrew Schofield
Hi David, Thanks for your interest in KIP-714. Because this KIP is under development at the same time as KIP-848, it will need to support both the existing KafkaConsumer code and the refactored code being worked on under KIP-848. I’ve updated the Threading section accordingly. Thanks, Andrew >

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-10-02 Thread Matthias J. Sax
Hi, I did not pay attention to this KIP in the past; seems it was on-hold for a while. Overall it sounds very useful, and I think we should extend this with a follow up KIP for Kafka Streams. What is unclear to me at this point is the statement: Kafka Streams applications have an

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-29 Thread David Jacot
Hi Andrew, Thanks for driving this one. I haven't read all the KIP yet but I already have an initial question. In the Threading section, it is written "KafkaConsumer: the "background" thread (based on the consumer threading refactor which is underway)". If I understand this correctly, it means

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-22 Thread Andrew Schofield
Hi Philip, No, I do not think it should actively search for a broker that supports the new RPCs. In general, either all of the brokers or none of the brokers will support it. In the window, where the cluster is being upgraded or client telemetry is being enabled, there might be a mixed situation.

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-22 Thread Philip Nee
Hi Andrew - Question on top of your answers: Do you think the client should actively search for a broker that supports this RPC? As previously mentioned, the broker uses the leastLoadedNode to find its first connection (am I correct?), and what if that broker doesn't support the metric push? P

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-22 Thread Andrew Schofield
Hi Kirk, Thanks for your question. You are correct that the presence or absence of the new RPCs in the ApiVersionsResponse tells the client whether to request the telemetry subscriptions and push metrics. This is of course tricky in practice. It would be conceivable, as a cluster is upgraded

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-22 Thread Kirk True
Hi Andrew/Jun, I want to make sure I understand question/comment #119… In the case where a cluster without a metrics client receiver is later reconfigured and restarted to include a metrics client receiver, do we want the client to thereafter begin pushing metrics to the cluster? From Andrew’s

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-21 Thread Andrew Schofield
Hi Jun, Thanks for your comments. I’ve updated the KIP to clarify where necessary. 110. Yes, agree. The motivation section mentions this. 111. The replacement of ‘-‘ with ‘.’ for metric names and the replacement of ‘-‘ with ‘_’ for attribute keys is following the OTLP guidelines. I think it’s a

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-09-20 Thread Jun Rao
Hi, Andrew, Thanks for updating the KIP and sorry for the late response. A few more comments. 110. Another potential motivation is the multiple clients support. Some of the places may not have good monitoring support for non-java clients. 111. OpenTelemetry Naming: We replace '-' with '.' for

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-08-25 Thread Andrew Schofield
Hi Tom, Thanks for your comments. Sorry for the delay in responding. They’re still useful comments in spite of the fact that the voting has begun. 1) This is a good idea. I expect the client will emit the client instance ID as a log line. 2) I will add PROXY protocol support to the future work.

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-08-11 Thread Tom Bentley
Hi Andrew, Thanks for picking this KIP up. I know you've started a vote, so these are unhelpfully late... sorry about that, but hopefully they're still useful. 1. "The Kafka Java client provides an API for the application to read the generated client instance id to assist in mapping and

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-08-11 Thread Andrew Schofield
Hi Doguscan, Thanks for your question. If the target broker is unreachable, the client can send the metrics to another broker. It can select any of the other brokers for this purpose. What I expect in practice is that it loses connection to the broker it’s being using for metrics, chooses or

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-08-08 Thread Doğuşcan Namal
Thanks for your answers Andrew. I share your pain that it took a while for you to get this KIP approved and you want to reduce the scope of it, will be happy to help you with the implementation :) Could you help me walk through what happens if the target broker is unreachable? Is the client going

RE: [DISCUSS] KIP-714: Client metrics and observability

2023-08-04 Thread Doğuşcan Namal
Hi Andrew, thanks a lot for this KIP. I was thinking of something similar so thanks for writing this down  Couple of questions related to the design: 1. Can we investigate the option for using the Kraft controllers instead of the brokers for sending metrics? The disadvantage of sending

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-08-04 Thread Andrew Schofield
Hi Doguscan, Thanks for your comments. I’m glad to hear you’re interested in this KIP. 1) It is preferred that a client sends its metrics to the same broker connection but actually it is able to send them to any broker. As a result, if a broker becomes unhealthy, the client can push its metrics

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-07-31 Thread Andrew Schofield
Hi Milind, Thanks for your question. On reflection, I agree that INVALID_RECORD is most likely to be caused by a problem in the serialization in the client. I have changed the client action in this case to “Log an error and stop pushing metrics”. I have updated the KIP text accordingly.

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-07-31 Thread Milind Luthra
Hi Andrew, Thanks for the clarifications. About 2b: In case a client has a bug while serializing, it might be difficult for the client to recover from that without code changes. In that, it might be good to just log the INVALID_RECORD as an error, and treat the error as fatal for the client (only

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-07-24 Thread Andrew Schofield
Hi Milind, Thanks for your questions about the KIP. 1) I did some archaeology and looked at historical versions of the KIP. I think this is just a mistake. 5 minutes is the default metric push interval. 30 minutes is a mystery to me. I’ve updated the KIP. 2) I think there are two situations in

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-07-17 Thread Milind Luthra
Hi Andrew, thanks for this KIP. I had a few questions regarding the "Error handling" section. 1. It mentions that "The 5 and 30 minute retries are to eventually trigger a retry and avoid having to restart clients if the cluster metrics configuration is disabled temporarily, e.g., by operator

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-06-26 Thread Andrew Schofield
Hi Kirk, Thanks for your comments. 1) I agree that this KIP is not aiming to create a new first-class construct of a unique, managed, per-client instance ID. I’ll add some clarifying words. 2) I can see what the KIP is trying to say about the Terminating flag, but it doesn’t quite do it for

Re: [DISCUSS] KIP-714: Client metrics and observability

2023-06-20 Thread Kirk True
Hi Andrew, > On Jun 13, 2023, at 8:06 AM, Andrew Schofield > wrote: > > Hi, > I would like to start a new discussion thread on KIP-714: Client metrics and > observability. > > https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability > > I have edited

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-23 Thread Kirk True
Hi Jun, On Tue, Jun 21, 2022, at 5:24 PM, Jun Rao wrote: > Hi, Magnus, Kirk, > > Thanks for the reply. A few more comments on your reply. > > 100. I agree there are some benefits of having a set of standard metrics > across all clients, but I am just wondering how practical it is, given that >

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-21 Thread Jun Rao
Hi, Magnus, Kirk, Thanks for the reply. A few more comments on your reply. 100. I agree there are some benefits of having a set of standard metrics across all clients, but I am just wondering how practical it is, given that the proposal doesn't require this set like the Kafka protocol. 100.1 A

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-21 Thread Kirk True
Hi Jun, Thank you for all your continued interest in shaping the KIP :) On Thu, Jun 16, 2022, at 2:38 PM, Jun Rao wrote: > Hi, Kirk, > > Thanks for the reply. A couple of more comments. > > (1) "Another perspective is that these two sets of metrics serve different > purposes and/or have

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-21 Thread Magnus Edenhill
Hey Jun and Kirk, I see that there's a lot of focus on the existing metrics in the Java clients, which makes sense, but the KIP aims to approach the problem space from a higher and more generic level by defining: 1) a standard protocol for subscribing to, and pushing metrics, 2) an existing

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-16 Thread Jun Rao
Hi, Kirk, Thanks for the reply. A couple of more comments. (1) "Another perspective is that these two sets of metrics serve different purposes and/or have different audiences, which argues that they should maintain their individuality and purpose. " Hmm, I am wondering if those metrics are

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-16 Thread Kirk True
Hi Jun, I'll try to answer the questions posed... On Tue, Jun 7, 2022, at 4:32 PM, Jun Rao wrote: > Hi, Magnus, > > Thanks for the reply. > > So, the standard set of generic metrics is just a recommendation and not a > requirement? This sounds good to me since it makes the adoption of the KIP

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-07 Thread Jun Rao
Hi, Magnus, Thanks for the reply. So, the standard set of generic metrics is just a recommendation and not a requirement? This sounds good to me since it makes the adoption of the KIP easier. Regarding the metric names, I have two concerns. (1) If a client already has an existing metric similar

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-07 Thread Magnus Edenhill
Hey Jun, I've clarified the scope of the standard metrics in the KIP, but basically: * We define a standard set of generic metrics that should be relevant to most client implementations, e.g., each producer implementation probably has some sort of per-partition message queue. * A client

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-06-01 Thread Jun Rao
Hi, Magnus, 51. Just to clarify my question. (1) Are standard metrics required for every client for this KIP to function? (2) Are we converting existing java metrics to the standard metrics and deprecating the old ones? If so, could we list all existing java metrics that need to be renamed and

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-05-31 Thread Jun Rao
Hi, Magnus, Thanks for the reply. 51. I think it's fine to have a list of recommended metrics for every client to implement. I am just not sure that standardizing on the metric names across all clients is practical. The list of common metrics in the KIP have completely different names from the

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-05-31 Thread Magnus Edenhill
Den fre 20 maj 2022 kl 01:23 skrev Jun Rao : > Hi, Magus, > > Thanks for the reply. > > 50. Sounds good. > > 51. I miss-understood the proposal in the KIP then. The proposal is to > define a set of common metric names that every client should implement. The > problem is that every client already

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-05-19 Thread Jun Rao
Hi, Magus, Thanks for the reply. 50. Sounds good. 51. I miss-understood the proposal in the KIP then. The proposal is to define a set of common metric names that every client should implement. The problem is that every client already has its own set of metrics with its own names. I am not sure

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-05-19 Thread Magnus Edenhill
Den ons 18 maj 2022 kl 19:57 skrev Jun Rao : > Hi, Magnus, > Hi Jun > > Thanks for the updated KIP. Just a couple of more comments. > > 50. To troubleshoot a particular client issue, I imagine that the client > needs to identify its client_instance_id. How does the client find this > out? Do

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-05-18 Thread Jun Rao
Hi, Magnus, Thanks for the updated KIP. Just a couple of more comments. 50. To troubleshoot a particular client issue, I imagine that the client needs to identify its client_instance_id. How does the client find this out? Do we plan to include client_instance_id in the client log, expose it as a

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-04-04 Thread Jun Rao
Hi, Xavier, Thanks for the reply. 28. It does seem that we have started using KafkaMetrics on the broker side. Then, my only concern is on the usage of Histogram in KafkaMetrics. Histogram in KafkaMetrics statically divides the value space into a fixed number of buckets and only returns values

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-31 Thread Xavier Léauté
> > Are there cases where the metrics plugin developers would want to forward > the compressed payload without decompressing? The only interoperable use-case I can think of would be to forward the payloads directly to an OpenTelemetry collector backend. Today OTLP only mandates gzip/none

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-31 Thread Xavier Léauté
> > 28. On the broker, we typically use Yammer metrics. Only for metrics that > depend on Kafka metric features (e.g., quota), we use the Kafka metric. > Yammer metrics have 4 types: gauge, meter, histogram and timer. meter > calculates a rate, but also exposes an accumulated value. > I don't see

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-31 Thread Magnus Edenhill
Hey Ismael, > > The PushTelemetryRequest handler decompresses the payload before passing > it > > to the metrics plugin. > > This was done to avoid having to expose a public decompression interface > to > > metrics plugin developers. > > > > Are there cases where the metrics plugin developers

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-30 Thread Ismael Juma
On Wed, Mar 30, 2022 at 4:08 AM Magnus Edenhill wrote: > > 41. We include CompressionType in PushTelemetryRequestV0, but not in > > ClientTelemetryPayload. How would the implementer know the compression > type > > for the telemetry payload? > The PushTelemetryRequest handler decompresses the

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-30 Thread Magnus Edenhill
Hey Jun, see response inline: Den mån 21 mars 2022 kl 19:31 skrev Jun Rao : > Hi, Kirk, Sarat, > > A few more comments. > > 40. GetTelemetrySubscriptionsResponseV0 : RequestedMetrics Array[string] > uses "Array[0] empty string" to represent all metrics subscribed. We had a > similar issue with

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-21 Thread Jun Rao
Hi, Kirk, Sarat, A few more comments. 40. GetTelemetrySubscriptionsResponseV0 : RequestedMetrics Array[string] uses "Array[0] empty string" to represent all metrics subscribed. We had a similar issue with the topics field in MetadataRequest and used the following convention. In version 1 and

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-10 Thread Jun Rao
Hi, Kirk, Sarat, Thanks for the reply. 28. On the broker, we typically use Yammer metrics. Only for metrics that depend on Kafka metric features (e.g., quota), we use the Kafka metric. Yammer metrics have 4 types: gauge, meter, histogram and timer. meter calculates a rate, but also exposes an

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-10 Thread Sarat Kakarla
Jun, >> 28. For the broker metrics, could you spell out the full metric name >> including groups, tags, etc? We typically don't add the broker_id label for >> broker metrics. Also, brokers use Yammer metrics, which doesn't have type >> Sum. Sure, I will update the KIP-714 with

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-09 Thread Kirk True
Hi Jun, On Tue, Mar 8, 2022, at 5:47 PM, Jun Rao wrote: > Hi, Magnus, Sarat and Xavier, > > Thanks for the reply. A few more comments below. > > 20. It seems that we are piggybacking the plugin on the > existing MetricsReporter. So, this seems fine. > > 21. That could work. Are we requiring

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-08 Thread Jun Rao
Hi, Magnus, Sarat and Xavier, Thanks for the reply. A few more comments below. 20. It seems that we are piggybacking the plugin on the existing MetricsReporter. So, this seems fine. 21. That could work. Are we requiring any additional jar dependency on the client? Or, are you suggesting that we

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-08 Thread Tom Bentley
Hi Ashenafi, You'll need to unsubscribe from the dev mailing list by sending an email to dev-unsubscr...@kafka.apache.org. No one else can do this for you. Kind regards, Tom On Tue, 8 Mar 2022 at 04:40, Ashenafi Marcos wrote: > Hi, > Can you please take out my email I’d so that will not be

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-07 Thread Ashenafi Marcos
Hi, Can you please take out my email I’d so that will not be able to receive any mail from you. Thank you On Tue, Oct 19, 2021 at 1:30 PM Mickael Maison wrote: > Hi Magnus, > > Thanks for the proposal. > > 1. Looking at the protocol section, isn't "ClientInstanceId" expected > to be a field in

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-03-07 Thread Magnus Edenhill
Hi Jun, thanks for your initiated questions, see my answers below. There's been a number of clarifications to the KIP. Den tors 27 jan. 2022 kl 20:08 skrev Jun Rao : > Hi, Magnus, > > Thanks for updating the KIP. The overall approach makes sense to me. A few > more detailed comments below. >

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-02-02 Thread Xavier Léauté
24.2 Does delta only apply to Counter type? > 24.3 In the delta representation, the first request needs to send the full > value, how does the broker plugin know whether a value is full or delta? > The temporarily semantics are defined by the OpenTelemetry data model. Deferring to OpenTelemetry

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-01-31 Thread Sarat Kakarla
Jun Following are the answers for some the questions raised by you. >> 26. client-metrics entity: >> 26.1 It seems that we could add multiple entities that match to the same >> client. Which one takes precedent? All the matching client metrics would be compiled into a single list and send to

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-01-27 Thread Jun Rao
Hi, Magnus, Thanks for updating the KIP. The overall approach makes sense to me. A few more detailed comments below. 20. ClientTelemetry: Should it be extending configurable and closable? 21. Compression of the metrics on the client: what's the default? 22. "Client metrics plugin / extending

Re: [DISCUSS] KIP-714: Client metrics and observability

2022-01-26 Thread Magnus Edenhill
Hi all, I've updated the KIP with responses to the latest comments: Java client dependencies (Thanks Kirk!), alternate designs (separate cluster, separate producer, etc), etc. I will revive the vote thread. Thanks, Magnus Den mån 13 dec. 2021 kl 22:32 skrev Ryanne Dolan : > I think we should

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-12-13 Thread Ryanne Dolan
I think we should be very careful about introducing new runtime dependencies into the clients. Historically this has been rare and essentially necessary (e.g. compression libs). Ryanne On Mon, Dec 13, 2021, 1:06 PM Kirk True wrote: > Hi Jun, > > On Thu, Dec 9, 2021, at 2:28 PM, Jun Rao wrote:

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-12-13 Thread Kirk True
Hi Jun, On Thu, Dec 9, 2021, at 2:28 PM, Jun Rao wrote: > 13. Using OpenTelemetry. Does that require runtime dependency > on OpenTelemetry library? How good is the compatibility story > of OpenTelemetry? This is important since an application could have other > OpenTelemetry dependencies than the

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-12-09 Thread Jun Rao
Hi, Magnus. Thanks for the KIP. A few comments below. 10. There seems to be some questions on the use cases of this KIP since we already have a client side metric reporter. It would be useful to provide a bit more details on that. To me, there are 3 potential use cases: (1) not all organizations

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-29 Thread Magnus Edenhill
Hey Bob, That's a good point. Request type labels were considered but since they're already tracked by broker-side metrics they were left out as to avoid metric duplication, however those metrics are not per connection, so they won't be that useful in practice for troubleshooting specific client

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-29 Thread Magnus Edenhill
Hi Viktor, that's a good idea, I've added a bunch of broker-side metrics for the client metrics handling. There might be more added during development as the need arise.

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-23 Thread Bob Barrett
Hi Magnus, Thanks for the thorough KIP, this seems very useful. Would it make sense to include the request type as a label for the `client.request.success`, `client.request.errors` and `client.request.rtt` metrics? I think it would be very useful to see which specific requests are succeeding and

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-22 Thread Viktor Somogyi-Vass
Hi Magnus, I think this is a very useful addition. We also have a similar (but much more simplistic) implementation of this. Maybe I missed it in the KIP but what about adding metrics about the subscription cache itself? That I think would improve its usability and debuggability as we'd be able

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-18 Thread Magnus Edenhill
Hi Mickael, see inline. Den ons 10 nov. 2021 kl 15:21 skrev Mickael Maison : > Hi Magnus, > > I see you've addressed some of the points I raised above but some (4, > 5) have not been addressed yet. > Re 4) How will the user/app know metrics are being sent. One possibility is to add a JMX

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-10 Thread Mickael Maison
Hi Magnus, I see you've addressed some of the points I raised above but some (4, 5) have not been addressed yet. I'm really uneasy with this being enabled by default on the client side. When collecting data, I think the best practice is to ensure users are explicitly enabling it. You mentioned

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-07 Thread Magnus Edenhill
Thanks David for pointing this out, I've updated the KIP to include client_id as a matching selector. Regards, Magnus Den tors 4 nov. 2021 kl 18:01 skrev David Mao : > Hey Magnus, > > I noticed that the KIP outlines the initial selectors supported as: > >- client_instance_id -

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-11-04 Thread David Mao
Hey Magnus, I noticed that the KIP outlines the initial selectors supported as: - client_instance_id - CLIENT_INSTANCE_ID UUID string representation. - client_software_name - client software implementation name. - client_software_version - client software implementation version. In

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-19 Thread Magnus Edenhill
Hi Mickael! Den tis 19 okt. 2021 kl 19:30 skrev Mickael Maison : > Hi Magnus, > > Thanks for the proposal. > > 1. Looking at the protocol section, isn't "ClientInstanceId" expected > to be a field in GetTelemetrySubscriptionsResponseV0? Otherwise, how > does a client retrieve this value? > Good

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-19 Thread Mickael Maison
Hi Magnus, Thanks for the proposal. 1. Looking at the protocol section, isn't "ClientInstanceId" expected to be a field in GetTelemetrySubscriptionsResponseV0? Otherwise, how does a client retrieve this value? 2. In the client API section, you mention a new method "clientInstanceId()". Can you

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-19 Thread Magnus Edenhill
Den tis 19 okt. 2021 kl 13:22 skrev Tom Bentley : > Hi Magnus, > > I reviewed the KIP since you called the vote (sorry for not reviewing when > you announced your intention to call the vote). I have a few questions on > some of the details. > > 1. There's no Javadoc on

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-19 Thread Tom Bentley
Hi Magnus, I reviewed the KIP since you called the vote (sorry for not reviewing when you announced your intention to call the vote). I have a few questions on some of the details. 1. There's no Javadoc on ClientTelemetryPayload.data(), so I don't know whether the payload is exposed through this

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-07 Thread Magnus Edenhill
Hi all, I've updated the KIP following our recent discussions on the mailing list: - split the protocol in two, one for getting the metrics subscriptions, and one for pushing the metrics. - simplifications: initially only one supported metrics format, no client.id in the instance id, etc. -

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-04 Thread Magnus Edenhill
Hi Gwen, I'm finishing up the KIP based on the last couple of discussion points in this thread and will call the Vote later this week. Best, Magnus Den lör 2 okt. 2021 kl 02:01 skrev Gwen Shapira : > Hey, > > I noticed that there was no discussion for the last 10 days, but I couldn't > find

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-10-01 Thread Gwen Shapira
Hey, I noticed that there was no discussion for the last 10 days, but I couldn't find the vote thread. Is there one that I'm missing? Gwen On Wed, Sep 22, 2021 at 4:58 AM Magnus Edenhill wrote: > Den tis 21 sep. 2021 kl 06:58 skrev Colin McCabe : > > > On Mon, Sep 20, 2021, at 17:35, Feng Min

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-22 Thread Magnus Edenhill
Den tis 21 sep. 2021 kl 06:58 skrev Colin McCabe : > On Mon, Sep 20, 2021, at 17:35, Feng Min wrote: > > Thanks Magnus & Colin for the discussion. > > > > Based on KIP-714's stateless design, Client can pretty much use any > > connection to any broker to send metrics. We are not associating >

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-22 Thread Magnus Edenhill
Den mån 20 sep. 2021 kl 20:41 skrev Colin McCabe : > On Tue, Sep 14, 2021, at 00:47, Magnus Edenhill wrote: > > Thanks for your feedback Colin, see my updated proposal below. > > ... > > Hi Magnus, > > Thanks for the update. > > > > > Splitting up the API into separate data and control requests

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-21 Thread Feng Min
Hi Colin, It was just analogy to say api version is similar to subscription Id. Every request come with api version information, broker can return an error if supported api version has been changed. It’s similar to the role of subscriptionid here. Thanks, Feng On Mon, Sep 20, 2021 at 9:51 PM

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-20 Thread Colin McCabe
On Mon, Sep 20, 2021, at 17:35, Feng Min wrote: > Thanks Magnus & Colin for the discussion. > > Based on KIP-714's stateless design, Client can pretty much use any > connection to any broker to send metrics. We are not associating connection > with client metric state. Is my understanding correct?

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-20 Thread Colin McCabe
On Mon, Sep 20, 2021, at 12:30, Feng Min wrote: > Some comments about subscriptionId. > > ApiVersion is not a good example. API Version here is actually acting like > an identifier as the client will carry this information. Forcing to > disconnect a connection from the server side is quite heavy.

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-20 Thread Feng Min
Thanks Magnus & Colin for the discussion. Based on KIP-714's stateless design, Client can pretty much use any connection to any broker to send metrics. We are not associating connection with client metric state. Is my understanding correct? If yes, how about the following two scenarios 1) One

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-20 Thread Feng Min
Some comments about subscriptionId. On Mon, Sep 20, 2021 at 11:41 AM Colin McCabe wrote: > On Tue, Sep 14, 2021, at 00:47, Magnus Edenhill wrote: > > Thanks for your feedback Colin, see my updated proposal below. > > ... > > Hi Magnus, > > Thanks for the update. > > > > > Splitting up the API

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-20 Thread Colin McCabe
On Tue, Sep 14, 2021, at 00:47, Magnus Edenhill wrote: > Thanks for your feedback Colin, see my updated proposal below. > ... Hi Magnus, Thanks for the update. > > Splitting up the API into separate data and control requests makes sense. > With a split we would have one API for querying the

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-20 Thread Feng Min
LGTM in terms of RPC separation and the new SubscriptionId to detect target metric change on the server side. On Tue, Sep 14, 2021 at 12:48 AM Magnus Edenhill wrote: > Thanks for your feedback Colin, see my updated proposal below. > > > Den tors 22 juli 2021 kl 03:17 skrev Colin McCabe : > > >

Re: [DISCUSS] KIP-714: Client metrics and observability

2021-09-14 Thread Magnus Edenhill
Thanks for your feedback Colin, see my updated proposal below. Den tors 22 juli 2021 kl 03:17 skrev Colin McCabe : > On Tue, Jun 29, 2021, at 07:22, Magnus Edenhill wrote: > > Den tors 17 juni 2021 kl 00:52 skrev Colin McCabe : > > > A few critiques: > > > > > > - As I wrote above, I think this

  1   2   >