Add support for the Strimzi Metrics Reporter to Kafka brokers/controllers components #11051

OwenCorrigan76 · 2025-01-16T17:14:53Z

Type of change

Enhancement / new feature

Description

This patch adds support for the Strimzi Metrics Reporter to Kafka brokers/controllers components as described by the following proposal:

https://github.com/strimzi/proposals/blob/main/064-prometheus-metrics-reporter.md

Related to #10753

Support for Kafka Connect and MirrorMaker2 will be added in subsequent PRs.

Checklist

Write tests
Make sure all tests pass
Update documentation
Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
Reference relevant issue(s) and close them after merging
Update CHANGELOG.md

scholzj

I left some comments after an initial pass. You should also update the names / description / CHANGELOG to make it clear what this really does. Kafka compoenents are brokers/controllers, Connect and MM2. You add this only for brokers/controllers. It should be clear from the CHANGELOG and PR name / desc.

scholzj · 2025-01-16T17:19:04Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Support for MirrorMaker 1 has been removed
 * Added support to configure `dnsPolicy` and `dnsConfig` using the `template` sections.
 * Store Kafka node certificates in separate Secrets, one Secret per pod.
+* Added support for Strimzi Metrics Reporter to the Kafka component.


What Kafka component?

Thanks for the feedback @scholzj. I will address this.

Not addressed yet I guess?

Suggested change

* Added support for Strimzi Metrics Reporter to the Kafka component.

* Added support for Strimzi Metrics Reporter to the Kafka brokers.

Changed to Kafka brokers / controllers

packaging/examples/metrics/grafana-dashboards/strimzi-kafka.json

packaging/examples/metrics/grafana-dashboards/strimzi-kraft.json

scholzj · 2025-01-16T17:28:32Z

api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaClusterSpec.java

@@ -62,7 +62,8 @@ public class KafkaClusterSpec implements HasConfigurableMetrics, HasConfigurable
            + "cruise.control.metrics.topic, cruise.control.metrics.reporter.bootstrap.servers, "
            + "node.id, process.roles, controller., metadata.log.dir, zookeeper.metadata.migration.enable, " // KRaft options
            + "client.quota.callback.static.kafka.admin., client.quota.callback.static.produce, client.quota.callback.static.fetch, "
-            + "client.quota.callback.static.storage.per.volume.limit.min.available., client.quota.callback.static.excluded.principal.name.list";
+            + "client.quota.callback.static.storage.per.volume.limit.min.available., client.quota.callback.static.excluded.principal.name.list, "
+            + "kafka.metric.reporters, prometheus.metrics.reporter.";


Why are we disabling kafka.metric.reporters? What if someone uses it? If we want to disable it, it is definitely worth adding to CHANGELOG. But maybe it should be kept configurable?

The naming here is a little unfortunate. We are not disabling metric.reporters but we are locking down kafka.metrics.reporters as the proposal requests. metric.reporters is a Kafka config that is configurable but kafka.metrics.reporters is a Reporter config that is not configurable.

There is no option metrics.reporters. You are disabling the option kafka.metric.reporters that is a standard broker configuration option and might be used by the users. If nothing else, this deserves more discussion as it has backward compatibility implications. Was it part of the proposal? I do not remember that.

From the Kafka docs, kafka.metric.reporters seems to be a list. So are there any reasons why we can't have custom and our reporters?

You still have to remove kafka.metric.reporters here.

I missed this. I will fix

scholzj · 2025-01-16T17:29:09Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ConfigMapUtils.java

@@ -67,7 +67,7 @@ public static Map<String, String> generateMetricsAndLogConfigMapData(Reconciliat
            data.put(supportsLogging.logging().configMapKey(), supportsLogging.logging().loggingConfiguration(reconciliation, metricsAndLogging.loggingCm()));
        }

-        if (model instanceof SupportsMetrics supportMetrics) {
+        if (model instanceof SupportsMetrics supportMetrics && supportMetrics.metrics() != null) {


Why do we need this?

Now refactored

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java

scholzj · 2025-01-16T17:31:50Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+    private MetricsModel jmxExporterMetrics;
+    private StrimziMetricsReporterModel strimziMetricsReporter;


We have MetricsModel for a reason. We should try to unify both metrics types into a single class (e.g. have MetricsModel as abstract class or interface and then two implementations for the different metric types).

The latest commit deals with this and provides and interface with two implementations.

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

scholzj · 2025-01-16T17:33:32Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

+    /**
+     * The configuration field name for Kafka metric reporters.
+     */
+    public static final String KAFKA_METRIC_REPORTERS_CONFIG_FIELD = "metric.reporters";


It is not even used here. So why is it defined here?

I defined it here because this is where the other Kafka config variables were defined. I will move it into the class where it is used and make it private.

OwenCorrigan76 · 2025-01-17T12:01:44Z

@scholzj I am currently working on the changes I did not comment on yet.

CHANGELOG.md

scholzj · 2025-01-19T20:43:09Z

api/src/main/java/io/strimzi/api/kafka/model/common/metrics/StrimziMetricsReporterValues.java

+@EqualsAndHashCode()
+@ToString
+public class StrimziMetricsReporterValues implements UnknownPropertyPreserving {
+    private static final String DEFAULT_REGEX = ".*";


I think we should have the default tailor-made for each component. (Or it should be required) And the allow list should default to null to allow you to distinguish between not set and set to .*.

scholzj · 2025-01-19T20:51:41Z

api/src/main/java/io/strimzi/api/kafka/model/common/metrics/JmxPrometheusExporterMetrics.java

@@ -31,7 +30,6 @@ public class JmxPrometheusExporterMetrics extends MetricsConfig {
    private ExternalConfigurationReference valueFrom;

    @Description("ConfigMap entry where the Prometheus JMX Exporter configuration is stored. ")
-    @JsonProperty(required = true)


IIRC, you had to make it optional because of how the API is constructed. But it is required. So, we have to make sure:

It is handled in the code

Use the CEL validation (Use CEL validation in our CRDs #9417) to make this required that way (but this would need to be done right away). If you want, I can look at that when I get back to the office, but that would definitely delay this PR.

FYI: I opened #11068 for the CEL validation. Once/if merged, you would need to add the corresponding rule here.

CEL Validation added and working well

scholzj

I left some more comments. There also seem to be many unaddressed older comments. Not sure if that was intentional or not at this point.

scholzj · 2025-02-16T18:54:03Z

CHANGELOG.md

@@ -6,6 +6,7 @@
 * Support for MirrorMaker 1 has been removed
 * Added support to configure `dnsPolicy` and `dnsConfig` using the `template` sections.
 * Store Kafka node certificates in separate Secrets, one Secret per pod.
+* Added support for Strimzi Metrics Reporter to the Kafka component.


Not addressed yet I guess?

Suggested change

* Added support for Strimzi Metrics Reporter to the Kafka component.

* Added support for Strimzi Metrics Reporter to the Kafka brokers.

scholzj · 2025-02-16T18:55:50Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java

@@ -237,6 +249,10 @@ public static CruiseControl fromCrd(
        }
    }

+    private boolean hasMetricsConfig() {
+        return metrics != null && metrics.isEnabled();


Would there be some situation when it is not null and yet it is disabled?

No. && metrics.isEnabled() now removed.

scholzj · 2025-02-16T18:56:34Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java

@@ -499,7 +515,7 @@ public HashLoginServiceApiCredentials apiCredentials() {
    /**
     * @return  Metrics Model instance for configuring Prometheus metrics
     */
-    public MetricsModel metrics()   {
+    public JmxPrometheusExporterModel metrics()   {


Why does this not return the generic class?

This is still returning the concrete class. Please change to MetricsModel.

It should return the generic class. Now refatored.

scholzj · 2025-02-16T18:59:01Z

...tor/src/main/java/io/strimzi/operator/cluster/model/metrics/StrimziMetricsReporterModel.java

+     *
+     * @param spec Custom resource section configuring metrics.
+     */
+    public StrimziMetricsReporterModel(HasConfigurableMetrics spec) {


I don't understand the way this is constructed. You should check the type you have outside and create the right class given we have always only one type of metrics.

Refactored. Does it look better now?

scholzj · 2025-02-16T18:59:10Z

...ator/src/main/java/io/strimzi/operator/cluster/model/metrics/JmxPrometheusExporterModel.java

+     *
+     * @param spec       Custom resource section configuring metrics
+     */
+    public JmxPrometheusExporterModel(HasConfigurableMetrics spec) {


Same as for the other class. I don't understand the way this is constructed. You should check the type you have outside and create the right class given we have always only one type of metrics.

Refactored. Does it look better now?

scholzj · 2025-02-16T18:59:49Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/metrics/MetricsModel.java

-     */
-    public static final String METRICS_PORT_NAME = "tcp-prometheus";
-
+public interface MetricsModel {


Would this work better as an abstract class?

@scholzj Wondering, why this would be better as an abstract class? Thanks.

It is a bit hard to explain and sometimes you need to write the code to see what works better. But I feel like having an abstract class would better encapsulate the shared logic and shared fields.

I will return to this.

scholzj · 2025-02-16T19:02:51Z

packaging/examples/metrics/reporter/kafka-metrics.yaml

This directory likely needs some better name + some README with explanation.

Renamed to strimzi-metrics-reporter and added a brief README

fvaleri · 2025-02-17T07:56:46Z

There also seem to be many unaddressed older comments. Not sure if that was intentional or not at this point.

@scholzj I'm supporting Owen with this feature. He is working on and will eventually address all comments.

OwenCorrigan76 · 2025-02-17T11:15:26Z

@scholzj I know progress is going quite slowly. I will address the previous comments shortly. I'm currently working on the CEL validation. When that is committed, I'll go back and tackle the older comments and work through them.

scholzj · 2025-02-17T13:24:54Z

@scholzj I know progress is going quite slowly. I will address the previous comments shortly. I'm currently working on the CEL validation. When that is committed, I'll go back and tackle the older comments and work through them.

No worries Owen, I was just unsure what the expected state is. I did not intend to put any pressure on your. Sorry.

OwenCorrigan76 · 2025-02-25T11:02:31Z

The build failed with the following tests:
io.strimzi.api.kafka.model.kafka.KafkaCrdIT.testKafkaWithNullMaintenance
Currently investigating why.

fvaleri

Hi @OwenCorrigan76, we still need to do some changes and address the remaining comments, but it works fine.

For the KafkaCrdIT.testKafkaWithNullMaintenance failure, I think we simply need to update the expected error messages removing the invalid: prefix. Now, you get invalid: [, rather than invalid: . With CEL validation rules, we get the additional error message "validation rules were not checked".

fvaleri · 2025-02-25T08:04:02Z

api/src/main/java/io/strimzi/api/kafka/model/kafka/KafkaClusterSpec.java

@@ -62,7 +62,8 @@ public class KafkaClusterSpec implements HasConfigurableMetrics, HasConfigurable
            + "cruise.control.metrics.topic, cruise.control.metrics.reporter.bootstrap.servers, "
            + "node.id, process.roles, controller., metadata.log.dir, zookeeper.metadata.migration.enable, " // KRaft options
            + "client.quota.callback.static.kafka.admin., client.quota.callback.static.produce, client.quota.callback.static.fetch, "
-            + "client.quota.callback.static.storage.per.volume.limit.min.available., client.quota.callback.static.excluded.principal.name.list";
+            + "client.quota.callback.static.storage.per.volume.limit.min.available., client.quota.callback.static.excluded.principal.name.list, "
+            + "kafka.metric.reporters, prometheus.metrics.reporter.";


You still have to remove kafka.metric.reporters here.

api/src/main/java/io/strimzi/api/kafka/model/common/metrics/JmxPrometheusExporterMetrics.java

api/src/main/java/io/strimzi/api/kafka/model/common/metrics/StrimziMetricsReporterValues.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java

fvaleri · 2025-02-25T08:44:01Z

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/CruiseControl.java

@@ -499,7 +515,7 @@ public HashLoginServiceApiCredentials apiCredentials() {
    /**
     * @return  Metrics Model instance for configuring Prometheus metrics
     */
-    public MetricsModel metrics()   {
+    public JmxPrometheusExporterModel metrics()   {


This is still returning the concrete class. Please change to MetricsModel.

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectCluster.java

docker-images/kafka-based/kafka/scripts/kafka_connect_run.sh

docker-images/kafka-based/kafka/scripts/kafka_run.sh

packaging/examples/metrics/strimzi-metrics-reporter/kafka-metrics.yaml

fvaleri

@OwenCorrigan76 I was wrong on the "Unsupported metrics type", please correct.

api/src/main/java/io/strimzi/api/kafka/model/common/metrics/JmxPrometheusExporterMetrics.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java

cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaConnectCluster.java

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java

fvaleri

Changes looks good to me for now, but we still have some comments to address.

This patch adds support for the Strimzi Metrics Reporter to brokers and controllers as described by the following proposal: https://github.com/strimzi/proposals/blob/main/064-prometheus-metrics-reporter.md Signed-off-by: ocorriga <[email protected]>

Address initial feedback Signed-off-by: ocorriga <[email protected]> Reverse dashboards and fix build issue Signed-off-by: ocorriga <[email protected]> Update derived resources Signed-off-by: ocorriga <[email protected]> Trigger build Signed-off-by: Federico Valeri <[email protected]> Add MetricsModel interface and refactor affected classes Signed-off-by: ocorriga <[email protected]>

Signed-off-by: ocorriga <[email protected]>

OwenCorrigan76 requested review from a team and mimaison January 16, 2025 17:15

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch 2 times, most recently from e569be2 to 1180ee2 Compare January 16, 2025 17:23

scholzj reviewed Jan 16, 2025

View reviewed changes

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch from 1180ee2 to f943007 Compare January 17, 2025 12:00

fvaleri added this to the 0.46.0 milestone Jan 17, 2025

OwenCorrigan76 changed the title ~~Add support for the Strimzi Metrics Reporter to Kafka component~~ Add support for the Strimzi Metrics Reporter to Kafka brokers/controllers components Jan 17, 2025

scholzj reviewed Jan 19, 2025

View reviewed changes

scholzj mentioned this pull request Jan 22, 2025

Add support for CEL validation to our CRDs #11068

Merged

4 tasks

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch 2 times, most recently from 84fb016 to 8f5b28f Compare February 13, 2025 11:16

scholzj reviewed Feb 16, 2025

View reviewed changes

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch 3 times, most recently from ece3b1f to 75a829f Compare February 24, 2025 13:23

fvaleri reviewed Feb 25, 2025

View reviewed changes

...perator/src/main/java/io/strimzi/operator/cluster/model/KafkaBrokerConfigurationBuilder.java Outdated Show resolved Hide resolved

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch from 4304607 to f88ec2c Compare February 26, 2025 10:51

OwenCorrigan76 requested a review from fvaleri February 26, 2025 12:36

fvaleri reviewed Feb 26, 2025

View reviewed changes

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch from f88ec2c to 3c94f43 Compare February 27, 2025 15:39

ocorriga added 7 commits March 4, 2025 11:55

Add CEL validation and address comments

b94157c

Signed-off-by: ocorriga <[email protected]>

Addressing latest comments

06626e4

Signed-off-by: ocorriga <[email protected]>

add TODO in withStrimziMetricsReporter

0c1bc9f

Signed-off-by: ocorriga <[email protected]>

address latest comments

f7f912f

Signed-off-by: ocorriga <[email protected]>

Add StrimziMetricsReporterConfig class

c81fe85

Signed-off-by: ocorriga <[email protected]>

Add defaults for allowlist

3969049

Signed-off-by: ocorriga <[email protected]>

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch from 3c94f43 to 3969049 Compare March 4, 2025 12:21

OwenCorrigan76 requested a review from fvaleri March 4, 2025 14:27

	* Added support for Strimzi Metrics Reporter to the Kafka component.
	* Added support for Strimzi Metrics Reporter to the Kafka brokers.

		private MetricsModel jmxExporterMetrics;
		private StrimziMetricsReporterModel strimziMetricsReporter;

Add support for the Strimzi Metrics Reporter to Kafka brokers/controllers components #11051

Are you sure you want to change the base?

Add support for the Strimzi Metrics Reporter to Kafka brokers/controllers components #11051

Conversation

OwenCorrigan76 commented Jan 16, 2025 • edited Loading

Type of change

Description

Checklist

scholzj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenCorrigan76 Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OwenCorrigan76 commented Jan 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj Jan 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvaleri commented Feb 17, 2025 • edited Loading

OwenCorrigan76 commented Feb 17, 2025

scholzj commented Feb 17, 2025

OwenCorrigan76 commented Feb 25, 2025

fvaleri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvaleri left a comment • edited Loading

Choose a reason for hiding this comment

fvaleri left a comment

Choose a reason for hiding this comment

OwenCorrigan76 commented Jan 16, 2025 •

edited

Loading

OwenCorrigan76 Jan 17, 2025 •

edited

Loading

scholzj Jan 22, 2025 •

edited

Loading

fvaleri commented Feb 17, 2025 •

edited

Loading

fvaleri left a comment •

edited

Loading