[2.0] Updated QueryLogger from tests.

[caciobanu]

Q	A
Type	improvement
BC Break	no
Fixed issues	#1791

Summary

Giving it another try after the discussion on #1849. This time I've used the feature explained here: http://php.net/manual/en/mongodb.tutorial.apm.php.

I've only updated one test to get some feedback before.

[caciobanu]

Build failure doesn't seem related to these modifications.

[malarzm]

To me this looks great, but I'd defer decision to people who know new driver, @jmikola @alcaeus?

[alcaeus]

This looks good. If I remember correctly, query logging is used in multiple tests. If so, we might want to extract common functionality to a QueryLoggerTestCase class so we don't have to repeatedly copy/paste the same code snippet.

[jmikola]

IMO, the parent's tearDown() should be called last so it follows the opposite order of setup:

1. Setup parent
2. Setup ourself
3. Tear down ourself
4. Tear down parent

[jmikola]

Why not have the QueryLogger register itself? Note that while you can register the class in the constructor, you can't rely on unregistering via a destructor, as the driver ends up holding a reference to the registered subscriber instance. Therefore, I'd advocate for explicit register() and unregister() methods (or similar).

Alternatively, you may want to consider the design of CommandObserver from PHPLIB. That class accepts an arbitrary closure to run between calls to add/removeSubscriber so it's not registered for the entirety of the test (e.g. setup and tearDown, other ops run before the query we want to monitor).

[jmikola]

I'm not sure what array $query was previously (the query document or find filter?), but this value is certainly different. As such, this may break any tests that attempt to access QueryLogger::$queries via getAll().

If you want to log the outgoing command, you should collect CommandStartedEvent::getCommand() from commandStarted() and be prepared for BSON documents to be represented as stdClass instances.

In commandSucceeded(), you can capture the command reply via CommandSucceededEvent::getReply(), but I don't believe any ODM tests care about replies.

[alcaeus]

I've updated the following:

• Renamed the logger class to CommandLogger
• Add logging for failed commands as well (not needed currently)
• Added register and unregister methods to the logger as per suggestion by @jmikola
• Made setUp and tearDown handling consistent with all tests

How we proceed with the class mainly depends on the ODM bundle: we'll have to add an APM class there as well to log queries for the web developer toolbar. Given that, the class may well be updated once more and be moved to the src folder - for now this serves the purpose of testing quite well. Thanks @caciobanu!

Edit: maybe we do use it after all. Back to updating some tests.

[caciobanu]

Sorry for not taking care of this PR but I lacked the necessary time.

[alcaeus]

Sorry for not taking care of this PR but I lacked the necessary time.

That's alright, I needed to take a look at the APM functionality for the bundle anyways, so this was a good opportunity.

Just a heads up, I'm currently working on a more advanced logger that we can leverage for the Web Developer Toolbar in the ODM bundle as well, so I may end up dropping the original logger class for a more robust implementation that will also be part of the public API.

[caciobanu]

@alcaeus is it ok if I finish the implementation for this PR ?

[alcaeus]

I’m working on a command logger that exposes the command itself along with the result (or error if it failed) - we need that for some sharding tests as well as the logger and data collector in the ODM bundle. I’ll have a PR ready tomorrow.

[alcaeus]

Closing in favour of #1866. Thanks for your work @caciobanu!