Add support for Strimzi Metrics Reporter to the Strimzi Kafka Bridge by OwenCorrigan76 · Pull Request #11708 · strimzi/strimzi-kafka-operator

OwenCorrigan76 · 2025-07-31T14:02:53Z

Type of change

Enhancement / new feature

Description

This patch adds support for the Strimzi Metrics Reporter to Strimzi Kafka Bridge as described by the following proposals:

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

Related to #10753

We won’t initially support the CruiseControl component. To make it work, CC should be changed to expose metrics through its HTTP endpoint.

Documentation will be updated in a separate PR.

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

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

fvaleri · 2025-07-31T14:53:51Z

    "replicas", "image", "bootstrapServers", "tls", "authentication", "http", "adminClient", "consumer",
    "producer", "resources", "jvmOptions", "logging", "clientRackInitImage", "rack",
-    "enableMetrics", "livenessProbe", "readinessProbe", "template", "tracing"})
+    "enableMetrics", "livenessProbe", "metricsConfig", "readinessProbe", "template", "tracing"})


Nit: let put this right after enableMetrics, like the get/set methods.

fvaleri · 2025-07-31T14:58:00Z

+                ? StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER
+                : "disabled";


Can you add another level of alignment to these 2 rows to make it more readable?

fvaleri · 2025-07-31T14:59:26Z

+        if (model != null) {
+            printSectionHeader("Strimzi Metrics Reporter configuration");
+            writer.println("bridge.metrics=" + TYPE_STRIMZI_METRICS_REPORTER);
+            writer.println("kafka.metric.reporters=" + StrimziMetricsReporterConfig.KAFKA_CLASS);


Suggested change

writer.println("kafka.metric.reporters=" + StrimziMetricsReporterConfig.KAFKA_CLASS);

// the kafka. prefix is required by the Bridge to pass Kafka client configurations

writer.println("kafka.metric.reporters=" + StrimziMetricsReporterConfig.KAFKA_CLASS);

fvaleri · 2025-07-31T15:02:14Z

+    public KafkaBridgeConfigurationBuilder withJmxPrometheusExporter(JmxPrometheusExporterModel model, boolean isMetricsEnabled)  {
+        if (model != null || isMetricsEnabled) {
+            printSectionHeader("Prometheus Jmx Exporter configuration");
+            writer.println("bridge.metrics=jmxPrometheusExporter");


Replace jmxPrometheusExporter with JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER.

fvaleri · 2025-07-31T15:07:05Z

+        KafkaBridge resource = new KafkaBridgeBuilder(this.resource)
+                .build();
+        KafkaBridgeCluster kbc = KafkaBridgeCluster.fromCrd(Reconciliation.DUMMY_RECONCILIATION, resource, SHARED_ENV_PROVIDER);
+        assertThat((JmxPrometheusExporterModel) (kbc.metrics()), is(nullValue()));


Why the cast is needed here?

fvaleri · 2025-07-31T15:15:49Z

+    public void testWithJmxPrometheusExporterNotIsMetricsEnabled() {
+        JmxPrometheusExporterModel model = new JmxPrometheusExporterModel(
+                new KafkaBridgeSpecBuilder()
+                        .withMetricsConfig(new JmxPrometheusExporterMetricsBuilder()


Here you can use withNewJmxPrometheusExporterMetricsConfig.

fvaleri · 2025-07-31T15:17:15Z

+    }
+
+    @ParallelTest
+    public void testWithJmxPrometheusExporterIsMetricsEnabled() {


As metricsEnable is deprecated, we don't need to create any new test.

fvaleri · 2025-07-31T15:18:10Z

+    }
+
+    @ParallelTest
+    public void testWithJmxPrometheusExporterNotIsMetricsEnabled() {


The name is very confusing. What about testWithPrometheusJmxExporter?

fvaleri · 2025-07-31T15:18:38Z

+
+        assertThat(configuration, isEquivalent(
+                "bridge.id=my-bridge",
+                "bridge.metrics=jmxPrometheusExporter",


Replace jmxPrometheusExporter with JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER.

fvaleri · 2025-07-31T15:19:11Z

+                "bridge.id=my-bridge",
+                "bridge.metrics=jmxPrometheusExporter",
+                "kafka.bootstrap.servers=my-cluster-kafka-bootstrap:9092",
+                "bridge.metrics.exporter.config.path=/opt/strimzi/custom-config/metrics-config.json",


replace /opt/strimzi/custom-config/metrics-config.json with KafkaBridgeCluster.KAFKA_BRIDGE_CONFIG_VOLUME_MOUNT + JmxPrometheusExporterModel.CONFIG_MAP_KEY.

fvaleri · 2025-07-31T15:47:24Z

Grafana dashboard screenshots:

Screenshot 2025-07-31 at 17-45-50 Strimzi Kafka Bridge - Grafana

Screenshot 2025-07-31 at 17-46-08 Strimzi Kafka Bridge - Grafana

fvaleri · 2025-07-31T16:40:06Z

@OwenCorrigan76 you also need to update KafkaBridgeAssemblyOperatorTest#testCreateOrUpdateCreatesCluster with the new Annotations.ANNO_STRIMZI_IO_CONFIGURATION_HASH. Look at the test failure to see what's the new generated hash.

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

fvaleri · 2025-08-01T14:07:43Z

                .build();
        KafkaBridgeCluster kbc = KafkaBridgeCluster.fromCrd(Reconciliation.DUMMY_RECONCILIATION, resource, SHARED_ENV_PROVIDER);
-        assertThat((JmxPrometheusExporterModel) (kbc.metrics()), is(nullValue()));
+        assertThat((kbc.metrics()), is(nullValue()));


Nit: you should also remove the extra parentheses here.

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

ppatierno · 2025-08-04T10:58:49Z

+                ? JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER
+                : metrics instanceof StrimziMetricsReporterModel
+                    ? StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER
+                    : "disabled";


I do really love the ternary operator in Java but ... just at one level. I think that the code is not that much readable when it comes to start inserting one inside the other.

Agreed. Refactored.

ppatierno · 2025-08-04T11:02:24Z

+            writer.println("bridge.metrics=" + JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER);
+           // if isMetricsEnabled is not used, we pass the path of the config file. If it is used, the bridge will use the fallback config
+            if (!isMetricsEnabled) {
+                String configFilePath = KAFKA_BRIDGE_CONFIG_VOLUME_MOUNT + JmxPrometheusExporterModel.CONFIG_MAP_KEY;


avoid an additional variable here.

Agreed. Refactored.

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

scholzj · 2025-08-04T19:04:21Z

  The `strimzi.io/node-pools` and `strimzi.io/kraft` annotations are not required anymore and will be ignored if set.
 * Make properties `broker.session.timeout.ms`, `broker.heartbeat.interval.ms` and `controller.socket.timeout.ms` configurable
 * Add monitoring of custom resources using [kubernetes-state-metrics (KSM)](https://github.com/kubernetes/kube-state-metrics) (see [Strimzi proposal 087](https://github.com/strimzi/proposals/blob/main/087-monitoring-of-custom-resources.md))
+* Added support for Strimzi Metrics Reporter to the Kafka Bridge


Can you please add Connect and MM2 here as wlel? Looks like that was missed in the last PR :-/.

You should also add the deprecation of .spec.enableMetrics (in KafkaBridge) to the section below.

scholzj · 2025-08-04T19:08:13Z


    @JsonInclude(JsonInclude.Include.NON_NULL)
-    @Description("Enable the metrics for the Kafka Bridge. Default is false.")
+    @Description("Enable the metrics for the Kafka Bridge. Default is false. Deprecated, use `spec.metricsConfig`.")


I guess you should add the @Deprecated, @DeprecatedProperty(description="...", movedToPath="...") and @PresentInVersions` annotations as well. IIRc that should also automatically generate the docs so you might want to revert the description change.

Ok. Thanks.

scholzj · 2025-08-04T19:08:55Z

        return enableMetrics;
    }

+    @Deprecated


We in general deprecated the getters only. Not the setters.

Ok. Reverted.

scholzj · 2025-08-04T19:11:31Z

+        } else if (spec.getMetricsConfig() instanceof StrimziMetricsReporter) {
+            result.metrics = new StrimziMetricsReporterModel(spec, DEFAULT_METRICS_ALLOW_LIST);
+        } else {
+            result.isMetricsEnabled = spec.getEnableMetrics();


Should we bypass this and set it to JMX Exporter with default configuration directly from the cluster operator? Not a strong opinion, just a thought 🤔.

Update: maybe that makes no sense based on the later code. But at least we should rename isMetricsEnabled to something more descriptive to what it does. E.g. isLegacyMetricsConfigurationEnabled or something.

Ok. Renamed.

scholzj · 2025-08-04T19:14:17Z

+        // mapping the old deprecated enableMetrics to JMX Exporter type
+        String metricsType;
+        if ((metrics instanceof JmxPrometheusExporterModel) || isMetricsEnabled) {
+            metricsType = JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER;
+        } else {
+            if (metrics instanceof StrimziMetricsReporterModel) {
+                metricsType = StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER;
+            } else {
+                metricsType = "disabled";
+            }
+        }
+
+        if (metricsType.equals(JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER)) {
+            builder.withJmxPrometheusExporter((JmxPrometheusExporterModel) metrics, isMetricsEnabled);
+        } else if (metricsType.equals(StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER)) {
+            builder.withStrimziMetricsReporter((StrimziMetricsReporterModel) metrics);
+        }


Why exactly do we need two separate ifs here? Seems like it could be easily merged into one.

The mapping part is on a separate block because it was supposed to go away after the JMX exporter deprecation period. I think this plan is still valid, but we have the complication that Cruise Control is not supported, so it may take longer. We could keep this code as it is and open a task for adding Cruise Control support.

Why is this code better than this:

Suggested change

// mapping the old deprecated enableMetrics to JMX Exporter type

String metricsType;

if ((metrics instanceof JmxPrometheusExporterModel) || isMetricsEnabled) {

metricsType = JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER;

} else {

if (metrics instanceof StrimziMetricsReporterModel) {

metricsType = StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER;

} else {

metricsType = "disabled";

}

}

if (metricsType.equals(JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER)) {

builder.withJmxPrometheusExporter((JmxPrometheusExporterModel) metrics, isMetricsEnabled);

} else if (metricsType.equals(StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER)) {

builder.withStrimziMetricsReporter((StrimziMetricsReporterModel) metrics);

}

if ((metrics instanceof JmxPrometheusExporterModel) || isLegacyMetricsConfigEnabled)) {

builder.withJmxPrometheusExporter((JmxPrometheusExporterModel) metrics, isLegacyMetricsConfigEnabled);

} else if (metrics instanceof StrimziMetricsReporterModel) {

builder.withStrimziMetricsReporter((StrimziMetricsReporterModel) metrics);

}

Doesn't that do exactly the same?

Yep, sorry I didn't get what you mean. Done.

scholzj · 2025-08-04T19:17:38Z


    /**
-     * @return  Logging Model instance for configuring logging
+     * @return Logging Model instance for configuring logging


Having less various unnecessary whitespace changes would make the review much easier. It is especially wierd if you remove tabs from the Javadocs in one place and use them in new Javadocs in other place.

Agreed. Reverted.

scholzj · 2025-08-04T19:21:00Z

+    public KafkaBridgeConfigurationBuilder withJmxPrometheusExporter(JmxPrometheusExporterModel model, boolean isMetricsEnabled)  {
+        if (model != null || isMetricsEnabled) {
+            printSectionHeader("Prometheus Jmx Exporter configuration");
+            writer.println("bridge.metrics=" + JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER);
+           // if isMetricsEnabled is not used, we pass the path of the config file. If it is used, the bridge will use the fallback config
+            if (!isMetricsEnabled) {
+                writer.println("bridge.metrics.exporter.config.path="
+                        + KAFKA_BRIDGE_CONFIG_VOLUME_MOUNT + JmxPrometheusExporterModel.CONFIG_MAP_KEY);
+            }
+            writer.println();
+        }
+        return this;
+    }


Some more spacing can improve the readabilkty a lot ... e.g. ....

Suggested change

public KafkaBridgeConfigurationBuilder withJmxPrometheusExporter(JmxPrometheusExporterModel model, boolean isMetricsEnabled) {

if (model != null || isMetricsEnabled) {

printSectionHeader("Prometheus Jmx Exporter configuration");

writer.println("bridge.metrics=" + JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER);

// if isMetricsEnabled is not used, we pass the path of the config file. If it is used, the bridge will use the fallback config

if (!isMetricsEnabled) {

writer.println("bridge.metrics.exporter.config.path="

+ KAFKA_BRIDGE_CONFIG_VOLUME_MOUNT + JmxPrometheusExporterModel.CONFIG_MAP_KEY);

}

writer.println();

}

return this;

}

public KafkaBridgeConfigurationBuilder withJmxPrometheusExporter(JmxPrometheusExporterModel model, boolean isMetricsEnabled) {

if (model != null || isMetricsEnabled) {

printSectionHeader("Prometheus Jmx Exporter configuration");

writer.println("bridge.metrics=" + JmxPrometheusExporterMetrics.TYPE_JMX_EXPORTER);

// if isMetricsEnabled is not used, we pass the path of the config file. If it is used, the bridge will use the fallback config

if (!isMetricsEnabled) {

writer.println("bridge.metrics.exporter.config.path="

+ KAFKA_BRIDGE_CONFIG_VOLUME_MOUNT + JmxPrometheusExporterModel.CONFIG_MAP_KEY);

}

writer.println();

}

return this;

}

scholzj · 2025-08-04T19:21:09Z

+     */
+    public KafkaBridgeConfigurationBuilder withJmxPrometheusExporter(JmxPrometheusExporterModel model, boolean isMetricsEnabled)  {
+        if (model != null || isMetricsEnabled) {
+            printSectionHeader("Prometheus Jmx Exporter configuration");


Suggested change

printSectionHeader("Prometheus Jmx Exporter configuration");

printSectionHeader("Prometheus JMX Exporter configuration");

scholzj · 2025-08-04T19:23:57Z

Please update also https://github.com/strimzi/strimzi-kafka-operator/blob/main/packaging/examples/metrics/kafka-bridge-metrics.yaml as you are deprecating the enableMetrics field.

Yep. I forgot to add this one, but I have it locally. Added.

scholzj · 2025-08-04T19:32:22Z

/azp run regression

azure-pipelines · 2025-08-04T19:32:36Z

Azure Pipelines successfully started running 1 pipeline(s).

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

im-konge

LGTM from STs side.

ppatierno

Just one last nit, LGTM. Thanks!

ppatierno · 2025-08-05T11:48:03Z

+     * Configures the JMX Prometheus Metrics Exporter.
+     *
+     * @param model JMX Prometheus Metrics Exporter configuration
+     * @param isLegacyMetricsConfigEnabled Flag which indicates whether the metrics are enabled or not.


we should improve "indicates whether the metrics are enabled or not." to be clear they are enabled or not but with the legacy way. Because you can enable them by passing a model != null but the flag as false because you are using the new metrics way.

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

fvaleri · 2025-08-05T15:23:16Z

Failed tests seems to be unrelated.

scholzj · 2025-08-05T16:48:06Z

/azp run regression

azure-pipelines · 2025-08-05T16:48:19Z

Azure Pipelines successfully started running 1 pipeline(s).

OwenCorrigan76 · 2025-08-05T17:24:22Z

Thanks @fvaleri for addressing the comments above.

…trimzi#11708) Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com> Signed-off-by: Federico Valeri <fedevaleri@gmail.com> Co-authored-by: Federico Valeri <fedevaleri@gmail.com>

Added support for Strimzi Metrics Reporter to the Kafka Bridge

f5c62c1

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

OwenCorrigan76 added this to the 0.48.0 milestone Jul 31, 2025

Remove unsused enabledMetrics field

dacd104

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

fvaleri reviewed Jul 31, 2025

View reviewed changes

fvaleri force-pushed the Strimzi_Metrics_Reporter_Bridge_Integration branch from abfb9c0 to df30841 Compare July 31, 2025 15:36

Address Fede's comments

b94669f

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

OwenCorrigan76 force-pushed the Strimzi_Metrics_Reporter_Bridge_Integration branch from df30841 to b94669f Compare August 1, 2025 13:42

fvaleri approved these changes Aug 1, 2025

View reviewed changes

OwenCorrigan76 and others added 2 commits August 1, 2025 15:35

Address Nit

135675f

Signed-off-by: OwenCorrigan76 <owencorrigan76@gmail.com>

Fix broken test

2a8c049

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

ppatierno requested a review from a team August 4, 2025 10:53

ppatierno reviewed Aug 4, 2025

View reviewed changes

fvaleri added 2 commits August 4, 2025 14:07

Add back ST, example and dashboard

36ab6ef

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

Address comments

6e6dcc3

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

fvaleri force-pushed the Strimzi_Metrics_Reporter_Bridge_Integration branch from 6439a72 to 6e6dcc3 Compare August 4, 2025 12:27

scholzj reviewed Aug 4, 2025

View reviewed changes

fvaleri added 2 commits August 5, 2025 10:37

Address comments

201e746

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

Address comments

719296f

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

im-konge approved these changes Aug 5, 2025

View reviewed changes

im-konge requested a review from a team August 5, 2025 10:07

ppatierno approved these changes Aug 5, 2025

View reviewed changes

Address comments

68740c2

Signed-off-by: Federico Valeri <fedevaleri@gmail.com>

scholzj approved these changes Aug 5, 2025

View reviewed changes

scholzj merged commit 45a850c into strimzi:main Aug 5, 2025
22 checks passed

fvaleri deleted the Strimzi_Metrics_Reporter_Bridge_Integration branch August 6, 2025 06:39

scholzj added this to Roadmap Aug 23, 2025

scholzj moved this to 0.48.0 (Work in Progress) in Roadmap Aug 23, 2025

		? StrimziMetricsReporter.TYPE_STRIMZI_METRICS_REPORTER
		: "disabled";

	writer.println("kafka.metric.reporters=" + StrimziMetricsReporterConfig.KAFKA_CLASS);
	// the kafka. prefix is required by the Bridge to pass Kafka client configurations
	writer.println("kafka.metric.reporters=" + StrimziMetricsReporterConfig.KAFKA_CLASS);

	printSectionHeader("Prometheus Jmx Exporter configuration");
	printSectionHeader("Prometheus JMX Exporter configuration");

Conversation

OwenCorrigan76 commented Jul 31, 2025

Type of change

Description

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri commented Jul 31, 2025

Uh oh!

fvaleri commented Jul 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fvaleri Aug 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

fvaleri Aug 5, 2025 •

edited

Loading