Set a injected CDI CommandListener to the mongodb configuration. #12082

juazugas · 2020-09-14T14:33:12Z

This implementation allows injecting a CommandListener to the current MongoDB client configuration via CDI bean definition.
The idea came from using the MP opentracing library and trying to include the mongodb tracing spans.

Following the instructions in the OpenTracing Mongo Driver Instrumentation there is a way to include the tracing information simply adding a CommandListener to the mongoclient settings.

I prepared an example to show the MongoDB tracing:
https://github.com/juazugas/quarkus-mongodb-opentracing/blob/master/src/main/java/com/example/mongodb/tracing/TracingCommandListenerProducer.java
on project sample https://github.com/juazugas/quarkus-mongodb-opentracing/

gsmet

I added a few small comments.

gsmet · 2020-09-14T15:13:13Z

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

                List<ConnectionPoolListener> connectionPoolListeners = new ArrayList<>(connectionPoolListenerSuppliers.size());
                for (Supplier<ConnectionPoolListener> item : connectionPoolListenerSuppliers) {
                    connectionPoolListeners.add(item.get());
                }
+                final CommandListener commandListener = Arc.container().instance(CommandListener.class).get();


Shouldn't we support multiple CommandListeners given it looks like MongoDB supports it?

Agree. The limitation is on my knowledge of how to retrieve the List of CDI beans of a given type. any help?

Set<Bean<?>> beans = Arc.container().beanManager().getBeans(CommandListener.class); will return all beans of type CommandListener.

Then you can get the bean from Arc by type if the name is null or by name.
You can find an example here but there may be an easier way to do it if you don't rely on CDI but instead use Jandex and retrieve all implementors of CommandListener.

gsmet · 2020-09-14T15:13:41Z

...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClientSupport.java

+        return commandListeners;
+    }
+
+    public void setCommandListenerList(List<CommandListener> commandListenersList) {


I would drop this method, it's useless, right?

It depends on the above comment (#12082 (comment)). If we need to add a single CommandListener or a full list. But finally it should exist only the one that it's used.

gsmet · 2020-09-14T15:14:22Z

...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClientSupport.java

+    }
+
+    public void addCommandListener(CommandListener commandListener) {
+        if (null != commandListener) {


You already test the caller, I think we can drop this test here and consider it will be called with a non-null commandListener.

gsmet · 2020-09-14T15:15:48Z

@loicmathieu @pavolloffay WDYT about this? I wonder if we should somehow integrate things better with OpenTracing to just have a boolean configuration to enable tracing?

I have the same question for the JDBC driver implementation btw.

loicmathieu · 2020-09-14T15:21:37Z

This PR raises two questions:

If we automatically add CommandListener, people may ask for the other types of listeners
Why not discovering the listeners from Jandex as we did for Discriminators and Codecs ? Note that in this case they will not be CDI beans so you will not be able to inject dependencies on them

For opentracing support, I would rather prefere activating it via a boolean property (with a check to verify that smallrye-opentracing is in the classpath).

For the JDBC support it's a different story as it's already configured via the inclusion of the library + some config property (JDBC driver name).

juazugas · 2020-09-14T15:35:31Z

This PR raises two questions:

If we automatically add CommandListener, people may ask for the other types of listeners

The current approach is based on the user definition via CDI of the CommandListener that the application would like to be injected.

Why not discovering the listeners from Jandex as we did for Discriminators and Codecs ? Note that in this case they will not be CDI beans so you will not be able to inject dependencies on them

+1 That's another good approach, define the specific TracingCommandListener "the same way" as the metrics to the ConnectionListeners are defined. We could define more settings for the Listener or even there could be more configuration values specifying the commands to exclude from the tracing. (https://github.com/opentracing-contrib/java-mongo-driver#exclude-commands-from-tracing)

For opentracing support, I would rather prefere activating it via a boolean property (with a check to verify that smallrye-opentracing is in the classpath).

+1 to that approach ^^

extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClients.java

juazugas · 2020-09-20T18:16:45Z

@gsmet @loicmathieu
In reference to the comments above (#12082 (comment))

For opentracing support, I would rather prefere activating it via a boolean property (with a check to verify that smallrye-opentracing is in the classpath).

I've prepared another branch with the suggestions (the branch extends from the one of this PR).

The tracing of the Mongo driver commands now checks the build property quarkus.mongodb.tracing.enabled and when is set to true and the opentracing capability is active then the TracingCommandListener is prepared and injected into the MongoDB settings.

The comparison of the changes can be seen on:
master...juazugas:ext-mongodb-opentracing-byconfig

As a working example of this branch, the project mongodb-opentracing/ext-mongodb-opentracing-byconfig has the property quarkus.mongo.tracing.enabled set to true.

pavolloffay · 2020-09-22T08:59:58Z

@loicmathieu @pavolloffay WDYT about this? I wonder if we should somehow integrate things better with OpenTracing to just have a boolean configuration to enable tracing?

You should consider that OpenTracing is basically in the maintenance mode, it will be sunsetted once OpenTelemetry is released. Maybe it's better to wait until OTEL GA is released and then do the integration?

loicmathieu · 2020-09-23T07:42:04Z

@pavolloffay yesterday Ken Finnigan told me that OpenTelemetry will be integrated in Quarkus by the end of the year.
And that Quarkus will support the two during some time.

So it makes sense to me to integrate MongoDB with OpenTracing while waiting for OpenTelemetry support.

loicmathieu

My main point is whether or not the CommandListener needs to comes from CDI (so we can inject dependencies in it) or from Jandex (so we can retrieve it easily but injection will not work for it).
Everything else is build from CDI for MongoDB

loicmathieu · 2020-09-23T07:51:49Z

...ions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClientSupport.java

+        this.commandListeners = new ArrayList<>();
+        this.disableSslSupport = disableSslSupport;
+    }
+
+    public MongoClientSupport(List<String> codecProviders, List<String> bsonDiscriminators,
+            List<ConnectionPoolListener> connectionPoolListeners, List<CommandListener> commandListeners,
+            boolean disableSslSupport) {
+        this.codecProviders = codecProviders;
+        this.bsonDiscriminators = bsonDiscriminators;
+        this.connectionPoolListeners = connectionPoolListeners;
+        this.commandListeners = commandListeners;


I don't think this is needed to provides methods with and without the List of CommandListener. This method is only used inside the deployment module so it's signature can be modified.

loicmathieu · 2020-09-23T07:53:08Z

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

+                if (null != commandListener) {
+                    mongoClientSupport.addCommandListener(commandListener);
+                }


This should not be needed, just pass the Command listener list as we do for all other MongoDB related classes

loicmathieu · 2020-09-23T07:57:04Z

@juazugas regarding what you done here master...juazugas:ext-mongodb-opentracing-byconfig there is no need for a CDI Command Listener so I would implement the retrieval of the Command Listener list from Jandex (as we did for BsonDiscriminators and other MongoDB related classes).

juazugas · 2020-09-23T15:32:43Z

@juazugas regarding what you done here master...juazugas:ext-mongodb-opentracing-byconfig there is no need for a CDI Command Listener so I would implement the retrieval of the Command Listener list from Jandex (as we did for BsonDiscriminators and other MongoDB related classes).

@loicmathieu
Exactly!, that branch is meant to override the one in this PR. No CDI injection of command listener/s and if enabled by the config a specific tracing command listener is created for the mongo client configuration.
On that branch, there's no command listener injected by CDI (maybe I chose a bad name for the MongoTracingCommandListenerProducer ). However, the TracingCommandListener cannot be instantiated from the constructor like the CodecProviders or registered like the BsonDiscriminator, the tracer is needed to be instantiated.
What could be done (I think it is already registered at that stage) is to extract it from the INSTANCE of the GlobalTracer, so no access using CDI would be done (like in MongoMetricsConnectionPoolListener).

loicmathieu · 2020-09-23T15:39:50Z

OK, I propose to make this in two phases.
First, go with this PR without CDI.
Then open a PR to deal with OpenTracing.

juazugas · 2020-09-23T16:02:42Z

First, go with this PR without CDI.

Ok. Search the implementors and add them after default constructor instantiation.

Then open a PR to deal with OpenTracing.

Super! baking new PR. 😁

loicmathieu · 2020-09-30T11:12:50Z

extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClients.java

@@ -249,6 +250,12 @@ private MongoClientSettings createMongoConfiguration(MongoClientConfig config) {
                CodecRegistries.fromProviders(providers));
        settings.codecRegistry(registry);

+        List<CommandListener> listeners = new ArrayList<>();


You create a List fo CommandListener in the getCommandListener method so this one is not needed.

loicmathieu · 2020-09-30T11:41:55Z

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

+        assertThat(startedEvent, notNullValue());
+        assertThat(startedEvent.getCommandName(), anyOf(equalTo("listDatabases"), equalTo("endSessions")));
+    }


Having the insertions here is not realy readable for a test.
Can you instead register the CommandEvent in a static List and assert in the test that the event is present inside the list ?

loicmathieu

Please stash all your commits then I'll approve this PR

loicmathieu · 2020-09-30T14:14:37Z

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


 import com.mongodb.event.CommandListener;
 import com.mongodb.event.CommandStartedEvent;

 public class MockCommandListener implements CommandListener {

-    private CommandStartedEvent commandStartedEvent;
+    public static final List<String> events = new ArrayList<>();


constants should be uppercase

loicmathieu

LGTM

loicmathieu · 2020-10-01T15:32:47Z

@gsmet this PR changed to only allow to setup a CommandListener, not via CDI as for other MongoDB component.

It will then enable to implement an OpenTracing CommandListener for MongoDB.

gsmet

Added some comments inline.

...-client/deployment/src/main/java/io/quarkus/mongodb/deployment/CommandListenerBuildItem.java

gsmet · 2020-10-08T08:01:21Z

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

+    }
+
+    @BuildStep
+    List<ReflectiveClassBuildItem> addCodecsAndDiscriminatorsAndListenersToNative(CodecProviderBuildItem codecProviders,


Maybe rename the method to addExtensionPointsToNative ?

gsmet · 2020-10-08T08:03:26Z

extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClients.java

+        List<CommandListener> listeners = new ArrayList<>();
+        for (String name : classNames) {
+            try {
+                Class<?> clazz = Thread.currentThread().getContextClassLoader().loadClass(name);


Could we use Class.forName here instead?

Ok. Tested on a project with the jar and the native image does not make the difference, I suppose because we do
the initialization immediately after loading the class.

gsmet · 2020-10-08T08:03:45Z

extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClients.java

@@ -249,6 +250,8 @@ private MongoClientSettings createMongoConfiguration(MongoClientConfig config) {
                CodecRegistries.fromProviders(providers));
        settings.codecRegistry(registry);

+        settings.commandListenerList(getCommandListeners(mongoClientSupport.getCommandListeners()));


Maybe call this one commandListeners?

I'm not sure about this suggestion.

The settings class is from mongodb driver (com.mongodb.MongoClientSettings.Builder)

The MongoClientSupport class has all its fields exposed to getter methods

The private method getCommandListeners is called after the same naming pattern as the getCodeProviders method.

It could be ... extracting the call to the getter function to a variable called commandListeners (very likely to happen in a future PR). For example:

List<CommandListeners> commandListeners = getCommandListeners(mongoClientSupport.getCommandListeners()); settings.commandListenerList(commandListeners);

gsmet · 2020-10-12T14:57:28Z

Hmmm, I don't know what was merged that is causing so many conflicts but this patch will need a big rebase.

Please ping me when done so that I can have a final look. Thanks!

juazugas · 2020-10-12T15:33:43Z

Hmmm, I don't know what was merged that is causing so many conflicts but this patch will need a big rebase.

Please ping me when done so that I can have a final look. Thanks!

Ping @gsmet, the rebase is done.

gsmet

I adjusted the Class.forName() call a bit and squashed everything.

Looks good now, thanks!

gastaldi · 2020-10-19T20:49:56Z

...-client/deployment/src/main/java/io/quarkus/mongodb/deployment/CommandListenerBuildItem.java

+
+import io.quarkus.builder.item.SimpleBuildItem;
+
+public final class CommandListenerBuildItem extends SimpleBuildItem {


Can you please add some Javadoc to this class? This is necessary for the All Build Items page

gsmet · 2020-10-22T13:36:35Z

I added some javadoc and rebased.

gsmet requested changes Sep 14, 2020

View reviewed changes

gastaldi reviewed Sep 15, 2020

View reviewed changes

extensions/mongodb-client/runtime/src/main/java/io/quarkus/mongodb/runtime/MongoClients.java Outdated Show resolved Hide resolved

juazugas force-pushed the ext-mongodb-opentracing branch from 80d7036 to b7092f2 Compare September 16, 2020 13:05

loicmathieu requested changes Sep 23, 2020

View reviewed changes

juazugas marked this pull request as ready for review September 29, 2020 14:49

juazugas requested review from loicmathieu and gsmet September 29, 2020 17:03

loicmathieu requested changes Sep 30, 2020

View reviewed changes

juazugas force-pushed the ext-mongodb-opentracing branch from 7715e92 to 8a5a020 Compare September 30, 2020 15:17

loicmathieu approved these changes Sep 30, 2020

View reviewed changes

gsmet requested changes Oct 8, 2020

View reviewed changes

gsmet added the triage/needs-rebase This PR needs to be rebased first because it has merge conflicts label Oct 12, 2020

juazugas force-pushed the ext-mongodb-opentracing branch from aee8c58 to 557b58a Compare October 12, 2020 15:30

juazugas requested a review from gsmet October 12, 2020 15:49

gsmet force-pushed the ext-mongodb-opentracing branch from 557b58a to 1230272 Compare October 13, 2020 12:45

gsmet approved these changes Oct 13, 2020

View reviewed changes

gsmet added triage/waiting-for-ci Ready to merge when CI successfully finishes and removed triage/needs-rebase This PR needs to be rebased first because it has merge conflicts labels Oct 13, 2020

gsmet added this to the 1.10 - master milestone Oct 13, 2020

gastaldi reviewed Oct 19, 2020

View reviewed changes

jzuriaga and others added 2 commits October 22, 2020 15:36

Add CommandListener implementors to the mongoDB settings.

b7f3acc

Add javadoc for MongoDB client build items

e09bd88

gsmet force-pushed the ext-mongodb-opentracing branch from 1230272 to e09bd88 Compare October 22, 2020 13:36

gastaldi merged commit cba0b3b into quarkusio:master Oct 22, 2020

loicmathieu mentioned this pull request Dec 10, 2020

Provide Event Listener for MongoDB (Panache) persist / update / delete events #11164

Closed

juazugas mentioned this pull request Dec 13, 2020

Add OpenTracing Command Listener to MongoDB client #13863

Merged


		import io.quarkus.builder.item.SimpleBuildItem;

		public final class CommandListenerBuildItem extends SimpleBuildItem {

Set a injected CDI CommandListener to the mongodb configuration. #12082

Set a injected CDI CommandListener to the mongodb configuration. #12082

Conversation

juazugas commented Sep 14, 2020

gsmet left a 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

gsmet commented Sep 14, 2020

loicmathieu commented Sep 14, 2020

juazugas commented Sep 14, 2020 • edited Loading

juazugas commented Sep 20, 2020

pavolloffay commented Sep 22, 2020

loicmathieu commented Sep 23, 2020

loicmathieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicmathieu commented Sep 23, 2020

juazugas commented Sep 23, 2020

loicmathieu commented Sep 23, 2020

juazugas commented Sep 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicmathieu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

loicmathieu left a comment

Choose a reason for hiding this comment

loicmathieu commented Oct 1, 2020

gsmet left a 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

gsmet commented Oct 12, 2020

juazugas commented Oct 12, 2020

gsmet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gsmet commented Oct 22, 2020

juazugas commented Sep 14, 2020 •

edited

Loading

juazugas commented Sep 23, 2020 •

edited

Loading