Fix registration of command subscribers #569

alcaeus · 2019-07-12T08:48:11Z

Fixes #563. Command subscribers were not properly registered - just adding a method call to the service definition is not enough.

To work around this, we add a command subscriber registry that is registered and unregistered when the bundle is booted and shut down. The registry proxies all calls to register and unregister to all registered subscribers. Since we don't have an interface for registrable command subscribers, I simply added method_exists checks to avoid fatal errors. We can still add an interface for such subscribers to the next release candidate for mongodb-odm and change the code to throw exceptions instead of silently skipping incompatible subscribers. Opinions @jmikola @malarzm?

jmikola

I'm a bit confused by this PR, as I thought ODM 2.0 was not planning to utilize ext-mongodb's APM functionality for its query logger.

IIRC, ODM 1.x populated Symfony's profiler with logged queries/commands from its own internal logging mechanism. I believe that's what the OP in #563 was referring to. Was that internal logging removed in ODM 2.0?

Logging via a CommandSubscriber would be entirely new functionality, and I expect an instance would need to pipe data to the Symfony service from one of its three listener methods (or collect all events until the end of a request and send it over in one batch).

One issue with doing so is that APM logging is global (beyond the scope of even a single Manager instance), so it will pick up commands from any library using ext-mongodb. This could be actually prove problematic if profiler or log data ends up being written to MongoDB (see: PHPC-1410).

jmikola · 2019-07-15T16:25:54Z

APM/CommandSubscriberRegistry.php

+
+    public function __construct(iterable $commandSubscribers)
+    {
+        $this->commandSubscribers = $commandSubscribers;


Would it make sense for this to validate that each element within $commandSubscribers is actually a CommandSubscriber instance?

On second thought, I wonder if this is instead supposed to accept only Doctrine\ODM\MongoDB\APM\CommandLogger instances, as that class is where register() and unregister() methods are defined.

We'll have to introduce a new interface, as the CommandLogger class is final but we also need a command logger that logs to any PSR-compatible logger. I'd introduce a CommandLogger interface that extends CommandSubscriber and exposes register and unregister methods, then limit possible arguments to this method to that interface.

jmikola · 2019-07-15T16:27:41Z

APM/CommandSubscriberRegistry.php

+                continue;
+            }
+
+            $subscriber->register();


Since CommandSubscriber has no register() and unregister() methods, I don't believe we'd ever reach this point. Subscribers are registered with the extension globally via these functions.

The APM tutorial has some examples that tie this all together.

jmikola · 2019-07-15T19:01:46Z

Since we don't have an interface for registrable command subscribers, I simply added method_exists checks to avoid fatal errors. We can still add an interface for such subscribers to the next release candidate for mongodb-odm and change the code to throw exceptions instead of silently skipping incompatible subscribers.

This might be related to my last comment. When I initially reviewed this I was not aware of the CommandLogger class in ODM.

Was the CommandSubscriber type hint purposeful?

I understand if you don't want to add an interface with register()/unregister() methods for the ODM subscriber in this PR, but I think it may be worth throwing an exception out of the gate so listeners are never silently ignored. Alternatively, you could manually check for CommandLogger instances when constructing the registry; however, that may be a no-go if the intention is to avoid checking classes at all costs (favoring interfaces).

alcaeus · 2019-07-22T12:32:53Z

Looking at the comments, it definitely makes more sense to introduce our interface for loggers, which only declares the register and unregister methods.

However, the fact that command subscriber don't only react to commands sent through the current client but rather collect all commands executed on the server makes me doubt whether we want to add the logging functionality in this form. I believe it is definitely unexpected and potentially dangerous to log all commands executed on the server in the application. Since the events also don't expose any information on who triggered the command, we most likely won't be able to filter out any unwanted commands either.

markitosgv · 2019-08-05T13:33:04Z

Any updates? Thanks!!

alcaeus · 2019-08-05T13:53:41Z

Any updates? Thanks!!

Not yet.

alcaeus · 2019-09-24T08:15:21Z

Updated the code with the new CommandLoggerInterface contained in RC2 of ODM. This can be merged once RC2 is released, at which point I'll also remove the commit requiring dev-master.

malarzm · 2019-09-24T17:32:00Z

APM/CommandLoggerRegistry.php

+
+    public function unregister() : void
+    {
+        array_map(static function (CommandLoggerInterface $commandLogger) {


Foreach is bad? :D

One benefit is that this applies type checking to each element, which would otherwise require an instanceof check within foreach.

That was one of the reasons why I added it. Of course, a type check is already done in the constructor when storing the items, but then I've started using array_map in place of foreach to make it more explicit. I can change it to a loop if you prefer that

jmikola · 2019-09-24T17:52:19Z

APM/CommandLoggerRegistry.php

+
+    public function unregister() : void
+    {
+        array_map(static function (CommandLoggerInterface $commandLogger) {


One benefit is that this applies type checking to each element, which would otherwise require an instanceof check within foreach.

jmikola · 2019-09-24T17:53:38Z

DoctrineMongoDBBundle.php

+        $commandLoggerRegistry->unregister();
+    }
+
+    public function shutdown()


Should this have a return type hint, or is that omitted because we're overriding a method from Symfony's base class?

I've omitted it because Symfony doesn't have one and I haven't had time to test this against Symfony 5 so far. All Doctrine Bundles will get the Symfony 4.4/5.0 treatment after the Symfony Code Freeze next week (DoctrineBundle has been the guinea pig for this). This will add some return type annotations where applicable. I'll have to see how to treat this since I don't want to hold 4.0 back until November, but I also don't want to release a new major version shortly after releasing 4.0 already.

jmikola · 2019-09-24T17:55:17Z

composer.json

@@ -13,7 +13,7 @@
    "require": {
        "php": "^7.2",
        "doctrine/annotations": "^1.2",
-        "doctrine/mongodb-odm": "^2.0.0-beta1",
+        "doctrine/mongodb-odm": "dev-master",


Should this be changed to 2.0.0-RC2 (following its release, of course)?

EDIT: Just saw the "REMOVE:" commit. Makes sense.

jmikola · 2019-09-24T17:56:34Z

Tests/FixtureIntegrationTest.php

@@ -41,7 +41,7 @@ protected function setUp()

    public function testFixturesLoader() : void
    {
-        $kernel = new IntegrationTestKernel('dev', true);
+        $kernel = new IntegrationTestKernel('dev', false);


Is this pertinent to the PR, or just some extra cleanup?

This was some necessary cleanup as tests started failing.

alcaeus added the bug label Jul 12, 2019

alcaeus added this to the 4.0.0 milestone Jul 12, 2019

alcaeus requested review from jmikola and malarzm July 12, 2019 08:48

alcaeus self-assigned this Jul 12, 2019

alcaeus force-pushed the fix-command-logger-registration branch from ea17dc8 to 8386bcd Compare July 14, 2019 18:38

jmikola reviewed Jul 15, 2019

View reviewed changes

alcaeus mentioned this pull request Sep 11, 2019

Introduce interface for command loggers doctrine/mongodb-odm#2059

Merged

alcaeus force-pushed the fix-command-logger-registration branch from 8386bcd to 30833ab Compare September 24, 2019 08:14

alcaeus requested a review from jmikola September 24, 2019 08:15

alcaeus force-pushed the fix-command-logger-registration branch from 30833ab to a024f9d Compare September 24, 2019 14:18

malarzm reviewed Sep 24, 2019

View reviewed changes

malarzm approved these changes Sep 24, 2019

View reviewed changes

jmikola approved these changes Sep 24, 2019

View reviewed changes

alcaeus added 2 commits September 24, 2019 20:50

Require RC2 of ODM

64fb75c

Fix registration of command subscribers

17657d2

alcaeus force-pushed the fix-command-logger-registration branch from a024f9d to 17657d2 Compare September 24, 2019 18:59

alcaeus merged commit 4173faf into doctrine:master Sep 24, 2019

alcaeus deleted the fix-command-logger-registration branch September 24, 2019 19:16

alcaeus mentioned this pull request Sep 25, 2019

bug with mismatch type of method in 4.0.0-RC2 #584

Closed

malarzm mentioned this pull request Jul 5, 2020

Rewrite factories for mongodb-odm version 2 doctrine/DoctrineMongoODMModule#222

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix registration of command subscribers #569

Fix registration of command subscribers #569

alcaeus commented Jul 12, 2019

jmikola left a comment

jmikola Jul 15, 2019

jmikola Jul 15, 2019

alcaeus Sep 5, 2019

jmikola Jul 15, 2019

jmikola commented Jul 15, 2019

alcaeus commented Jul 22, 2019

markitosgv commented Aug 5, 2019

alcaeus commented Aug 5, 2019

alcaeus commented Sep 24, 2019

malarzm Sep 24, 2019

jmikola Sep 24, 2019

alcaeus Sep 24, 2019

jmikola Sep 24, 2019

jmikola Sep 24, 2019

alcaeus Sep 24, 2019

jmikola Sep 24, 2019

jmikola Sep 24, 2019

alcaeus Sep 24, 2019

Fix registration of command subscribers #569

Fix registration of command subscribers #569

Conversation

alcaeus commented Jul 12, 2019

jmikola 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

jmikola commented Jul 15, 2019

alcaeus commented Jul 22, 2019

markitosgv commented Aug 5, 2019

alcaeus commented Aug 5, 2019

alcaeus commented Sep 24, 2019

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