Proposal for splitting metrics-reporter into modules #147
Conversation
Signed-off-by: Mickael Maison <[email protected]>
|
|
||
| I propose splitting the project into 2 Java modules: | ||
|
|
||
| 1. `client-metrics-reporter`: This module will only depend on kafka-clients and the Prometheus libraries. It will be used by Apache Kafka clients (Producer, Admin, Consumer, Connect and Streams) by setting the `metric.reporters` configuration to `io.strimzi.kafka.metrics.ClientMetricsReporter`. All the existing metrics-reporter configurations will stay the same. The differences are: |
There was a problem hiding this comment.
Why not KafkaClientMetricsReporter, KafkaServerMetricsReporter and YammerServerMetricsReporter? It just reads better.
There was a problem hiding this comment.
Happy to change the names if people prefer your suggestions. The original names also included Prometheus, I decided to drop it to have shorter names, but some people may prefer keeping that.
There was a problem hiding this comment.
Maybe we can add prometheus to the package name? That would keep E.g. io.strimzi.kafka.metrics.prometheus.ClientMetricsReporter. (I suspect we might need other technologies as well one (maybe not so distant) day :-/)
There was a problem hiding this comment.
we might need other technologies as well
In that case, I would expect a config parameter to configure the output format with a default value set to Prometheus for backwards compatibility.
There was a problem hiding this comment.
If we are to add extra reporters in the future, I expect users would pick the format by using different classes. For example if you use io.strimzi.kafka.metrics.prometheus.ClientMetricsReporter the export format is Prometheus. So I wouldn't add a new configuration to the reporter itself.
There was a problem hiding this comment.
In that case, I would expect a config parameter to configure the output format with a default value set to Prometheus for backwards compatibility.
It is not only about the output format. It could be for example push-based model instead of pull-based like Prometheus has. So pretty much all the configuration would be different. Instead of listening on some port, you would connect somewhere to send the metrics there. That might be too complicated to just have it all in the same class.
There was a problem hiding this comment.
Ok, makes sense. Didn't know Prom had push-based model.
There was a problem hiding this comment.
I don't think it is Prometheus itself - it would be more in the OpenTelemetry realm. But I think the push-based metrics are becoming more common.
There was a problem hiding this comment.
AFAIK, there is Prometheus Agent which is able to use the push-based model so writing metrics remotely as well.
Regarding the naming I am fine with ClientMetricsReporter but maybe within a prometheus package as suggested. It would be clearly Kafka even because the FQDM has kafka in the package as well.
There was a problem hiding this comment.
I like the suggestion of having a prometheus package. If we have consensus on that point, I'll make the change.
|
|
||
| Another benefit of having separate modules is the ability to enforce dependencies. Depending on where the reporter runs, a Kafka client or a Kafka server, different dependencies are available at runtime. We need to make sure that logic used by clients does not load any classes only available on servers. We hit this issue in the past ([issue #48](https://github.com/strimzi/metrics-reporter/issues/48)). This is not something that can easily be tested so currently it relies on maintainers being diligent when making code changes to ensure we don't hit this issue again in the future. | ||
|
|
||
| I'm also considering adding support for [KIP-714](https://cwiki.apache.org/confluence/display/KAFKA/KIP-714%3A+Client+metrics+and+observability) to metrics-reporter. Again this feature is only implemented by server-side metrics reporters. Adding support for this feature will be addressed in a separate proposal. |
There was a problem hiding this comment.
This is a bit confusing for me. Shouldn't this be client-side only as the clients send the metrics through this KIP? Or maybe I misunderstood things? This might anyway deserve clarification (or removal as I think you have enough justification even without it 😉).
There was a problem hiding this comment.
The Kafka client has all the logic to send its metrics built-in. To support KIP-714, brokers must run a metrics reporter implementation that support the feature (by also implementing the ClientTelemetry interface).
There was a problem hiding this comment.
So, in order to support the KIP-714, the server module would in theory read the metrics from the clients and expose them as Prometheus metrics in the Kafka broker?
There was a problem hiding this comment.
Ahh, ok. That makes sense (or it actually does not in the bigger picture, but I get the context for the module split now and understand it :-D).
|
|
||
| I propose splitting the project into 2 Java modules: | ||
|
|
||
| 1. `client-metrics-reporter`: This module will only depend on kafka-clients and the Prometheus libraries. It will be used by Apache Kafka clients (Producer, Admin, Consumer, Connect and Streams) by setting the `metric.reporters` configuration to `io.strimzi.kafka.metrics.ClientMetricsReporter`. All the existing metrics-reporter configurations will stay the same. The differences are: |
There was a problem hiding this comment.
Can you please clarify how this will work for Connect / MM2? They use a lot of clients. But they also have their own metrics (connectors and tasks and their state, etc.). So it seems like the client reporter is not enough to provide these, or?
There was a problem hiding this comment.
The Connect runtime and Streams are considered clients. To collect their metrics you would still follow the instructions from https://github.com/strimzi/metrics-reporter?tab=readme-ov-file#kafka-connect-and-kafka-streams but use io.strimzi.kafka.metrics.ClientMetricsReporter as the class name.
There was a problem hiding this comment.
So essentially, when used in Connect, the client reporter will automatically handle the additional Connect metrics as well?
There was a problem hiding this comment.
Yes, like io.strimzi.kafka.metrics.KafkaPrometheusMetricsReporter does at the moment.
Co-authored-by: Jakub Scholz <[email protected]> Signed-off-by: Mickael Maison <[email protected]>
|
|
||
| I propose splitting the project into 2 Java modules: | ||
|
|
||
| 1. `client-metrics-reporter`: This module will only depend on kafka-clients and the Prometheus libraries. It will be used by Apache Kafka clients (Producer, Admin, Consumer, Connect and Streams) by setting the `metric.reporters` configuration to `io.strimzi.kafka.metrics.ClientMetricsReporter`. All the existing metrics-reporter configurations will stay the same. The differences are: |
There was a problem hiding this comment.
AFAIK, there is Prometheus Agent which is able to use the push-based model so writing metrics remotely as well.
Regarding the naming I am fine with ClientMetricsReporter but maybe within a prometheus package as suggested. It would be clearly Kafka even because the FQDM has kafka in the package as well.
| In addition of `metrics-reporter`, this will also impact: | ||
|
|
||
| - `strimzi-kafka-operator`: There is a PR ongoing to add support for the metrics-reporter. This will need to be updated to use the new class names and dependency. | ||
| - `strimzi-kafka-bridge`: The proposal to add support for the metrics-reporter is not impacted, but the implementation will need to be updated to use the new class names and dependency. |
There was a problem hiding this comment.
@fvaleri what should we do with strimzi/strimzi-kafka-bridge#954 then? Should we wait for this proposal being accepted, implemented and metrics reporter released? Or @mimaison this is going to take more time so it's bettere to proceed with a first metrics reporter integration within the bridge?
There was a problem hiding this comment.
If you are close to merge the PR, then you go ahead.
This proposal is not accepted yet, then we need to merge the code, and do a release.
This proposes splitting the metrics-reporter project into client and server modules so it's easier to evolve in the future.