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

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

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

CHANGELOG.md

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

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

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

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

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

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

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

config/application.properties

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

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

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() {


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

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

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

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

fvaleri · 2025-02-07T19:12:56Z

@ppatierno @scholzj your comments should be addressed, but there are a few open questions. For the new abstraction I used the MetricsCollector name, as MetricsReporter would create some confusion IMO. Let me know what you think.

ppatierno · 2025-02-12T10:05:03Z

@fvaleri can you resolve the conflicts before I have another pass please?

config/application.properties

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

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

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

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

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

fvaleri · 2025-02-13T11:32:06Z

Which kind of validation/spotpug error? We don't get any right now in the main. This refactoring seems to be useless from a pure code perspective.

[INFO] --- spotbugs:4.7.3.0:check (default-cli) @ kafka-bridge ---
[INFO] BugInstance size is 3
[INFO] Error size is 0
[INFO] Total bugs: 3
[ERROR] Low: Private method io.strimzi.kafka.bridge.http.HttpBridge.contentTypeToFormat(String) is never called [io.strimzi.kafka.bridge.http.HttpBridge] At HttpBridge.java:[lines 644-652] UPM_UNCALLED_PRIVATE_METHOD
[ERROR] High: Found reliance on default encoding in io.strimzi.kafka.bridge.http.converter.HttpTextMessageConverter.toKafkaRecord(String, Integer, byte[]): String.getBytes() [io.strimzi.kafka.bridge.http.converter.HttpTextMessageConverter, io.strimzi.kafka.bridge.http.converter.HttpTextMessageConverter] At HttpTextMessageConverter.java:[line 46]Another occurrence at HttpTextMessageConverter.java:[line 53] DM_DEFAULT_ENCODING
[ERROR] High: Found reliance on default encoding in io.strimzi.kafka.bridge.http.converter.HttpTextMessageConverter.toMessages(ConsumerRecords): new String(byte[]) [io.strimzi.kafka.bridge.http.converter.HttpTextMessageConverter, io.strimzi.kafka.bridge.http.converter.HttpTextMessageConverter] At HttpTextMessageConverter.java:[line 104]Another occurrence at HttpTextMessageConverter.java:[line 105] DM_DEFAULT_ENCODING

Again, why it's not happening today in the main?

I see these errors on the main branch too. Just tried with a fresh clone.

ppatierno · 2025-02-13T12:26:11Z

I see these errors on the main branch too. Just tried with a fresh clone.

Ok so I see another issue here. Our make all is not running the spotbugs target automatically and it should be fixed.
@fvaleri could you remove these changes from this PR, and opening a new one where you have those fixes and adding spotbugs as target during the make all process so that it also runs in the Azure and Travis CI pipeline please?
Then, after merging it, you can rebase this one. I am trying to have a separation of concerns here. Thanks!

fvaleri · 2025-02-13T13:02:18Z

Our make all is not running the spotbugs target automatically and it should be fixed.

The CO build does not run spotbugs as part of the all target, do we want the Bridge to be different? I can still open a dedicated PR for the spotbugs fixes, but maybe without the all target change. Just let me know.

Maybe it's the CI that should be modified to run make spotbugs? Let me check.

ppatierno · 2025-02-13T13:22:19Z

The CO build does not run spotbugs as part of the all target, do we want the Bridge to be different? I can still open a dedicated PR for the spotbugs fixes, but maybe without the all target change. Just let me know.
Maybe it's the CI that should be modified to run make spotbugs? Let me check.

Tbh I am confused now. The CO runs spotbugs as part of the Azure pipeline you are right but AFAICS it's the same for the bridge if you look at here https://github.com/strimzi/strimzi-kafka-bridge/blob/main/.azure/templates/jobs/build_java.yaml#L27
Also, last Azure runs shows no error from spotbugs https://dev.azure.com/cncf/strimzi/_build/results?buildId=183109&view=logs&j=2de397de-21fc-5e8b-587d-4865bafb3e06&t=f78e572c-4a87-5c91-14d8-0dcc4e3d1286

scholzj · 2025-02-16T19:04:36Z

@scholzj do you have anything to add here? so we can proceed with merging it?

You do not have the sufficient number of approvals, so you cannot proceed with merging it regardless of what I think.

ppatierno · 2025-02-16T19:25:08Z

I think you totally mislead what I meant. I just meant if you have anything to add which means review, leaving comments and/or approving. Then we can proceed. I wasn't going to merge without another approval, I know the rules. But because you reviewed this PR already I was just asking if you had anything more.

ppatierno · 2025-02-16T19:27:39Z

Adding that even if any other maintainer would have approved it, I was anyway going to ask you if you had anything to add despite I had all the rights to merge.

documentation/book/api/index.adoc

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

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

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

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

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

config/application.properties

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

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

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

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

ppatierno

LGTM. I left just one (hopefully last) nit. Thanks!

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

scholzj

Few more nits and questions -> but nothing major anymore.

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

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

ppatierno · 2025-02-19T07:46:55Z

@fvaleri other than resolving the remaining comments could you also rebase please? We have the fix for the Travis CI pipeline in main now. Thanks!

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]>

Signed-off-by: Federico Valeri <[email protected]>

scholzj

Thanks. LGTM.

PaulRMellor

Thanks Fede.
I left a few comments on the docs.

config/application.properties

PaulRMellor · 2025-02-19T16:07:50Z

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

 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 Prometheus JMX Exporter or Strimzi Metrics Reporter.


Suggested change

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 Prometheus JMX Exporter or Strimzi Metrics Reporter.

Configure a deployment of the Kafka Bridge with Kafka-related properties and specify the HTTP connection details needed to be able to interact with Kafka.

Additionally, enable metrics in Prometheus format using either the Prometheus JMX Exporter or the Strimzi Metrics Reporter.

Is there any difference/advantage to using one over the other that we should point out?

There are, I'm adding links so users can find them easily and we don't need to update the doc.

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

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

fvaleri · 2025-02-20T10:21:23Z

@PaulRMellor thanks for the great suggestions. Let me know if there is still some doc improvement or issue.

PaulRMellor

Thanks for dressing the comments, Fede.
Just a few nits as I re-read.

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

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

Signed-off-by: Federico Valeri <[email protected]>

ppatierno · 2025-02-20T11:14:22Z

@fvaleri thanks again for your work!

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

fvaleri force-pushed the bridge-metrics-rep branch from 8bc35ca to b7492ab Compare February 12, 2025 13:04

ppatierno requested changes Feb 13, 2025

View reviewed changes

scholzj reviewed Feb 16, 2025

View reviewed changes

fvaleri requested a review from scholzj February 17, 2025 10:52

ppatierno requested changes Feb 17, 2025

View reviewed changes

config/application.properties Outdated Show resolved Hide resolved

src/main/java/io/strimzi/kafka/bridge/http/HttpConfig.java Outdated Show resolved Hide resolved

src/main/java/io/strimzi/kafka/bridge/http/HttpBridge.java Show resolved Hide resolved

ppatierno requested changes Feb 17, 2025

View reviewed changes

fvaleri force-pushed the bridge-metrics-rep branch from 6e5e782 to 11d996c Compare February 17, 2025 14:32

ppatierno approved these changes Feb 17, 2025

View reviewed changes

src/main/java/io/strimzi/kafka/bridge/config/BridgeConfig.java Outdated Show resolved Hide resolved

fvaleri force-pushed the bridge-metrics-rep branch 2 times, most recently from a6e0a77 to 5897639 Compare February 17, 2025 17:07

scholzj requested review from PaulRMellor and removed request for scholzj February 19, 2025 00:12

scholzj reviewed Feb 19, 2025

View reviewed changes

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

src/main/java/io/strimzi/kafka/bridge/config/BridgeConfig.java Show resolved Hide resolved

fvaleri force-pushed the bridge-metrics-rep branch 2 times, most recently from f9eea03 to fcc70d4 Compare February 19, 2025 10:36

fvaleri requested a review from scholzj February 19, 2025 10:49

Address comments

d931a3b

Signed-off-by: Federico Valeri <[email protected]>

fvaleri force-pushed the bridge-metrics-rep branch from fcc70d4 to d931a3b Compare February 19, 2025 11:04

scholzj approved these changes Feb 19, 2025

View reviewed changes

PaulRMellor reviewed Feb 19, 2025

View reviewed changes

PaulRMellor approved these changes Feb 20, 2025

View reviewed changes

Improve docs

40b3c28

Signed-off-by: Federico Valeri <[email protected]>

fvaleri force-pushed the bridge-metrics-rep branch from 87e3ed8 to 40b3c28 Compare February 20, 2025 10:57

ppatierno merged commit 403bd27 into strimzi:main Feb 20, 2025
12 checks passed

fvaleri deleted the bridge-metrics-rep branch February 20, 2025 12:54

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

ppatierno Jan 28, 2025

fvaleri commented Feb 7, 2025

ppatierno commented Feb 12, 2025

fvaleri commented Feb 13, 2025

ppatierno commented Feb 13, 2025 •

edited

Loading

fvaleri commented Feb 13, 2025 •

edited

Loading

ppatierno commented Feb 13, 2025

scholzj commented Feb 16, 2025

ppatierno commented Feb 16, 2025

ppatierno commented Feb 16, 2025

ppatierno left a comment

scholzj left a comment

ppatierno commented Feb 19, 2025

scholzj left a comment

PaulRMellor left a comment

PaulRMellor Feb 19, 2025

PaulRMellor Feb 19, 2025

fvaleri Feb 20, 2025 •

edited

Loading

fvaleri commented Feb 20, 2025

PaulRMellor left a comment

ppatierno commented Feb 20, 2025

Add support for Strimzi Metrics Reporter #954

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

ppatierno Jan 28, 2025

Choose a reason for hiding this comment

fvaleri commented Feb 7, 2025

ppatierno commented Feb 12, 2025

fvaleri commented Feb 13, 2025

ppatierno commented Feb 13, 2025 • edited Loading

fvaleri commented Feb 13, 2025 • edited Loading

ppatierno commented Feb 13, 2025

scholzj commented Feb 16, 2025

ppatierno commented Feb 16, 2025

ppatierno commented Feb 16, 2025

ppatierno left a comment

Choose a reason for hiding this comment

scholzj left a comment

Choose a reason for hiding this comment

ppatierno commented Feb 19, 2025

scholzj left a comment

Choose a reason for hiding this comment

PaulRMellor left a comment

Choose a reason for hiding this comment

PaulRMellor Feb 19, 2025

Choose a reason for hiding this comment

PaulRMellor Feb 19, 2025

Choose a reason for hiding this comment

fvaleri Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

fvaleri commented Feb 20, 2025

PaulRMellor left a comment

Choose a reason for hiding this comment

ppatierno commented Feb 20, 2025

fvaleri commented Dec 17, 2024 •

edited

Loading

fvaleri commented Dec 17, 2024 •

edited

Loading

fvaleri commented Dec 17, 2024 •

edited

Loading

ppatierno commented Feb 13, 2025 •

edited

Loading

fvaleri commented Feb 13, 2025 •

edited

Loading

fvaleri Feb 20, 2025 •

edited

Loading