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 <mag...@edenhill.se> wrote:
> Thanks for your feedback Colin, see my updated proposal below.
>
>
> Den tors 22 juli 2021 kl 03:17 skrev Colin McCabe <cmcc...@apache.org>:
>
> > On Tue, Jun 29, 2021, at 07:22, Magnus Edenhill wrote:
> > > Den tors 17 juni 2021 kl 00:52 skrev Colin McCabe <cmcc...@apache.org
> >:
> > > > A few critiques:
> > > >
> > > > - As I wrote above, I think this could benefit a lot by being split
> > into
> > > > several RPCs. A registration RPC, a report RPC, and an unregister RPC
> > seem
> > > > like logical choices.
> > > >
> > >
> > > Responded to this in your previous mail, but in short I think a single
> > > request is sufficient and keeps the implementation complexity / state
> > down.
> > >
> >
> > Hi Magnus,
> >
> > I still suspect that trying to do everything with a single RPC is more
> > complex than using multiple RPCs.
> >
> > Can you go into more detail about how the client learns what metrics it
> > should send? This was the purpose of the "registration" step in my scheme
> > above.
> >
> > It seems quite awkward to combine an RPC for reporting metrics with and
> > RPC for finding out what metrics are configured to be reported. For
> > example, how would you build a tool to check what metrics are configured
> to
> > be reported? Does the tool have to report fake metrics, just because
> > there's no other way to get back that information? Seems wrong. (It would
> > be a bit like combining createTopics and listTopics for "simplicity")
> >
>
>
>
> Splitting up the API into separate data and control requests makes sense.
> With a split we would have one API for querying the broker for configured
> metrics subscriptions,
> and one API for pushing the collected metrics to the broker.
>
> A mechanism is still needed to notify the client when the subscription is
> changed;
> I’ve added a SubscriptionId for this purpose (which could be a checksum of
> the configured metrics subscription), this id is sent to the client along
> with the metrics subscription, and the client sends it back to the broker
> when pushing metrics. If the broker finds the pushed subscription id to
> differ from what is expected it will return an error to the client, which
> triggers the client to retrieve the new subscribed metrics and an updated
> subscription id. The generation of the subscriptionId is opaque to the
> client.
>
>
> Something like this:
>
> // Get the configured metrics subscription.
> GetTelemetrySubscriptionsRequest {
> StrNull ClientInstanceId // Null on first invocation to retrieve a
> newly generated instance id from the broker.
> }
>
> GetTelemetrySubscriptionsResponse {
> Int16 ErrorCode
> Int32 SubscriptionId // This is used for comparison in
> PushTelemetryRequest. Could be a crc32 of the subscription.
> Str ClientInstanceId
> Int8 AcceptedContentTypes
> Array SubscribedMetrics[] {
> String MetricsPrefix
> Int32 IntervalMs
> }
> }
>
>
> The ContentType is a bitmask in this new proposal, high bits indicate
> compression:
> 0x01 OTLPv08
> 0x10 GZIP
> 0x40 ZSTD
> 0x80 LZ4
>
>
> // Push metrics
> PushTelemetryRequest {
> Str ClientInstanceId
> Int32 SubscriptionId // The collected metrics in this request are
> based on the subscription with this Id.
> Int8 ContentType // E.g., OTLPv08|ZSTD
> Bool Terminating
> Binary Metrics
> }
>
>
> PushTelemetryResponse {
> Int32 ThrottleTime
> Int16 ErrorCode
> }
>
>
> An example run:
>
> 1. Client instance starts, connects to broker.
> 2. > GetTelemetrySubscriptionsRequest{ ClientInstanceId=Null } // Requests
> an instance id and the subscribed metrics.
> 3. < GetTelemetrySubscriptionsResponse{
> ErrorCode = 0,
> SubscriptionId = 0x234adf34,
> ClientInstanceId = f00d-feed-deff-ceff-ffff-…,
> AcceptedContentTypes = OTLPv08|ZSTD|LZ4,
> SubscribeddMetrics[] = {
> { “client.producer.tx.”, 60000 },
> { “client.memory.rss”, 900000 },
> }
> }
> 4. Client updates its metrics subscription, next push to fire in 60
> seconds.
> 5. 60 seconds passes
> 6. > PushTelemetryRequest{
> ClientInstanceId = f00d-feed-deff-ceff-ffff-….,
> SubscriptionId = 0x234adf34,
> ContentType = OTLPv8|ZSTD,
> Terminating = False,
> Metrics = …// zstd-compressed OTLPv08-protobuf-serialized metrics
> }
> 7. < PushTelemetryResponse{ 0, NO_ERROR }
> 8. 60 seconds passes
> 9. > PushTelemetryRequest…
> …
> 56. The operator changes the configured metrics subscriptions (through
> Admin API).
> 57. > PushTelemetryRequest{ .. SubscriptionId = 0x234adf34 .. }
> 58. The subscriptionId no longer matches since the subscription has been
> updated, broker responds with an error:
> 59. < PushTelemetryResponse{ 0, ERR_INVALID_SUBSCRIPTION_ID }
> 60. The error triggers the client to request the subscriptions again.
> 61. > GetTelemetrySubscriptionsRequest{..}
> 62. < GetTelemetrySubscriptionsResponse { .. SubscriptionId = 0x777772211,
> SubscribedMetrics[] = .. }
> 63. Client update its subscription and continues to push metrics
> accordingly.
> …
>
>
> If the broker connection goes down or the connection is to be used for
> other purposes (e.g., blocking FetchRequests), the client will send
> PushTelemetryRequests to any other broker in the cluster, using the same
> ClientInstanceId and SubscriptionId as received in the latest
> GetTelemetrySubscriptionsResponse.
>
> While the subscriptionId may change during the lifetime of the client
> instance (when metric subscriptions are updated), the ClientInstanceId is
> only acquired once and must not change (as it is used to identify the
> unique client instance).
>
>
> >
> > > > - I don't think the client should be able to choose its own UUID.
> This
> > > > adds complexity and introduces a chance that clients will choose an
> ID
> > that
> > > > is not unique. We already have an ID that the client itself supplies
> > > > (clientID) so there is no need to introduce another such ID.
> > > >
> > >
> > > The CLIENT_INSTANCE_ID (which is a combination of the client.id and a
> > UUID)
> > > is actually generated by the receiving broker on first contact.
> > > The need for a new unique semi-random id is outlined in the KIP, but in
> > > short; the client.id is not unique, and we need something unique that
> > still
> > > is prefix-matchable to the client.id so that we can add subscriptions
> > > either using prefix-matching of just the client.id (which may match
> one
> > or
> > > more client instances), and exact matching which will match a one
> > specific
> > > client instance.
> >
> > Hmm... the client id is already sent in every RPC as part of the header.
> > It's not necessary to send it again as part of one of the other RPC
> fields,
> > right?
> >
> > More generally, why does the client instance ID need to be
> > prefix-matchable? That seems like an implementation detail of the metrics
> > collection system used on the broker side. Maybe someone wants to group
> by
> > things other than client IDs -- perhaps client versions, for instance. By
> > the same argument, we should put the client version string in the client
> > instance ID, since someone might want to group by that. Or maybe we
> should
> > include the hostname, and the IP, and, and, and.... You see the issue
> here.
> > I think we shouldn't get involved in this kind of decision -- if we just
> > pass a UUID, the broker-side software can group it or prefix it however
> it
> > wants internally.
> >
>
> Yes, I agree, other selectors will indeed be needed eventually.
> I'll remove the client.id from the CLIENT_INSTANCE_ID and only keep the
> UUID part.
> My assumption is that the set of subscribed metrics prefixes throughout a
> cluster will be quite small initially, so maybe we could leave fine-grained
> selectors out of this proposal
> and address it later when an actual need arises (maybe ACLs can be used for
> selector matching).
> And there is no harm for a client in having a metrics subscription with
> metrics it does not provide, e.g., including the consumer metrics for a
> producer, and vice versa, it will just be ignored by the client
> if it doesn't match a metrics prefix it can provide.
>
> What we do want though is ability to single out a specific client instance
> to give it a more fine-grained subscription for troubleshooting, and
> we can do that with the current proposal with matching solely on the
> CLIENT_INSTANCE_ID.
> In other words; all clients will have the same standard metrics
> subscription, but specific client instances can have alternate
> subscriptions.
>
>
> > > - In general the schema seems to have a bad case of string-itis. UUID,
> > > > content type, and requested metrics are all strings. Since these
> > messages
> > > > will be sent very frequently, it's quite costly to use strings for
> all
> > > > these things. We have a type for UUID, which uses 16 bytes -- let's
> use
> > > > that type for client instance ID, rather than a string which will be
> > much
> > > > larger. Also, since we already send clientID in the message header,
> > there
> > > > is no need to include it again in the instance ID.
> > > >
> > >
> > > As explained above we need the client.id in the CLIENT_INSTANCE_ID.
> And
> > I
> > > don't think the overhead of this one string per request is going to be
> > much
> > > of an issue,
> > > typical metric push intervals are probably in the >60s range.
> > > If this becomes a problem we could use a per-connection identifier that
> > the
> > > broker translates to the client instance id before pushing metrics
> > upwards
> > > in the system.
> > >
> >
> > This is actually an interesting design question -- why not use a
> > per-TCP-connection identifier, rather than a per-client-instance
> > identifier? If we are grouping by other things anyway (clientID,
> principal,
> > etc.) on the server side, do we need to maintain a per-process identifier
> > rather than a per-connection one?
> >
>
>
> The metrics collector/tsdb/whatever will need to identify a single client
> instance, regardless of which broker received the metrics.
> The chapter on CLIENT_INSTANCE_ID motivates why we need a unique
> identifier, basically because neither clientID, principal or remote
> address:port, etc, can be
> used to identify a single client instance.
>
>
>
>
> > >
> > > > - I think it would also be nice to have an enum or something for
> > > > AcceptedContentTypes, RequestedMetrics, etc. We know that new
> > additions to
> > > > these categories will require KIPs, so it should be straightforward
> > for the
> > > > project to just have an enum that allows us to communicate these as
> > ints.
> > > >
> > >
> > > I'm thinking this might be overly constraining. The broker doesn't
> parse
> > or
> > > handle the received metrics data itself but just pushes it to the
> metrics
> > > plugin, using an enum would require a KIP and broker upgrade if the
> > metrics plugin
> > > supports a newer version of OTLP.
> > > It is probably better if we don't strictly control the metric format
> > itself.
> > >
> >
> > Unfortunately, we have to strictly control the metrics format, because
> > otherwise clients can't implement it. I agree that we don't need to
> specify
> > how the broker-side code works, since that is pluggable. It's also
> > reasonable for the clients to have pluggable extensions as well, but this
> > KIP won't be of much use if we don't at least define a basic set of
> metrics
> > that most clients can understand how to send. The open source clients
> will
> > not implement anything more than what is specified in the KIP (or at
> least
> > the AK one won't...)
> >
>
> Makes sense, in the updated proposal above I changed ContentType to a
> bitmask.
>
>
> >
> > >
> > >
> > > > - Can you talk about whether you are adding any new library
> > dependencies
> > > > to the Kafka client? It seems like you'd want to add opencensus /
> > > > opentelemetry, if we are using that format here.
> > > >
> > >
> > > Yeah, as we get closer to concensus more implementation specific
> details
> > > will be added to the KIP.
> > >
> >
> > I'm not sure if OpenCensus adds any value to this KIP, to be honest.
> Their
> > primary focus was never on the format of the data being sent (in fact,
> the
> > last time they checked, they left the format up to each OpenCensus
> > implementation). That may have changed, but I think it still has limited
> > usefulness to us, since we have our own format which we have to use
> anyway.
> >
>
> Oh, I meant concensus as in kafka-dev agreement :)
>
> Feng is looking into the implementation details of the Java client and will
> update the KIP with regards to dependencies.
>
>
>
> >
> > >
> > > >
> > > > - Standard client resource labels: can we send these only in the
> > > > registration RPC?
> > > >
> > >
> > > These labels are part of the serialized OTLP data, which means it would
> > > need to be unpacked and repacked (including compression) by the broker
> > (or
> > > metrics plugin), which I believe is more costly than sending them for
> > each request.
> > >
> >
> > Hmm, that data is about 10 fields, most of which are strings. It
> certainly
> > adds a lot of overhead to resend it each time.
> >
> > I don't follow the comment about unpacking and repacking -- since the
> > client registered with the broker it already knows all this information,
> so
> > there's nothing to unpack or repack, except from memory. If it's more
> > convenient to serialize it once rather than multiple times, that is an
> > implementation detail of the broker side plugin, which we are not
> > specifying here anyway.
> >
>
> The current proposal is pretty much stateless on the broker, it does not
> need to hold any state for a client (instance), and no state
> synchronization is needed
> between brokers in the cluster, which allows a client to seamlessly send
> metrics to any broker it wants and keeps the API overhead down (no need to
> re-register when
> switching brokers for instance).
>
> We could remove the labels that are already available to the broker on a
> per-request basis or that it already maintains state for:
> - client_id
> - client_instance_id
> - client_software_*
>
> Leaving the following to still be included:
> - group_id
> - group_instance_id
> - transactional_id
> etc..
>
> What do you think of that?
>
>
> Thanks,
> Magnus
>
>
>
> >
> > best,
> > Colin
> >
> > > Thanks,
> > > Magnus
> > >
> > > >
> > > >
> > >
> >
>
--
Best,
Feng
