Replace QueryLogger in tests with CommandLogger #1866
Conversation
alcaeus
force-pushed
the
add-apm-classes
branch
from
4762439
to
2ad12c0
Compare
| $this->logCommand(Command::createForSucceededCommand($commandStartedEvent, $event)); | ||
| } | ||
|
|
||
| public function commandFailed(CommandFailedEvent $event) |
There was a problem hiding this comment.
What is the use case for having 2 methods for finishing a command? I think it'd be easier for a consumer to have one commandFinished(CommandSucceededEvent|CommandFailedEvent $event)
There was a problem hiding this comment.
You'll have to ask @jmikola or @derickr as this interface is provided by the driver: http://php.net/manual/en/class.mongodb-driver-monitoring-commandsubscriber.php.
There was a problem hiding this comment.
Ah, that makes more sense now, I somehow thought that users will be calling these methods. Need a coffee ;)
There was a problem hiding this comment.
Don't blame us, this is a cross-driver spec: https://github.com/mongodb/specifications/blob/master/source/command-monitoring/command-monitoring.rst
😄
| private function getCommandStartedEvent(string $requestId): ?CommandStartedEvent | ||
| { | ||
| $startedEvent = $this->startedCommands[$requestId] ?? null; | ||
| unset($this->startedCommands[$requestId]); |
There was a problem hiding this comment.
I think method name should reflect that state is changing, right now calling this twice will yield different results
There was a problem hiding this comment.
What do you think about findCommandStartedEvent?
There was a problem hiding this comment.
findAndRemoveCommandStartedEvent? Lengthy but self explaining
|
Note: corresponding functionality (still WIP) can be seen in doctrine/DoctrineMongoDBBundle@32f92ec. |
alcaeus
force-pushed
the
add-apm-classes
branch
from
2ad12c0
to
5e1639c
Compare
This PR supersedes #1850. I took the original idea and tracked the
CommandStartedEventas well. The reason behind this is that we need to store the original command to use it in the ShardKeyTest. Apart from that, the DataCollector used in the Symfony Bundle also needs to know all commands that were run, which is why I've moved the class to its own Sub-Namespace. With it being part in the API, we can use it in the upcoming bundle version to track queries for the web developer toolbar.Thanks @caciobanu for getting this started - it was your implementation that gave me the idea how to handle this in the bundle 👍