-
Notifications
You must be signed in to change notification settings - Fork 78
Proposal for adding KIP-714 support to metrics-reporter #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,104 @@ | ||
| # Add KIP-714 support to metrics-reporter | ||
|
|
||
| Apache Kafka 3.7.0 introduced the ability for Kafka clients to send their metrics to brokers via [KIP-714](https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability). | ||
|
|
||
| The Kafka clients have this feature built-in and enabled by default. However for this feature to be usable, administrators must implement and provide a broker-side plugin to collect the metrics. The plugin must be a [`MetricReporter`](https://kafka.apache.org/40/javadoc/org/apache/kafka/common/metrics/MetricsReporter.html) instance that also implements the [`ClientTelemetry`](https://kafka.apache.org/40/javadoc/org/apache/kafka/server/telemetry/ClientTelemetry.html) interface. | ||
|
|
||
| Then administrators must set metrics subscriptions to define the metrics clients will send. There are no default subscriptions. Subscriptions can be set, updated and deleted at runtime via the `kafka-configs.sh` or `kafka-client-metrics.sh` tools, or via the `Admin` API. For example: | ||
| ```sh | ||
| ./bin/kafka-client-metrics.sh --bootstrap-server localhost:9092 \ | ||
| --alter --name topic-metrics \ | ||
| --metrics org.apache.kafka.producer.topic. \ | ||
| --interval 30000 | ||
| ``` | ||
| A subscription is composed of: | ||
| - A name: This can be provided by the administrators or generated using the `--generate-name` flag. This is used to unique identify the subscription to describe, alter or delete it. | ||
| - A list of metric prefixes: They indicate which metrics the clients should send to the brokers. It can be set to `*` to request all metrics. | ||
| - An optional list of client matching filters: They indicate which clients this subscription is for. The filters are `client_id`, `client_instance_id`, `client_software_name`, `client_software_version`, `client_source_address`, `client_source_port`. If not specified, the subscription applies to all clients. | ||
| - An interval: This indicates how often clients matching this subscription should send their metrics. The interval is in milliseconds and it must be between 100 and 3600000 (1 hour). If not specified, it defaults to 300000 (5 minutes). | ||
|
|
||
| This proposes adding support for KIP-714 to the server-side metric reporter, `ServerKafkaMetricsReporter`. | ||
|
|
||
| ## Motivation | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I strongly disagree with this section. Monitoring applications is essential. And one of the keys to making it useful in situations when you need the monitoring the most are independent communication channels. I.e. do not monitor Kafka dependents through Kafka (the same Kafka cluster they depend on / the same SaaS Kafka service etc.). The KIP-714 is a great business strategy for some vendors, but a bad architecture pattern. And the cloud native landscape has various established solutions for metric collection across distributed application landscapes, which provides this. I'm not saying that we cannot have some support for KIP-714 - it is a Kafka feature and we provide support for Kafka 🤷. But we should not present it as some superior solution.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've not tried comparing the different monitoring solutions, each has different tradeoffs and strengths. Nowhere I say this is superior to another alternative. I just described some of the benefits support for KIP-714 could bring to users and I believe all the statements I made are valid. I'm happy including weaknesses of the KIP-714 model if you think that would be useful.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't read the motivation as a way to highlight the Kafka client metrics support as the best one.
... could be changed in a way that it mentions the Kafka solution. Not sure if Jakub is referring to this sentence like "having the clients to send metrics to broker is the easiest and best solution ..."?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think the motivation is valid. The main goal of KIP-714 is to make troubleshooting Kafka clients easier. Specifically, the zero-config approach is something that established solutions can't provide. Maybe we can explicitly mention that.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Trust me: Troubleshooting Kafka client through information that is supposed to be sent by ... wait for it ... exactly the same Kafka client you are trying to troubleshoot ... does not work well and is not a good practice.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I re-read the motivation again and my perspective is still the same after my first comment ... it doesn't sound like Mickael si saying this is a great and awesome feature and the best one compared to others.
@scholzj Mickael is not a user per-se but a contributor. Why should we accept this only from a user? Because it would be clear that coming from users it would be useful? It's even possible that adding this feature, we'll have a lot of users using it.
@mimaison remember the Strimzi maintainers is a group of people ;-)
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@ppatierno I did not say we should accept it only from a user. I said that if it would be up to me, I would leave it for a user contribution as I do not consider it well invested time and since user contributions are a useful measure of a real user interest (and I'm actually a bit surprised no user - as far as I know - asked about this feature in the past). But as I said, it is not my time being invested here, so it is not my problem, just my opinion. I also acknowledged that my (maintainer of a CNCF project) role is obviously different from Mickael's role and our motivations might differ. But I stand behind my opinion that I'm not going to give my +1 vote to a feature which has a motivation that does not follow the best interest of users but the best interest of one or more Kafka vendors. That said, I'm not likely to -1 this just because of the motivation. So as you said, there are other maintainers as well. 🤷
That is of course true. But I think it is sad that you and me are the only maintainers who actually commented on this during the time this is opened. I think that makes it pretty hard for people to read the room and understand that
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My intention was just to make clear to Mickael that contributors should not give up just because one maintainer doesn't agree on a proposal. It always happen to me as well to be against something but it's not the reason why something new shouldn't be added.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am happy for Strimzi to provide support for this feature. I can see it being useful for organisations where a single team manages the Kafka cluster while other teams, that are inexperienced with Kafka internals, write and manage the client applications. I don't have any problem with the motivation section. To me it mostly reads as explaining why users might want to do this, rather than it being a strong recommendation that this is the best way. The only sentence that could be softened is
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have a problem with the motivation section either. Maybe just softening that |
||
|
|
||
| This proposal shares its motivations with KIP-714. Monitoring applications is essential to ensure they function correctly. While brokers emit themselves a lot of metrics, it's often necessary to get client metrics to diagnose issues. However collecting metrics from all applications can be challenging. This can be due to multiple reasons: | ||
| - Applications deployed in distributed and heterogeneous environments | ||
| - Applications run and owned by separate teams | ||
| - Kafka clients embedded in complex applications | ||
|
|
||
| When clients are able to send their metrics to broker this eases their collection, and greatly simplifies diagnosing issues. The mechanism to set subscriptions also allows users to precisely adjust metrics at runtime to collect the most relevant metrics. | ||
|
|
||
| ## Proposal | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. could we try to clearly split which part of the proposal is for the metrics-reporter component and which one is for the operator?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The part specific for the operator is in the |
||
|
|
||
| Now that we separated the metrics-reporter into client-side and server-side modules ([Proposal 96](./096-split-metrics-reporter-into-modules.md)), we can make the server-side module implement the `ClientTelemetry` interface to support KIP-714. | ||
|
|
||
| ### Naming | ||
|
|
||
| The metrics reporter will expose client metrics with the `clients_` prefix. For example if a subscription is created for `org.apache.kafka.producer.topic` and a client emits the `org.apache.kafka.producer.topic.byte.rate` metric, it will be exposed as `clients_org_apache_kafka_producer_topic_byte_rate`. | ||
|
|
||
| When a client retrieves its metric subscriptions, it is assigned a unique client instance Id (UUID). This Id is used to identify all the metrics for a specific client instance, even if it sends them to different brokers. This Id is always added as a label using the `client_instance_id` name to all metric series. | ||
|
|
||
| ### Configurations | ||
|
|
||
| The `client_instance_id` label is nice to identify all metrics from a client but does not enable to identify which client it is. Fortunately every time clients send metrics, they attach a bunch of metadata to identify themselves. This metadata is an [`AuthorizableRequestContext`](https://kafka.apache.org/40/javadoc/org/apache/kafka/server/authorizer/AuthorizableRequestContext.html) object that contains the client address, client Id, correlation Id, listener name, principal, request type, request version and security protocol from the client. | ||
|
|
||
| A few of these fields are of very low value for metrics: | ||
| - request type: This is always `72` which is the [`PushTelemetry`](https://kafka.apache.org/protocol#The_Messages_PushTelemetry) API key. | ||
| - request version: This is the version of the `PushTelemetry` request. As of Kafka 4.0, it is always `0`. | ||
| - correlation Id: This starts at zero for each client and keeps increasing every time the client sends metrics. This shouldn't be used as a label as it would effectively create a new metric series each time. | ||
|
|
||
| The reporter can convert the other fields as labels and add them to the metric series. In many cases it does not make sense to add all of them, for example if the cluster is behind a proxy the client address will always be the same. Also using all of them can create high cardinality labels series which can be problematic in Prometheus. | ||
|
|
||
| I propose introducing a configuration to select the metadata fields to use as labels: | ||
| - `prometheus.metrics.reporter.telemetry.labels`: List of label names in client metrics. The valid names are `client_id`, `listener_name`, `security_protocol`, `principal`, `client_address`. This defaults to `client_id`. This configuration is reconfigurable at runtime. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it ok to default to an optional configuration? Maybe we could default to client_id and client_address.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see what you mean about In the end you need to know which are the best labels to use for your own environment.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree to use |
||
|
|
||
| ### Client metrics life cycle | ||
|
|
||
| When the reporter receives metrics from a client, it converts them and adds them into Prometheus registry, ready to be collected when the `GET /metrics` endpoint is scraped. The subscription specifies an interval for the client to send metrics. However when metrics are received, the interval or any details about the subscription that caused this emission is not available. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If Strimzi provided a way to manage the subscription, and therefore could inspect the interval, would that enable a better process for managing the lifecycle of the client metrics?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really. You'd have to find the matching rule. This would also not work outside of the Strimzi operator. |
||
|
|
||
| So we need a mechanism to remove metrics from the registry when a client stops emitting metrics, otherwise if a client shuts down, the metrics reporter will keep reporting its metrics. As we don't have the interval, the simplest approach is to delete client metrics whenever they are scraped. While this may create gaps in metric series, this ensures that metrics collected are always valid. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What if we have two Prometheus instances scraping from the same SMR endpoint? Depending on configuration, one of them could be starved and never see client metrics.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is it common to have 2 Prometheus servers scraping the same endpoints? I have to admit that's not a scenario I've considered.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I mean, why not? You could easily have dedicated Prometheus instances, for example one dedicated to server metrics and another one to client metrics, or maybe one instance collecting a critical set of metrics from all Kafka clusters, and then a smaller instance for each cluster with a bigger set of metrics. What about deleting removed metrics on telemetry pushes? I don't know if there is any technical limitation, but you already have the old set in the Prometheus registry, so it should be possible to remove metrics by comparing them with the incoming set.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
The issue is when the client stops pushing new metrics, you then keep the last value you received and start exposing stale metrics.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, that's a problem.
Isn't this a design flaw that should be addressed in Kafka with a small KIP? Or can we use the admin client to get this information?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean if Prometheus is scraping the endpoint, but then a user directly queries it then it would prevent the metrics from being collect by Prometheus? If so we would need to make sure this is very clear in the docs. I would assume mostly this wouldn't happen, but when troubleshooting sometimes people do unexpected things.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
With the current proposal, when the endpoint is scraped by anything, user or Prometheus, all existing client metrics in the Prometheus registry are returned, and then the client metrics are deleted from the registry. Clients send their metrics periodically based on the configured push interval. Until clients push their metrics again, if the endpoint is scraped again, there won't be any client metrics.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was wondering if for debugging purposes, when the user wants to scrape without deleting metrics, we could have a query parameter like |
||
|
|
||
| Typically the subscription interval and the scrapping interval should be of the same order of magnitude. [Prometheus default scaping interval](https://prometheus.io/docs/prometheus/latest/configuration/configuration/#configuration-file) is 1 minute, and the [default metric interval](https://github.com/apache/kafka/blob/trunk/server/src/main/java/org/apache/kafka/server/metrics/ClientMetricsConfigs.java#L89) is 5 minutes, so this should work fine and not cause too much confusion. | ||
|
|
||
| ### Monitoring client metric collection | ||
|
|
||
| Kafka brokers expose a number of metrics to monitor the client metrics collection. | ||
|
|
||
| - `kafka_server_client_metrics_instance_count`: This is the number of clients currently emitting metrics. | ||
| - `kafka_server_client_metrics_plugin_error_count{client_instance_id="<ID>"}`/`kafka_server_client_metrics_plugin_error_rate{client_instance_id="<ID>"}`: The count and rate of errors while handling client metrics. An error means the metrics reporter threw an exception while converting client metrics to Prometheus. | ||
| - `kafka_server_client_metrics_plugin_export_count{client_instance_id="<ID>"}`/`kafka_server_client_metrics_plugin_export_rate{client_instance_id="<ID>"}`: The count and rate of calls to the metric reporter to convert client metrics to Prometheus. | ||
| - `kafka_server_client_metrics_plugin_export_time_avg{client_instance_id="<ID>"}`/`kafka_server_client_metrics_plugin_export_time_max{client_instance_id="<ID>"}`: The average and maximum time the metrics reporter takes to convert client metrics to Prometheus. | ||
| - `kafka_server_client_metrics_throttle_count{client_instance_id="<ID>"}`/`kafka_server_client_metrics_throttle_rate{client_instance_id="<ID>"}`: `PushTelemetry` requests can be throttled if they violate quotas defined by the cluster administrator. The count and rate of throttled `PushTelemetry` requests. | ||
| - `kafka_server_client_metrics_unknown_subscription_request_count`/`kafka_server_client_metrics_unknown_subscription_request_rate`: The count and rate of `PushTelemetry` requests received from clients that have not retrieved their subscriptions. | ||
|
|
||
| Administrators should monitor these metrics when they set subscriptions. | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will we document this as a recommendation, or did you have something else in mind?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes this recommendation could go in the docs. I think it's important to understand the extra load subscriptions can add to your clusters.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do we have some quantification here? I mean roughly in %, how much overhead could it add? (I know it depends on factors like number of clients, metrics, and intervals, but in general...)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No I don't have numbers as I have only run this in a local one node cluster. As you said, the impact will largely depend on the number of clients and the subscriptions enabled, so it probably extends from insignificant to major slowdown. That's why I mention these metrics as these should help evaluate the impact of handling client metrics. |
||
|
|
||
| ### Updates to strimzi-kafka-operator | ||
|
|
||
| The `metricsConfig` definition will support the new configuration if the `type` is set to `strimziMetricsReporter`, via a new value `telemetryLabels`. For example: | ||
| ```yaml | ||
| metricsConfig: | ||
| type: strimziMetricsReporter | ||
| values: | ||
| allowList: | ||
| - "kafka_log.*" | ||
| - "kafka_network.*" | ||
| telemetryLabels: | ||
| - "client_id" | ||
| - "principal" | ||
|
Comment on lines
+86
to
+88
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, how do these labels apply to the Kafka server metrics? I would expect this configuration to be a bit more sophisticated:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These labels would only apply to client metrics and have no impact on server metrics. Likewise the I understand your concern that this could be confusing. We could start by adjust the name of the new field, maybe We've now split the reporter into server and client side modules, but it's not in a metrics-reporter release yet, and that will need to be introduced into the operator too. The client label configuration only applies to the server side module.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would go with the |
||
| ``` | ||
| If this value is not set, it will default to `client_id` like in the metrics reporter. | ||
|
|
||
| ## Affected/not affected projects | ||
|
|
||
| This affects metrics-reporter and strimzi-kafka-operator. | ||
|
|
||
| ## Compatibility | ||
|
|
||
| This feature will only have an effect when subscriptions are set. If none are set, which is the default, there should be no change in behavior. | ||
|
|
||
| ## Rejected alternatives | ||
|
|
||
| ### Configuration to disable/enable client metrics | ||
|
|
||
| - Administrators must define subscriptions to specify the clients and the metrics they are interested in collecting. By default there are no subscriptions, meaning no clients are sending metrics. For that reason, I don't propose adding a configuration to the metrics-reporter to enable or disable client metrics collection, this should be managed via the subscriptions. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sounds like it should have some declarative way of managing and not rely on the Kafka commands ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Chatting with @ppatierno last week he also suggested introducing a mechanism to manage metric subscriptions.
I've not included this in the proposal yet as I'm not familiar enough with the operator to propose a design that makes sense. So help from maintainers would be appreciated.
Would a simple
ConfigMapbe enough to put the subscriptions? Then we would need a way to attach it to the cluster definition. I also don't understand well the mechanisms that would process and apply the subscriptions.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So AFAIU you can have one or more subscriptions and for each of them you have a couple of lists (metrics prefixes and filters) in addition to an interval value.
Thinking out loud we should start from the
Kafkacustom resource to have a field dedicated to the client metrics (because we already have ametricsConfigfield to support JMX and Prometheus server related metrics). If such a field is, for example,clientsMetrics(or whatever), it could be a list of subscriptions with the fields for corresponding configuration, or it could point to aConfigMap. I guess the choice could depend on how big this configuration could be but also taking into account that theConfigMapis not strongly typed (as it could be a direct API in the Kafka CR) so we should think more of how to add such configuration (i.e. a JSON?).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mimaison can the subscriptions be set at deploy time, or only set at runtime? I see here you listed the dedicated shell script, but this can be configured via the configs script as well can't it? Is it effectively a configuration option that can be updated dynamically (i.e. without rolling the brokers)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Subscriptions can only be set, updated and deleted at runtime. Yes you can also use the
kafka-configs.shtool or theincrementalAlterConfigs()Admin APIs to configure subscriptions.Exactly