Add support for Strimzi Metrics Reporter #954

fvaleri · 2024-12-17T09:26:38Z

This patch adds support for metrics types configuration, integrating the Strimzi Metrics Reporter), and custom Prometheus JMX Exporter configuration. The custom configuration feature is needed for Cluster Operator integration, but it can also be leveraged by standalone users. See proposal 43 for more details.

fvaleri · 2024-12-17T09:27:37Z

@OwenCorrigan76 @mimaison fyi

mimaison

I've not looked at the code yet. I left comments trying to understand how this will be used.

config/application.properties

documentation/modules/proc-configuring-kafka-bridge-jmx-metrics.adoc

documentation/modules/proc-configuring-kafka-bridge-smr-metrics.adoc

scholzj

I wonder if this should go through a dedicated proposal first. The original proposal - https://github.com/strimzi/proposals/blob/main/064-prometheus-metrics-reporter.md - sadly didn't cover the bridge in sufficient depth and there will be some backward compatibility impact here or in the operators repo that needs to be carefully considered. A proposal would be the right place to discuss it and decide on the best approach.

fvaleri · 2024-12-17T16:41:16Z

there will be some backward compatibility impact here or in the operators repo that needs to be carefully considered

I agree. @ppatierno wdyt?

ppatierno

I left just some minor comments but I will review it again when the proposal is accepted (to avoid reviewing now something that will be changed later by the final proposal)

config/application.properties

CHANGELOG.md

pom.xml

This patch adds support for metrics types configuration, integrating the Strimzi Metrics Reporter, and custom Prometheus JMX Exporter configuration. The custom configuration feature is needed for Cluster Operator integration, but it can also be leveraged by standalone users. See Proposal 43 for more details. Signed-off-by: Federico Valeri <[email protected]>

fvaleri · 2025-01-27T10:27:38Z

@strimzi/maintainers proposal 92 has been accepted, and this PR is aligned, so open for review. Thanks.

scholzj · 2025-01-28T07:46:39Z

CHANGELOG.md

@@ -9,6 +9,15 @@
  * Both the `/openapi` and `/openapi/v3` endpoints return the OpenAPI v3 definition of the bridge REST API.
 * Removed script to build bridge configuration within the container. 
  It is going to be set up by the Strimzi operator within a ConfigMap and mounted as volume on the bridge pod.
+* Added support for [Strimzi Metrics Reporter](https://github.com/strimzi/metrics-reporter).
+  This is a Kafka plugin that directly exposes metrics in Prometheus format without using JMX, and can be enabled by setting `bridge.metrics=strimziMetricsReporter`.


I do not think users really understand the JMX reference here and the use of JMX is actually completely internal to what it replaces.

scholzj · 2025-01-28T07:48:38Z

documentation/modules/proc-configuring-kafka-bridge-jmx-metrics.adoc

@@ -0,0 +1,33 @@
+[id='proc-configuring-kafka-bridge-jmx-metrics-{context}']
+= Configuring JMX Exporter metrics


Suggested change

= Configuring JMX Exporter metrics

= Configuring Prometheus JMX Exporter metrics

scholzj · 2025-01-28T07:49:19Z

documentation/assemblies/assembly-kafka-bridge-config.adoc

@@ -8,11 +8,13 @@
 [role="_abstract"]
 Configure a deployment of the Kafka Bridge using configuration properties.
 Configure Kafka and specify the HTTP connection details needed to be able to interact with Kafka.
+It is possible to enable metrics in Prometheus format with JMX Prometheus Exporter or Strimzi Metrics Reporter.


Suggested change

It is possible to enable metrics in Prometheus format with JMX Prometheus Exporter or Strimzi Metrics Reporter.

It is possible to enable metrics in Prometheus format with Prometheus JMX Exporter or Strimzi Metrics Reporter.

scholzj · 2025-01-28T07:50:00Z

documentation/modules/proc-configuring-kafka-bridge-jmx-metrics.adoc

+bridge.metrics=jmxPrometheusExporter
+----
+
+Optionally, you can set a custom JMX Exporter configuration using the `bridge.metrics.exporter.config.path` property.


Suggested change

Optionally, you can set a custom JMX Exporter configuration using the `bridge.metrics.exporter.config.path` property.

Optionally, you can set a custom Prometheus JMX Exporter configuration using the `bridge.metrics.exporter.config.path` property.

scholzj · 2025-01-28T07:51:09Z

documentation/modules/proc-configuring-kafka-bridge-jmx-metrics.adoc

+./bin/kafka_bridge_run.sh --config-file=<path>/application.properties
+----
+
+With metrics enabled, you can use `GET /metrics` with the `/metrics` endpoint to retrieve Kafka Bridge metrics in Prometheus format.


Suggested change

With metrics enabled, you can use `GET /metrics` with the `/metrics` endpoint to retrieve Kafka Bridge metrics in Prometheus format.

With metrics enabled, you can scrape metrics in the Prometheus format from the `/metrics` endpoint of the Kafka Bridge.

scholzj · 2025-01-28T08:09:21Z

src/main/java/io/strimzi/kafka/bridge/http/HttpBridge.java

+        } else if (routingContext.statusCode() == HttpResponseStatus.NOT_IMPLEMENTED.code()) {
+            message = HttpResponseStatus.NOT_IMPLEMENTED.reasonPhrase();


What are we using the not-implemented code for? Not implemented is not the same as not enabled.

scholzj · 2025-01-28T08:10:21Z

src/main/java/io/strimzi/kafka/bridge/http/HttpBridge.java

+    static class NotSupportedApi extends HttpOpenApiOperation {
+        public NotSupportedApi(HttpOpenApiOperations operationId) {
+            super(operationId);
+        }
+
+        @Override
+        public void process(RoutingContext routingContext) {
+            HttpBridgeError error = new HttpBridgeError(HttpResponseStatus.GONE.code(), "OpenAPI v2 Swagger not supported");
+            HttpUtils.sendResponse(routingContext, HttpResponseStatus.GONE.code(),
+                BridgeContentType.KAFKA_JSON, JsonUtils.jsonToBytes(error.toJson()));
+        }
+    }


Is this really related to the metrics?

scholzj · 2025-01-28T08:10:47Z

src/main/java/io/strimzi/kafka/bridge/http/converter/HttpTextMessageConverter.java

@@ -43,14 +44,14 @@ public ProducerRecord<byte[], byte[]> toKafkaRecord(String kafkaTopic, Integer p
                if (!keyNode.isTextual()) {
                    throw new IllegalStateException("Because the embedded format is 'text', the key must be a string");
                }
-                key = keyNode.asText().getBytes();
+                key = keyNode.asText().getBytes(Charset.forName("UTF-8"));


Same as above, are these changes really related to the metrics? How?

scholzj · 2025-01-28T08:13:17Z

src/main/java/io/strimzi/kafka/bridge/metrics/MetricsReporter.java

Should this be rather some abstract class / interface with two implementations then this kind of union class?

currently it's the same (I guess the new file shows up because of the new metrics subpackage).
This class has the final goal to allow scraping (see scrape method) metrics from several registries: the JMX exporter one (for Kafka related metrics) and Vert.x one (for HTTP related metrics).
The weird thing is that it would contain now two registry instances which can't live together: JMX and Strimzi reporter.
Maybe this class should be more a MetricsScraper and we should have an abstrac/interface class named MetricsReporter for two different reporters?

scholzj · 2025-01-28T08:14:42Z

src/main/java/io/strimzi/kafka/bridge/metrics/StrimziCollectorRegistry.java

Should there be some registry interface or something?

ppatierno · 2025-01-28T14:08:41Z

config/application.properties

@@ -1,5 +1,16 @@
 #Bridge related settings
 bridge.id=my-bridge
+
+# uncomment the following line to enable JMX Prometheus Exporter, check the documentation for more details


Suggested change

# uncomment the following line to enable JMX Prometheus Exporter, check the documentation for more details

# uncomment the following line to enable Prometheus JMX Exporter, check the documentation for more details

ppatierno · 2025-01-28T14:12:16Z

documentation/modules/proc-configuring-kafka-bridge-smr-metrics.adoc

+Optionally, you can set a comma-separated list of regexes used to filter exposed metrics using the `kafka.prometheus.metrics.reporter.allowlist` property.
+If not set, all available metrics will be exposed.
+
+You can add any plugin configuration to the Bridge properties file using the `kafka.` prefix.


Technically, by using kafka. you are providing configuration which will be common to all the Kafka clients used by the bridge (producer, consumer, admin). You could use kafka.producer., kafka.consumer. or kafka.admin. prefixes to set configuration for a specific client. I was wondering, does the Strimzi metrics reporter work in this case?

ppatierno · 2025-01-28T14:24:17Z

src/main/java/io/strimzi/kafka/bridge/config/BridgeConfig.java

+                    && !metricsTypeValue.equals(MetricsType.STRIMZI_REPORTER.toString())) {
+                throw new IllegalArgumentException(
+                    String.format("Invalid %s configuration, choose one of %s and %s", 
+                        METRICS_TYPE, MetricsType.JMX_EXPORTER, MetricsType.STRIMZI_REPORTER)


just a nit but you are in a if condition related to metrics validation only so the error message can only be related to that, not sure how much String formatting makes sense instead of a hard-coded error message.

ppatierno · 2025-01-28T14:34:09Z

src/main/java/io/strimzi/kafka/bridge/config/BridgeConfig.java

+            if (metricsTypeValue.equals(MetricsType.STRIMZI_REPORTER.toString())) {
+                map.putIfAbsent("kafka.metric.reporters", "io.strimzi.kafka.metrics.KafkaPrometheusMetricsReporter");
+                map.putIfAbsent("kafka.prometheus.metrics.reporter.listener.enable", "false");
+                map.putIfAbsent("kafka.prometheus.metrics.reporter.allowlist", ".*");


one question ... I would expect that this part will be always set by the operator when the bridge is running within Kubernetes. We should also think about having under the overall kafka. prefix or for all specific components (i.e. do we allow the user to set different allowlist for producer, consumer, admin?).
If running on bare metal/VM are we sure that we want to set defaults instead of raising an exception for missing configuration?

ppatierno · 2025-01-28T14:40:25Z

src/main/java/io/strimzi/kafka/bridge/config/BridgeConfig.java

+    /**
+     * @return the metric system to be used in the bridge
+     */
+    public String getMetrics() {


ppatierno · 2025-01-28T14:40:53Z

src/main/java/io/strimzi/kafka/bridge/config/BridgeConfig.java

+    }
+
+    /**
+     * @return the JMX Exporter configuration file path


Suggested change

* @return the JMX Exporter configuration file path

* @return the Prometheus JMX Exporter configuration file path

ppatierno · 2025-01-28T14:45:04Z

src/main/java/io/strimzi/kafka/bridge/Application.java

+     * @throws IOException
+     */
+    private static JmxCollectorRegistry getJmxCollectorRegistry(BridgeConfig bridgeConfig) throws MalformedObjectNameException, IOException {
+        if (bridgeConfig.getJmxExporterConfigPath() != null && Files.exists(bridgeConfig.getJmxExporterConfigPath())) {


But AFAICS it falls back to use the default configuration from the YAML within the jar. It's not ignoring, or?

ppatierno · 2025-01-28T14:46:02Z

src/main/java/io/strimzi/kafka/bridge/http/HttpBridge.java

+                if (metricsReporter != null) {
+                    routerBuilder.operation(this.METRICS.getOperationId().toString()).handler(this.METRICS);
+                }


Yeah. I think that the current implementation thanks to the default Vert.x routing will just return 404 NOT FOUND if you hit the /metrics endpoint but metrics are not enabled.
I would leave the same behaviour.

ppatierno · 2025-01-28T14:55:53Z

src/main/java/io/strimzi/kafka/bridge/metrics/MetricsReporter.java

currently it's the same (I guess the new file shows up because of the new metrics subpackage).
This class has the final goal to allow scraping (see scrape method) metrics from several registries: the JMX exporter one (for Kafka related metrics) and Vert.x one (for HTTP related metrics).
The weird thing is that it would contain now two registry instances which can't live together: JMX and Strimzi reporter.
Maybe this class should be more a MetricsScraper and we should have an abstrac/interface class named MetricsReporter for two different reporters?

fvaleri requested a review from ppatierno December 17, 2024 09:26

mimaison reviewed Dec 17, 2024

View reviewed changes

config/application.properties Show resolved Hide resolved

documentation/modules/proc-configuring-kafka-bridge-jmx-metrics.adoc Show resolved Hide resolved

documentation/modules/proc-configuring-kafka-bridge-smr-metrics.adoc Show resolved Hide resolved

fvaleri force-pushed the bridge-metrics-rep branch from a211d27 to 9385323 Compare December 17, 2024 12:29

scholzj reviewed Dec 17, 2024

View reviewed changes

fvaleri force-pushed the bridge-metrics-rep branch from 4a47979 to 4d87ff9 Compare December 19, 2024 08:41

ppatierno reviewed Dec 20, 2024

View reviewed changes

config/application.properties Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

pom.xml Show resolved Hide resolved

pom.xml Outdated Show resolved Hide resolved

fvaleri force-pushed the bridge-metrics-rep branch from 4d87ff9 to 8b9e345 Compare December 20, 2024 15:13

fvaleri force-pushed the bridge-metrics-rep branch from 8b9e345 to d0eedcd Compare January 2, 2025 08:04

fvaleri added this to the 0.32.0 milestone Jan 2, 2025

fvaleri force-pushed the bridge-metrics-rep branch from ddabfb1 to 5084403 Compare January 17, 2025 17:42

fvaleri changed the title ~~Add support for the Strimzi Metrics Reporter~~ Add support for for metrics types configuration Jan 24, 2025

fvaleri changed the title ~~Add support for for metrics types configuration~~ Add support for Strimzi Metrics Reporter Jan 24, 2025

fvaleri force-pushed the bridge-metrics-rep branch from 395ceda to 01d1b74 Compare January 24, 2025 17:29

fvaleri requested review from ppatierno, mimaison and scholzj January 27, 2025 10:26

scholzj reviewed Jan 28, 2025

View reviewed changes

ppatierno mentioned this pull request Jan 28, 2025

Proposal for splitting metrics-reporter into modules strimzi/proposals#147

Open

ppatierno reviewed Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Strimzi Metrics Reporter #954

Add support for Strimzi Metrics Reporter #954

fvaleri commented Dec 17, 2024 •

edited

Loading

fvaleri commented Dec 17, 2024 •

edited

Loading

mimaison left a comment

scholzj left a comment

fvaleri commented Dec 17, 2024 •

edited

Loading

ppatierno left a comment

fvaleri commented Jan 27, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

scholzj Jan 28, 2025

ppatierno Jan 28, 2025

scholzj Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

ppatierno Jan 28, 2025

		@@ -0,0 +1,33 @@
		[id='proc-configuring-kafka-bridge-jmx-metrics-{context}']
		= Configuring JMX Exporter metrics

	= Configuring JMX Exporter metrics
	= Configuring Prometheus JMX Exporter metrics

	It is possible to enable metrics in Prometheus format with JMX Prometheus Exporter or Strimzi Metrics Reporter.
	It is possible to enable metrics in Prometheus format with Prometheus JMX Exporter or Strimzi Metrics Reporter.

	Optionally, you can set a custom JMX Exporter configuration using the `bridge.metrics.exporter.config.path` property.
	Optionally, you can set a custom Prometheus JMX Exporter configuration using the `bridge.metrics.exporter.config.path` property.

	With metrics enabled, you can use `GET /metrics` with the `/metrics` endpoint to retrieve Kafka Bridge metrics in Prometheus format.
	With metrics enabled, you can scrape metrics in the Prometheus format from the `/metrics` endpoint of the Kafka Bridge.

		} else if (routingContext.statusCode() == HttpResponseStatus.NOT_IMPLEMENTED.code()) {
		message = HttpResponseStatus.NOT_IMPLEMENTED.reasonPhrase();

	# uncomment the following line to enable JMX Prometheus Exporter, check the documentation for more details
	# uncomment the following line to enable Prometheus JMX Exporter, check the documentation for more details

	* @return the JMX Exporter configuration file path
	* @return the Prometheus JMX Exporter configuration file path

Add support for Strimzi Metrics Reporter #954

Are you sure you want to change the base?

Add support for Strimzi Metrics Reporter #954

Conversation

fvaleri commented Dec 17, 2024 • edited Loading

fvaleri commented Dec 17, 2024 • edited Loading

mimaison left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

fvaleri commented Dec 17, 2024 • edited Loading

ppatierno left a comment

Choose a reason for hiding this comment

fvaleri commented Jan 27, 2025

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fvaleri commented Dec 17, 2024 •

edited

Loading

fvaleri commented Dec 17, 2024 •

edited

Loading

fvaleri commented Dec 17, 2024 •

edited

Loading