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 <d...@confluent.io.invalid>: > 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 the given reactive monitoring workflow, we mention that the application > user does not know their client's client instance ID, but it's outlined > that the operator can add a metrics subscription selecting for clientId. I > don't see clientId as one of the supported selectors. > I can see how this would have made sense in a previous iteration given that > the previous client instance ID proposal was to construct the client > instance ID using clientId as a prefix. Now that the client instance ID is > a UUID, would we want to add clientId as a supported selector? > Let me know what you think. > > David > > On Tue, Oct 19, 2021 at 12:33 PM Magnus Edenhill <mag...@edenhill.se> > wrote: > > > Hi Mickael! > > > > Den tis 19 okt. 2021 kl 19:30 skrev Mickael Maison < > > mickael.mai...@gmail.com > > >: > > > > > 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 catch, it got removed by mistake in one of the edits. > > > > > > > > > > 2. In the client API section, you mention a new method > > > "clientInstanceId()". Can you clarify which interfaces are affected? > > > Is it only Consumer and Producer? > > > > > > > And Admin. Will update the KIP. > > > > > > > > > 3. I'm a bit concerned this is enabled by default. Even if the data > > > collected is supposed to be not sensitive, I think this can be > > > problematic in some environments. Also users don't seem to have the > > > choice to only expose some metrics. Knowing how much data transit > > > through some applications can be considered critical. > > > > > > > The broker already knows how much data transits through the client > though, > > right? > > Care has been taken not to expose information in the standard metrics > that > > might > > reveal sensitive information. > > > > Do you have an example of how the proposed metrics could leak sensitive > > information? > > As for limiting the what metrics to export; I guess that could make sense > > in some > > very sensitive use-cases, but those users might disable metrics > altogether > > for now. > > Could these concerns be addressed by a later KIP? > > > > > > > > > > > > 4. As a user, how do you know if your application is actively sending > > > metrics? Are there new metrics exposing what's going on, like how much > > > data is being sent? > > > > > > > That's a good question. > > Since the proposed metrics interface is not aimed at, or directly > available > > to, the application > > I guess there's little point of adding it here, but instead adding > > something to the > > existing JMX metrics? > > Do you have any suggestions? > > > > > > > > > 5. If all metrics are enabled on a regular Consumer or Producer, do > > > you have an idea how much throughput this would use? > > > > > > > It depends on the number of partition/topics/etc the client is producing > > to/consuming from. > > I'll add some sizes to the KIP for some typical use-cases. > > > > > > Thanks, > > Magnus > > > > > > > Thanks > > > > > > On Tue, Oct 19, 2021 at 5:06 PM Magnus Edenhill <mag...@edenhill.se> > > > wrote: > > > > > > > > Den tis 19 okt. 2021 kl 13:22 skrev Tom Bentley <tbent...@redhat.com > >: > > > > > > > > > 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 method as compressed or > > > not. > > > > > Later on you say "Decompression of the payloads will be handled by > > the > > > > > broker metrics plugin, the broker should expose a suitable > > > decompression > > > > > API to the metrics plugin for this purpose.", which suggests it's > the > > > > > compressed data in the buffer, but then we don't know which codec > was > > > used, > > > > > nor the API via which the plugin should decompress it if required > for > > > > > forwarding to the ultimate metrics store. Should the > > > ClientTelemetryPayload > > > > > expose a method to get the compression and a decompressor? > > > > > > > > > > > > > Good point, updated. > > > > > > > > > > > > > > > > > 2. The client-side API is expressed as StringOrError > > > > > ClientInstance::ClientInstanceId(int timeout_ms). I understand that > > > you're > > > > > thinking about the librdkafka implementation, but it would be good > to > > > show > > > > > the API as it would appear on the Apache Kafka clients. > > > > > > > > > > > > > This was meant as pseudo-code, but I changed it to Java. > > > > > > > > > > > > > 3. "PushTelemetryRequest|Response - protocol request used by the > > > client to > > > > > send metrics to any broker it is connected to." To be clear, this > > means > > > > > that the client can choose any of the connected brokers and push to > > > just > > > > > one of them? What should a supporting client do if it gets an error > > > when > > > > > pushing metrics to a broker, retry sending to the same broker or > try > > > > > pushing to another broker, or drop the metrics? Should supporting > > > clients > > > > > send successive requests to a single broker, or round robin, or is > > > that up > > > > > to the client author? I'm guessing the behaviour should be sticky > to > > > > > support the rate limiting features, but I think it would be good > for > > > client > > > > > authors if this section were explicit on the recommended behaviour. > > > > > > > > > > > > > You are right, I've updated the KIP to make this clearer. > > > > > > > > > > > > > 4. "Mapping the client instance id to an actual application > instance > > > > > running on a (virtual) machine can be done by inspecting the > metrics > > > > > resource labels, such as the client source address and source port, > > or > > > > > security principal, all of which are added by the receiving broker. > > > This > > > > > will allow the operator together with the user to identify the > actual > > > > > application instance." Is this really always true? The source IP > and > > > port > > > > > might be a loadbalancer/proxy in some setups. The principal, as > > already > > > > > mentioned in the KIP, might be shared between multiple > applications. > > > So at > > > > > worst the organization running the clients might have to consult > the > > > logs > > > > > of a set of client applications, right? > > > > > > > > > > > > > Yes, that's correct. There's no guaranteed mapping from > > > client_instance_id > > > > to > > > > an actual instance, that's why the KIP recommends client > > implementations > > > to > > > > log the client instance id > > > > upon retrieval, and also provide an API for the application to > retrieve > > > the > > > > instance id programmatically > > > > if it has a better way of exposing it. > > > > > > > > > > > > 5. "Tests indicate that a compression ratio up to 10x is possible for > > the > > > > > standard metrics." Client authors might appreciate your mentioning > > > which > > > > > compression codec got these results. > > > > > > > > > > > > > Good point. Updated. > > > > > > > > > > > > > 6. "Should the client send a push request prior to expiry of the > > > previously > > > > > calculated PushIntervalMs the broker will discard the metrics and > > > return a > > > > > PushTelemetryResponse with the ErrorCode set to RateLimited." Is > this > > > > > RATE_LIMITED a new error code? It's not mentioned in the "New Error > > > Codes" > > > > > section. > > > > > > > > > > > > > That's a leftover, it should be using the standard ThrottleTime > > > mechanism. > > > > Fixed. > > > > > > > > > > > > > 7. In the section "Standard client resource labels" application_id > is > > > > > described as Kafka Streams only, but the section of "Client > > > Identification" > > > > > talks about "application instance id as an optional future > > nice-to-have > > > > > that may be included as a metrics label if it has been set by the > > > user", so > > > > > I'm confused whether non-Kafka Streams clients should set an > > > application_id > > > > > or not. > > > > > > > > > > > > > I'll clarify this in the KIP, but basically we would need to add an ` > > > > application.id` config > > > > property for non-streams clients for this purpose, and that's outside > > the > > > > scope of this KIP since we want to make it zero-conf:ish on the > client > > > side. > > > > > > > > > > > > > > > > > > Kind regards, > > > > > > > > > > Tom > > > > > > > > > > > > > Thanks for the review, > > > > Magnus > > > > > > > > > > > > > > > > > > > > > > On Thu, Oct 7, 2021 at 5:26 PM Magnus Edenhill <mag...@edenhill.se > > > > > wrote: > > > > > > > > > > > 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. > > > > > > - made CLIENT_METRICS subscription configuration entries more > > > structured > > > > > > and allowing better client matching selectors (not only on the > > > > > instance > > > > > > id, but also the other > > > > > > client resource labels, such as client_software_name, etc.). > > > > > > > > > > > > Unless there are further comments I'll call the vote in a day or > > two. > > > > > > > > > > > > Regards, > > > > > > Magnus > > > > > > > > > > > > > > > > > > > > > > > > Den mån 4 okt. 2021 kl 20:57 skrev Magnus Edenhill < > > > mag...@edenhill.se>: > > > > > > > > > > > > > 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 > > > > > > <g...@confluent.io.invalid > > > > > > > >: > > > > > > > > > > > > > >> 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 < > > > mag...@edenhill.se> > > > > > > >> wrote: > > > > > > >> > > > > > > >> > Den tis 21 sep. 2021 kl 06:58 skrev Colin McCabe < > > > > > cmcc...@apache.org > > > > > > >: > > > > > > >> > > > > > > > >> > > 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? > If > > > yes, > > > > > > how > > > > > > >> > about > > > > > > >> > > > the following two scenarios > > > > > > >> > > > > > > > > > >> > > > 1) One Client (Client-ID) registers two different client > > > > > instance > > > > > > id > > > > > > >> > via > > > > > > >> > > > separate registration. Is it permitted? If OK, how to > > > > > distinguish > > > > > > >> them > > > > > > >> > > from > > > > > > >> > > > the case 2 below. > > > > > > >> > > > > > > > > > >> > > > > > > > > >> > > Hi Feng, > > > > > > >> > > > > > > > > >> > > My understanding, which Magnus can clarify I guess, is > that > > > you > > > > > > could > > > > > > >> > have > > > > > > >> > > something like two Producer instances running with the > same > > > > > > client.id > > > > > > >> > > (perhaps because they're using the same config file, for > > > example). > > > > > > >> They > > > > > > >> > > could even be in the same process. But they would get > > separate > > > > > > UUIDs. > > > > > > >> > > > > > > > > >> > > I believe Magnus used the term client to mean "Producer or > > > > > > Consumer". > > > > > > >> So > > > > > > >> > > if you have both a Producer and a Consumer in your > > > application I > > > > > > would > > > > > > >> > > expect you'd get separate UUIDs for both. Again Magnus can > > > chime > > > > > in > > > > > > >> > here, I > > > > > > >> > > guess. > > > > > > >> > > > > > > > > >> > > > > > > > >> > That's correct. > > > > > > >> > > > > > > > >> > > > > > > > >> > > > > > > > > >> > > > 2) How about the client restarting? What's the > > expectation? > > > > > Should > > > > > > >> the > > > > > > >> > > > server expect the client to carry a persisted client > > > instance id > > > > > > or > > > > > > >> > > should > > > > > > >> > > > the client be treated as a new instance? > > > > > > >> > > > > > > > > >> > > The KIP doesn't describe any mechanism for persistence, > so I > > > would > > > > > > >> assume > > > > > > >> > > that when you restart the client you get a new UUID. I > agree > > > that > > > > > it > > > > > > >> > would > > > > > > >> > > be good to spell this out. > > > > > > >> > > > > > > > > >> > > > > > > > > >> > Right, it will not be persisted since a client instance > can't > > be > > > > > > >> restarted. > > > > > > >> > > > > > > > >> > Will update the KIP to make this clearer. > > > > > > >> > > > > > > > >> > /Magnus > > > > > > >> > > > > > > > >> > > > > > > >> > > > > > > >> -- > > > > > > >> Gwen Shapira > > > > > > >> Engineering Manager | Confluent > > > > > > >> 650.450.2760 | @gwenshap > > > > > > >> Follow us: Twitter | blog > > > > > > >> > > > > > > > > > > > > > > > > > > > > > > > >
