Hey Tom,
Den mån 21 juni 2021 kl 21:08 skrev Tom Bentley <tbent...@redhat.com>:
>
> 1. Did you consider using a `default ClientTelemetryReceiver
> clientReceiver() { return null; }` method on the existing MetricsReporter
> interface, avoiding the need for the ClientTelemetry trait?
>
I'll let Xavier answer this one since he designed the new interface.
> 2. On the metrics naming and format, I wasn't really clear about what's
> being proposed. I assume we're taking a subset of the existing client
> metrics and representing them as OpenTelemetry metrics, but it didn't
> really explain how the existing metric names would be mapped to meter and
> instrument names. Or did I misunderstand?
>
The KIP is approaching the set of standard metrics from a general viewpoint
rather
than what exactly is provided by the Java clients today, and this is
because we
want these standard metrics to make sense across all languages and all
client implementations.
They're loosely based on existing metrics across the dominant client
implementations.
It is up to each client maintainer to map its existing metrics to the
metrics defined here.
Also, not all metrics may make sense for all clients since the
implementations differ.
3. In the client behaviour section it doesn't explicitly say whether the
> client uses a dedicated thread for this work (I assume it does).
>
Client implementation details are currently left out of the KIP, the focus
is currently
more general protocol-level and high-level client and broker semantics.
I'm not sure if it's best to add Java client specifics to KIP-714, or make
a new KIP
with the Java client implementation details once KIP-714 is accepted.
4. The description of the FunctionalityNotEnabled error code suggests that
> PushTelemetryRequest would only be included in an ApiVersions response if
> the broker was configured with a plugin. I think the ApiVersionsResponse is
> normally a constant response (not dependent on broker config), so I wonder
> whether this is really a precedent we want to set here? Surely in a broker
> without a plugin configured it could just return an empty set of
> RequestedMetrics and a maxint NextPushMs in the PushTelemetryResponse?
>
Yes, that's a good idea. That would also solve the (future) issue with
enabling a metrics plugin
while the broker was running.
> 5. Maybe the AcceptedContentTypes should be documented to be in priority
> order. That would simplify the action for UnsupportedCompressionType.
>
Good idea!
> 6. """As the client will not know the broker id of its bootstrap servers
> the broker_id label should be set to “bootstrap”.""" Maybe using the same
> convention as is used in the NetworkClient, where bootstrap servers are the
> id of the negative of their index in the list?
>
This too!
> 7. Maybe call it "client.process.rss.bytes" rather than
> "client.process.memory.bytes",
> to be explicit?
>
Yeah I started out with rss but then went with something more generic.
Don't really have a strong opinion.
8. It's a little confusing that --id option to kafka-client-metrics.sh can
> be a prefix or an exact match. Perhaps --id and --id-prefix would be
> clearer.
>
Makes sense.
> 9. Maybe I missed it, but does the client continue to push metrics to the
> same broker as it randomly picked initially? If it gets disconnected from
> that broker what happens, does it just randomly pick another?
>
Yep, and the new broker must accept the already assigned CLIENT_INSTANCE_ID
that the client is using.
10. To subscribe to all metrics I assume I can just do
> `kafka-client-metrics.sh ... --metric ''`? It might be worth saying this
> explicitly. AFAICS this is the only way to find out all the metrics
> supported by a client if you don't already know from the client's software
> version.
>
Will make a note of that.
Thanks for the valuable input, will update the KIP accordingly.
/Magnus
>
