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
> >
> > >
> > >
> >
>
