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.

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?

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.

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?

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?

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.

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.

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.

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

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

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

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.

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 <ocorriga@redhat.com>

Address initial feedback Signed-off-by: ocorriga <ocorriga@redhat.com> Reverse dashboards and fix build issue Signed-off-by: ocorriga <ocorriga@redhat.com> Update derived resources Signed-off-by: ocorriga <ocorriga@redhat.com> Trigger build Signed-off-by: Federico Valeri <fedevaleri@gmail.com> Add MetricsModel interface and refactor affected classes Signed-off-by: ocorriga <ocorriga@redhat.com>

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 from 8f5b28f to 032882f Compare February 19, 2025 15:08

OwenCorrigan76 force-pushed the integrate_metrics_reporter branch from 032882f to ece3b1f Compare February 19, 2025 15:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

OwenCorrigan76 commented Jan 16, 2025 •

edited

Loading

scholzj left a comment

scholzj Jan 16, 2025

OwenCorrigan76 Jan 17, 2025

scholzj Feb 16, 2025

scholzj Jan 16, 2025

OwenCorrigan76 Jan 17, 2025 •

edited

Loading

scholzj Jan 19, 2025

scholzj Jan 16, 2025

OwenCorrigan76 Feb 19, 2025

scholzj Jan 16, 2025

OwenCorrigan76 Feb 12, 2025

scholzj Jan 16, 2025

OwenCorrigan76 Jan 17, 2025

OwenCorrigan76 commented Jan 17, 2025

scholzj Jan 19, 2025

scholzj Jan 19, 2025

scholzj Jan 22, 2025 •

edited

Loading

scholzj left a comment

scholzj Feb 16, 2025

scholzj Feb 16, 2025

scholzj Feb 16, 2025

scholzj Feb 16, 2025

scholzj Feb 16, 2025

scholzj Feb 16, 2025

OwenCorrigan76 Feb 19, 2025

scholzj Feb 19, 2025

scholzj Feb 16, 2025

fvaleri commented Feb 17, 2025 •

edited

Loading

OwenCorrigan76 commented Feb 17, 2025

scholzj commented Feb 17, 2025

	* 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

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

OwenCorrigan76 commented Jan 17, 2025

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

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

fvaleri commented Feb 17, 2025 • edited Loading

OwenCorrigan76 commented Feb 17, 2025

scholzj commented Feb 17, 2025

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