Enable more MongoDB Micrometer metrics #44999

vkn · 2024-12-09T09:56:38Z

Currently only connection pool micrometer metrics are supported. This PR enables all available metrics based on io.micrometer.core.instrument.binder.mongodb.MongoMetricsCommandListener

All unit tests in mongodb-client extension are based on microprofile metrics, so it was not possible to add tests for micrometer, without modifying dependencies and existing tests. However I've added an assertion in MongodbPanacheResourceTest.java in integration-tests/mongodb-panache module

gsmet

Thanks for the PR! I added a small comment inline.

I will defer to Loïc and Bruno to check the feature.

gsmet · 2024-12-10T13:10:23Z

...godb-client/deployment/src/main/java/io/quarkus/mongodb/deployment/MongoClientProcessor.java

+        return metricsCapability
+                .filter(cap -> buildTimeConfig.metricsEnabled && cap.metricsSupported(MetricsFactory.MICROMETER))
+                .map(supported -> new AdditionalIndexedClassesBuildItem(
+                        MongoClientRecorder.getMicrometerCommandListenerClassName()))
+                .orElseGet(AdditionalIndexedClassesBuildItem::new);


Any chance you could implement this with simple ifs (with early returns for edge cases)?

Because this is a bit hard to follow and I'm worried we will regret it when fixing a bug one early morning without coffee :).

@gsmet pushforced the change

brunobat

It looks ok to me but could you please add a test entry here:

quarkus/extensions/mongodb-client/deployment/src/test/java/io/quarkus/mongodb/MongoMetricsTest.java

Line 24 in ebee0a4

public class MongoMetricsTest extends MongoTestBase {

vkn · 2024-12-10T13:36:59Z

It looks ok to me but could you please add a test entry here:

quarkus/extensions/mongodb-client/deployment/src/test/java/io/quarkus/mongodb/MongoMetricsTest.java

Line 24 in ebee0a4

public class MongoMetricsTest extends MongoTestBase {

@brunobat the test is based on microprofile metrics. to add test for the micrometer metrics, we would have to refactor tests and create separate tests for micrometer and microprofile metrics. or do you see an easier way

brunobat · 2024-12-10T13:43:33Z

It looks ok to me but could you please add a test entry here:

quarkus/extensions/mongodb-client/deployment/src/test/java/io/quarkus/mongodb/MongoMetricsTest.java

Line 24 in ebee0a4

public class MongoMetricsTest extends MongoTestBase {

@brunobat the test is based on microprofile metrics. to add test for the micrometer metrics, we would have to refactor tests and create separate tests for micrometer and microprofile metrics. or do you see an easier way

yeah, sorry, didn't look at the imports... Then It's worse than I thought.
Can you please add a test class for Micrometer under the deployment module?

vkn · 2024-12-10T14:08:10Z

It looks ok to me but could you please add a test entry here:

quarkus/extensions/mongodb-client/deployment/src/test/java/io/quarkus/mongodb/MongoMetricsTest.java

Line 24 in ebee0a4

public class MongoMetricsTest extends MongoTestBase {

@brunobat the test is based on microprofile metrics. to add test for the micrometer metrics, we would have to refactor tests and create separate tests for micrometer and microprofile metrics. or do you see an easier way

yeah, sorry, didn't look at the imports... Then It's worse than I thought. Can you please add a test class for Micrometer under the deployment module?

@brunobat The deployment module pom has quarkus-smallrye-metrics-deployment dependency. so microprofile metrics are on the class path and there is no way to add micrometer metrics, unless you agree to remove it from the pom xml and add the dependency in QuarkusUnitTest programmatically. The existing test have to be then adjusted and not import microprofile metrics, as far as I can tell

loicmathieu · 2024-12-10T14:12:53Z

The deployment module pom has quarkus-smallrye-metrics-deployment dependency. so microprofile metrics are on the class path and there is no way to add micrometer metrics,

Mongodb metrics support preclude our switch to Micrometer by default that's why the test is based on Microprofile metrics, I'm OK to switch to test to Micrometer.

@vkn thanks a lot for your contribution. MongoDB could emit a lot of commands, I wonder if this listener should be added always of if we could be able to disable it for performance reason.

loicmathieu

As other already said, a test would be great

loicmathieu · 2024-12-10T14:19:30Z

...ons/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClientRecorder.java

+    public static String getMicrometerCommandListenerClassName() {
+        return MicrometerCommandListener.class.getName();
+    }
+


As this is used in a single place you can directly add the class name at the use site.
This would also avoid loading the class at runtime if not used wich would be better.

As this is used in a single place you can directly add the class name at the use site. This would also avoid loading the class at runtime if not used wich would be better.

micrometer wasn't on class path in deployment module, that's why I've put it there. I'll add optional dependency and move the code to the deployment

vkn · 2024-12-10T15:44:50Z

@brunobat @loicmathieu
I've changed the tests to be based on micrometer. MongoMetricsTest verifies the new mongodb.driver.commands metrics are available. Please have a look

extensions/mongodb-client/deployment/pom.xml

github-actions · 2024-12-11T13:11:02Z

🎊 PR Preview 0396c33 has been successfully built and deployed to https://quarkus-pr-main-44999-preview.surge.sh/version/main/guides/

Images of blog posts older than 3 months are not available.
Newsletters older than 3 months are not available.

brunobat

Looks good to me.
Thanks @vkn !

gsmet

Thanks for the adjustment. I requested a small change as we shouldn't produce an empty AdditionalIndexedClassesBuildItem when we don't want to produce anything.

Once this is fixed, it's all good to go.

I can do it if you prefer but I thought you might be interested in doing it yourself. Let me know!

gsmet · 2024-12-17T08:59:57Z

...godb-client/deployment/src/main/java/io/quarkus/mongodb/deployment/MongoClientProcessor.java

+            MongoClientBuildTimeConfig buildTimeConfig,
+            Optional<MetricsCapabilityBuildItem> metricsCapability) {
+        if (!buildTimeConfig.metricsEnabled) {
+            return new AdditionalIndexedClassesBuildItem();


Yeah, I wouldn't do that. Even if it doesn't complain right now, that's certainly something that could break later.

What you need to do is change the method to return void and inject a BuildProducer<AdditionalIndexedClassesBuildItem> additionalIndexedClasses and call the produce() method when it makes sense (and return; if you want to exit early).

@gsmet I've adjusted the code as you requested. Thanks!

Even if it doesn't complain right now, that's certainly something that could break later

Addressed.

quarkus-bot · 2024-12-17T13:07:10Z

Status for workflow `Quarkus Documentation CI`

This is the status report for running Quarkus Documentation CI on commit be1fa40.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

quarkus-bot · 2024-12-17T13:11:47Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit be1fa40.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

quarkus-bot bot added the area/mongodb label Dec 9, 2024

gsmet requested review from loicmathieu and brunobat December 10, 2024 13:07

gsmet reviewed Dec 10, 2024

View reviewed changes

brunobat added the area/metrics label Dec 10, 2024

Enable mongodb micrometer metrics

304406f

vkn force-pushed the mongodb-micrometer-metrics branch from e3d1e69 to 304406f Compare December 10, 2024 13:32

brunobat requested changes Dec 10, 2024

View reviewed changes

loicmathieu previously requested changes Dec 10, 2024

View reviewed changes

vkn requested review from loicmathieu and brunobat December 10, 2024 15:48

brunobat reviewed Dec 10, 2024

View reviewed changes

extensions/mongodb-client/deployment/pom.xml Outdated Show resolved Hide resolved

vkn force-pushed the mongodb-micrometer-metrics branch from 4659ab5 to dc0689b Compare December 11, 2024 12:38

This comment has been minimized.

Sign in to view

Migrate tests to micrometer metrics

e24f716

vkn force-pushed the mongodb-micrometer-metrics branch from dc0689b to e24f716 Compare December 11, 2024 13:42

This comment has been minimized.

Sign in to view

brunobat approved these changes Dec 11, 2024

View reviewed changes

gsmet changed the title ~~Enable more mongodb micrometer metrics~~ Enable more MongoDB Micrometer metrics Dec 17, 2024

gsmet requested changes Dec 17, 2024

View reviewed changes

Avoid producing empty AdditionalIndexedClassesBuildItem

be1fa40

Even if it doesn't complain right now, that's certainly something that could break later

vkn force-pushed the mongodb-micrometer-metrics branch from f29ef89 to be1fa40 Compare December 17, 2024 10:52

vkn requested a review from gsmet December 17, 2024 11:28

gsmet approved these changes Dec 17, 2024

View reviewed changes

brunobat merged commit 849ec70 into quarkusio:main Dec 17, 2024
24 checks passed

quarkus-bot bot added this to the 3.18 - main milestone Dec 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable more MongoDB Micrometer metrics #44999

Enable more MongoDB Micrometer metrics #44999

vkn commented Dec 9, 2024

gsmet left a comment

gsmet Dec 10, 2024

vkn Dec 10, 2024

brunobat left a comment

vkn commented Dec 10, 2024

brunobat commented Dec 10, 2024

vkn commented Dec 10, 2024

loicmathieu commented Dec 10, 2024

loicmathieu left a comment

loicmathieu Dec 10, 2024

vkn Dec 10, 2024

vkn commented Dec 10, 2024

This comment has been minimized.

github-actions bot commented Dec 11, 2024 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brunobat left a comment

gsmet left a comment

gsmet Dec 17, 2024

vkn Dec 17, 2024

quarkus-bot bot commented Dec 17, 2024

quarkus-bot bot commented Dec 17, 2024 •

edited by github-actions bot

Loading

Enable more MongoDB Micrometer metrics #44999

Enable more MongoDB Micrometer metrics #44999

Conversation

vkn commented Dec 9, 2024

gsmet left a comment

Choose a reason for hiding this comment

gsmet Dec 10, 2024

Choose a reason for hiding this comment

vkn Dec 10, 2024

Choose a reason for hiding this comment

brunobat left a comment

Choose a reason for hiding this comment

vkn commented Dec 10, 2024

brunobat commented Dec 10, 2024

vkn commented Dec 10, 2024

loicmathieu commented Dec 10, 2024

loicmathieu left a comment

Choose a reason for hiding this comment

loicmathieu Dec 10, 2024

Choose a reason for hiding this comment

vkn Dec 10, 2024

Choose a reason for hiding this comment

vkn commented Dec 10, 2024

This comment has been minimized.

github-actions bot commented Dec 11, 2024 • edited Loading

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

brunobat left a comment

Choose a reason for hiding this comment

gsmet left a comment

Choose a reason for hiding this comment

gsmet Dec 17, 2024

Choose a reason for hiding this comment

vkn Dec 17, 2024

Choose a reason for hiding this comment

quarkus-bot bot commented Dec 17, 2024

Status for workflow Quarkus Documentation CI

quarkus-bot bot commented Dec 17, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

github-actions bot commented Dec 11, 2024 •

edited

Loading

Status for workflow `Quarkus Documentation CI`

quarkus-bot bot commented Dec 17, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`