Split the reporter into client and server modules #73

mimaison · 2025-02-27T11:32:24Z

This implements Proposal 96 and splits the reporter into 2 modules:

client-metrics-reporter
server-metrics-reporter

This also adds support for dynamic reconfiguration (#55).

I'm still unsure about the packaging. Running mvn package produces archives under client-metrics-reporter and server-metrics-reporter. Is that sufficient? or do we want a single archive?

Signed-off-by: Mickael Maison <[email protected]>

mimaison · 2025-03-04T15:20:24Z

The CI is failing. I can reproduce locally but not sure I understand why this is happening.

Running:

mvn verify
mvn spotbugs:check

Hits the issue:

[ERROR] Failed to execute goal on project server-metrics-reporter: Could not resolve dependencies for project io.strimzi:server-metrics-reporter:jar:1.0.0-SNAPSHOT
[ERROR] dependency: io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT (compile)
[ERROR] 	Could not find artifact io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT
[ERROR] dependency: io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT (test)
[ERROR] 	Could not find artifact io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal on project server-metrics-reporter: Could not resolve dependencies for project io.strimzi:server-metrics-reporter:jar:1.0.0-SNAPSHOT
dependency: io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT (compile)
	Could not find artifact io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT
dependency: io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT (test)
	Could not find artifact io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT

But if I run:

mvn verify spotbugs:check

then it works

k-wall · 2025-03-07T17:31:33Z

README.md

@@ -37,47 +37,51 @@ The metrics reporter has the following configurations:
 ### Kafka Brokers


Should this say Kafka Brokers and Controllers?

I updated that section. I think we'll probably want further improvements to the README, it's still a bit bare bones.

see-quick · 2025-03-07T19:40:42Z

The CI is failing. I can reproduce locally but not sure I understand why this is happening.

Running:

mvn verify
mvn spotbugs:check

Hits the issue:

[ERROR] Failed to execute goal on project server-metrics-reporter: Could not resolve dependencies for project io.strimzi:server-metrics-reporter:jar:1.0.0-SNAPSHOT
[ERROR] dependency: io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT (compile)
[ERROR] 	Could not find artifact io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT
[ERROR] dependency: io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT (test)
[ERROR] 	Could not find artifact io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT
[ERROR] -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal on project server-metrics-reporter: Could not resolve dependencies for project io.strimzi:server-metrics-reporter:jar:1.0.0-SNAPSHOT
dependency: io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT (compile)
	Could not find artifact io.strimzi:client-metrics-reporter:jar:1.0.0-SNAPSHOT
dependency: io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT (test)
	Could not find artifact io.strimzi:client-metrics-reporter:jar:tests:1.0.0-SNAPSHOT

But if I run:

mvn verify spotbugs:check

then it works

I think mvn verify spotbugs:check works because during the verify phase, it shares artifacts in memory but if you try to run:

mvn verify
mvn spotbugs:check

then verify does not install artifacts to local repo .m2 and that those dependencies are not persistent for the next run of the mvn command (i.e., mvn spot bugs:check). To make those dependencies persistent, you should use mvn install -DskipTests I think.

k-wall · 2025-03-10T10:20:53Z

client-metrics-reporter/pom.xml

+    <artifactId>client-metrics-reporter</artifactId>
+
+    <dependencies>
+        <dependency>


For multi module projects, maven best practice is to express shared dependencies in the parent POM using a dependencyManagement section. You can then remove the version numbers from the dependencies in the child poms.

Thanks for the tip! I already had the versions defined in the parent POM so I removed the versions from the modules.

k-wall · 2025-03-10T10:29:51Z

...etrics-reporter/src/main/java/io/strimzi/kafka/metrics/prometheus/ClientMetricsReporter.java

    }

    @Override
    public void contextChange(MetricsContext metricsContext) {
        String prefix = metricsContext.contextLabels().get(MetricsContext.NAMESPACE);
+        if (!PREFIXES.contains(prefix)) {
+            throw new IllegalStateException("ClientMetricsReporter should only be used in Kafka servers");


ClientMetricsReporter should only be used by clients?

Yep, fixed.

k-wall · 2025-03-10T10:48:45Z

...rics-reporter/src/main/java/io/strimzi/kafka/metrics/prometheus/common/AbstractReporter.java

+
+    private static final Logger LOG = LoggerFactory.getLogger(AbstractReporter.class);
+
+    private final Map<Object, MetricWrapper> allowedMetrics = new ConcurrentHashMap<>();


I don't find the naming of otherMetrics too clear. It took me a while to grok what was going on:

You keep track of the otherMetrics so that in the case the allow list is changed, you can start to report on the newly matched metrics. You only need to do this if the implementation is reconfigurable.

Can I ask for a few more comments (class level)?

I agree with Keith, it would be better to have some comments.

I renamed otherMetrics to disallowedMetrics. I also added a few comments to this class to clarify its role. Let me know if some parts are still not clear.

k-wall · 2025-03-10T10:50:59Z

...rics-reporter/src/main/java/io/strimzi/kafka/metrics/prometheus/common/AbstractReporter.java

+     * Update the allowed metrics based on the current allowlist pattern.
+     */
+    public void updateAllowedMetrics() {
+        if (!isReconfigurable()) return;


We are guaranteed that this method is called by a single thread, right?

Yes in KRaft mode this should be called by a single thread. While this runs the addMetric() and removeMetric() methods could be called from another thread but that should be fine with ConcurrentHashMaps.

k-wall · 2025-03-10T10:55:27Z

...s-reporter/src/main/java/io/strimzi/kafka/metrics/prometheus/ServerKafkaMetricsReporter.java

+/**
+ * MetricsReporter implementation that expose Kafka server metrics in the Prometheus format.
+ */
+public class ServerKafkaMetricsReporter extends ClientMetricsReporter {


The class hierarchy is giving me pause for thought. Is ServerKafkaMetricsReporter really a ClientMetricsReporter. I wonder whether the common stuff should be pushed to the abstract class with only ServerKafkaMetricsReporter knowing about reconfigurability and 'otherMetric' tracking.

Yeah it's a bit complicated.

The allowedMetrics logic is shared across all 3 reporters (ClientMetricsReporter, ServerKafkaMetricsReporter and ServerYammerMetricsReporter) so it makes sense to have that in the abstract class.

For the reconfiguration logic (otherMetric) it's used by both ServerKafkaMetricsReporter and ServerYammerMetricsReporter. So it's still 2 out of 3 reporters hence why I put it there too. Otherwise we'd need a server abstract reporter but that seems overkill.

Signed-off-by: Mickael Maison <[email protected]>

mimaison · 2025-03-10T16:24:33Z

I think mvn verify spotbugs:check works because during the verify phase, it shares artifacts in memory but if you try to run:
1. mvn verify

2. mvn spotbugs:check
then verify does not install artifacts to local repo .m2 and that those dependencies are not persistent for the next run of the mvn command (i.e., mvn spot bugs:check). To make those dependencies persistent, you should use mvn install -DskipTests I think.

So what do you recommend? Should I add a step to run mvn install, or run verify and spotbugs:check together? What do other Strimzi projects do?

ppatierno · 2025-03-11T09:36:05Z

I was looking at the bridge as simpler compared to the operator. We have the verify and spotbugs as two separated stages anyway, you can see here:

https://github.com/strimzi/strimzi-kafka-bridge/blob/main/.azure/templates/jobs/build_java.yaml#L27

where the java_verify is a make target running mvn verify.
and then here:

https://github.com/strimzi/strimzi-kafka-bridge/blob/main/.azure/templates/jobs/build_java.yaml#L35

where the spotbugs is a make target running mvn spotbugs:check.

Signed-off-by: Mickael Maison <[email protected]>

mimaison · 2025-03-14T14:08:10Z

I was looking at the bridge as simpler compared to the operator. We have the verify and spotbugs as two separated stages anyway, you can see here:

https://github.com/strimzi/strimzi-kafka-bridge/blob/main/.azure/templates/jobs/build_java.yaml#L27

where the java_verify is a make target running mvn verify. and then here:

https://github.com/strimzi/strimzi-kafka-bridge/blob/main/.azure/templates/jobs/build_java.yaml#L35

where the spotbugs is a make target running mvn spotbugs:check.

I'm not sure I follow. What do you suggest doing?

see-quick · 2025-03-14T14:14:52Z

I think mvn verify spotbugs:check works because during the verify phase, it shares artifacts in memory but if you try to run:
1. mvn verify

2. mvn spotbugs:check
then verify does not install artifacts to local repo .m2 and that those dependencies are not persistent for the next run of the mvn command (i.e., mvn spot bugs:check). To make those dependencies persistent, you should use mvn install -DskipTests I think.
So what do you recommend? Should I add a step to run mvn install, or run verify and spotbugs:check together? What do other Strimzi projects do?

Well we simply do

      # Build the Java code without tests
      - bash: "make java_install"
        displayName: "Build Java code"
        env:
          MVN_ARGS: "-DskipTests -e -V -B"
          CURL_ARGS: "-s"

      # Run different checks on the code
      - bash: "make spotbugs"
        displayName: "Run Spotbugs"
        env:
          MVN_ARGS: "-e -V -B"

where make java_install, which will trigger java_build and then it would trigger ./build.sh and inside build.sh where in makefile we define

.PHONY: build clean release all $(SUBDIRS) $(DOCKER_TARGETS) spotbugs

so Spotbugs is part of the building process (but last to execute). I would go with mvn verify + spotbugs in one step IMHO in your case.

mimaison · 2025-03-14T14:17:22Z

I would go with mvn verify + spotbugs in one step IMHO in your case.

Yes that seems the simplest option to me. But I wanted to see what other projects did and understand if there was a good reason to have separated steps.

Signed-off-by: Mickael Maison <[email protected]>

mimaison · 2025-03-14T15:45:08Z

~~I merged both steps but it's not triggering the CI now~~
The yaml was broken, I fixed it (2c20f98) and the CI is running now.

Signed-off-by: Mickael Maison <[email protected]>

k-wall · 2025-03-19T22:48:15Z

lgtm - thanks @mimaison

mimaison mentioned this pull request Feb 27, 2025

Fix spotbugs errors and CI #74

Merged

Split the reporter into client and server modules

a479b7a

Signed-off-by: Mickael Maison <[email protected]>

mimaison force-pushed the split-modules branch from 69a96a8 to a479b7a Compare March 4, 2025 11:55

mimaison added this to the 0.2.0 milestone Mar 4, 2025

mimaison requested a review from ppatierno March 4, 2025 13:51

mimaison requested a review from k-wall March 7, 2025 10:52

k-wall reviewed Mar 7, 2025

View reviewed changes

k-wall reviewed Mar 10, 2025

View reviewed changes

Remove dependency versions from modules

c6b0e05

Signed-off-by: Mickael Maison <[email protected]>

Address feedback from Keith

b842092

Signed-off-by: Mickael Maison <[email protected]>

Merge build and spotbugs steps

bdcd088

Signed-off-by: Mickael Maison <[email protected]>

Remove extra quote in CI yaml

2c20f98

Signed-off-by: Mickael Maison <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Split the reporter into client and server modules #73

Split the reporter into client and server modules #73

mimaison commented Feb 27, 2025 •

edited

Loading

mimaison commented Mar 4, 2025

k-wall Mar 7, 2025

mimaison Mar 10, 2025

see-quick commented Mar 7, 2025

k-wall Mar 10, 2025

mimaison Mar 10, 2025

k-wall Mar 10, 2025

mimaison Mar 10, 2025

k-wall Mar 10, 2025

ppatierno Mar 11, 2025

mimaison Mar 14, 2025

k-wall Mar 10, 2025

mimaison Mar 14, 2025

k-wall Mar 10, 2025

mimaison Mar 10, 2025

mimaison commented Mar 10, 2025

ppatierno commented Mar 11, 2025

mimaison commented Mar 14, 2025

see-quick commented Mar 14, 2025 •

edited

Loading

mimaison commented Mar 14, 2025

mimaison commented Mar 14, 2025 •

edited

Loading

k-wall commented Mar 19, 2025

		@@ -37,47 +37,51 @@ The metrics reporter has the following configurations:
		### Kafka Brokers


		private static final Logger LOG = LoggerFactory.getLogger(AbstractReporter.class);

		private final Map<Object, MetricWrapper> allowedMetrics = new ConcurrentHashMap<>();

Split the reporter into client and server modules #73

Are you sure you want to change the base?

Split the reporter into client and server modules #73

Conversation

mimaison commented Feb 27, 2025 • edited Loading

mimaison commented Mar 4, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

see-quick commented Mar 7, 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

mimaison commented Mar 10, 2025

ppatierno commented Mar 11, 2025

mimaison commented Mar 14, 2025

see-quick commented Mar 14, 2025 • edited Loading

mimaison commented Mar 14, 2025

mimaison commented Mar 14, 2025 • edited Loading

k-wall commented Mar 19, 2025

mimaison commented Feb 27, 2025 •

edited

Loading

see-quick commented Mar 14, 2025 •

edited

Loading

mimaison commented Mar 14, 2025 •

edited

Loading