Skip to content
This repository has been archived by the owner on Nov 11, 2020. It is now read-only.

Better query logging capabilities - implementation #153

Closed
wants to merge 6 commits into from
Closed

Better query logging capabilities - implementation #153

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9315437
#151 - initial work for QueryLogger integration
woodworker Feb 3, 2014
4ae21a1
#151 - added sample implementations
woodworker Feb 3, 2014
c9f8c97
#151 - replaced phpstorm comment with doctrine copyright block
woodworker Feb 3, 2014
850f82d
replaced closure solution with simple variable
woodworker Feb 5, 2014
35faf6e
fixed small bug for logging and added unit tests for them
woodworker Feb 6, 2014
715a1f7
code style fixes
woodworker Feb 8, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/Doctrine/MongoDB/Configuration.php
View file
Open in desktop
Expand Up @@ -58,6 +58,26 @@ public function setLoggerCallable($loggerCallable)
$this->attributes['loggerCallable'] = $loggerCallable;
}

/**
* Set QueryLogger
*
* @param Logging\QueryLogger $queryLogger
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a use statement for the QueryLogger class? That makes it easier to see at a glance what the file is using.

*/
public function setQueryLogger(Logging\QueryLogger $queryLogger)
{
$this->attributes['queryLogger'] = $queryLogger;
}

/**
* Get QueryLogger
*
* @return null|Logging\QueryLogger
*/
public function getQueryLogger()
{
return isset($this->attributes['queryLogger']) ? $this->attributes['queryLogger'] : null;
}

/**
* Get the MongoDB command prefix.
*
Expand Down
5 changes: 3 additions & 2 deletions lib/Doctrine/MongoDB/Connection.php
View file
Open in desktop
Expand Up @@ -404,9 +404,10 @@ protected function doSelectDatabase($name)
$mongoDB = $this->mongoClient->selectDB($name);
$numRetries = $this->config->getRetryQuery();
$loggerCallable = $this->config->getLoggerCallable();
$queryLogger = $this->config->getQueryLogger();

return $loggerCallable !== null
? new LoggableDatabase($this, $mongoDB, $this->eventManager, $numRetries, $loggerCallable)
return ($loggerCallable !== null || $queryLogger!== null)
? new LoggableDatabase($this, $mongoDB, $this->eventManager, $numRetries, $loggerCallable, $queryLogger)
: new Database($this, $mongoDB, $this->eventManager, $numRetries);
}

Expand Down
161 changes: 113 additions & 48 deletions lib/Doctrine/MongoDB/LoggableCollection.php
View file
Open in desktop
Expand Up @@ -37,6 +37,11 @@ class LoggableCollection extends Collection implements Loggable
*/
protected $loggerCallable;

/**
* @var Logging\QueryLogger
*/
protected $queryLogger;

/**
* Constructor.
*
Expand All @@ -45,13 +50,15 @@ class LoggableCollection extends Collection implements Loggable
* @param EventManager $evm EventManager instance
* @param integer $numRetries Number of times to retry queries
* @param callable $loggerCallable The logger callable
* @param Logging\QueryLogger $queryLogger The QueryLogger object
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as earlier regarding a use statement for Logging\QueryLogger. That would also shorten this to QueryLogger and avoid the alignment issue.

*/
public function __construct(Database $database, \MongoCollection $mongoCollection, EventManager $evm, $numRetries, $loggerCallable)
public function __construct(Database $database, \MongoCollection $mongoCollection, EventManager $evm, $numRetries, $loggerCallable = null, Logging\QueryLogger $queryLogger = null)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwage: I'm worried about adding more constructor arguments here, since it's a change that then has to be implemented whenever we extend the class (to ensure we accept the argument with the same default value, and then pass it along to the parent). Assuming we allow both $loggerCallable and the QueryLogger to be optional (as I suggest here)., what do you think about having setter methods for each? We'd naturally leave the $loggerCallable argument in place with a null default value for BC.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. 👍

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i'll move it to a setter

but best solution would be to remove everything starting at $numRetries and just inject the configuration object so future config params will not be injected seperate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. This is something I've thought about for a while. I changed the constructor order of some of these core classes for 1.1.0 via 39a2b7c, so perhaps we can consider this for 1.2.0? Otherwise, it'd be something for 2.0 if we acknowledge it's a BC break. I didn't before, as I assumed most users access these classes through the Connection, so that was the public API to preserve. Thoughts, @jwage?

{
if ( ! is_callable($loggerCallable)) {
throw new \InvalidArgumentException('$loggerCallable must be a valid callback');
if ( ! is_callable($loggerCallable) && !($queryLogger instanceof Logging\QueryLogger)) {
throw new \InvalidArgumentException('$loggerCallable must be a valid callback or $queryLogger must be an instance of Doctrine\MongoDB\Logging\QueryLogger');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're already type-hinting $queryLogger, so does it matter if it's null?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, just saw the testNoLoggerGivenThrowsException() test below. Since you've changed the log() method to conditionally invoke $this->loggerCallable or the QueryLogger, I wonder if this constructor exception even matters anymore. If the class has neither a callable or QueryLogger set, we do nothing. On its own, the library shouldn't even construct the class based on Connection::doSelectDatabase().

}
$this->loggerCallable = $loggerCallable;
$this->queryLogger = $queryLogger;
parent::__construct($database, $mongoCollection, $evm, $numRetries);
}

Expand All @@ -65,211 +72,269 @@ public function log(array $log)
{
$log['db'] = $this->database->getName();
$log['collection'] = $this->getName();
call_user_func_array($this->loggerCallable, array($log));
if ($this->loggerCallable) {
call_user_func_array($this->loggerCallable, array($log));
}

if ($this->queryLogger instanceof Logging\QueryLogger) {
$this->queryLogger->startQuery($log);
}
}

private function logAfter()
{
if ($this->queryLogger instanceof Logging\QueryLogger) {
$this->queryLogger->stopQuery();
}
}

/**
* @see Collection::batchInsert()
*/
public function batchInsert(array &$a, array $options = array())
{
$this->log(array(
$log = array(
'batchInsert' => true,
'num' => count($a),
'data' => $a,
'options' => $options,
));
);

return parent::batchInsert($a, $options);
$this->log($log);
$data = parent::batchInsert($a, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::count()
*/
public function count(array $query = array(), $limit = 0, $skip = 0)
{
$this->log(array(
$log = array(
'count' => true,
'query' => $query,
'limit' => $limit,
'skip' => $skip,
));
);

return parent::count($query, $limit, $skip);
$this->log($log);
$data = parent::count($query, $limit, $skip);
$this->logAfter();
return $data;
}

/**
* @see Collection::deleteIndex()
*/
public function deleteIndex($keys)
{
$this->log(array(
$log = array(
'deleteIndex' => true,
'keys' => $keys,
));
);

return parent::deleteIndex($keys);
$this->log($log);
$data = parent::deleteIndex($keys);
$this->logAfter();
return $data;
}

/**
* @see Collection::deleteIndexes()
*/
public function deleteIndexes()
{
$this->log(array('deleteIndexes' => true));
$log = array('deleteIndexes' => true);

return parent::deleteIndexes();
$this->log($log);
$data = parent::deleteIndexes();
$this->logAfter();
return $data;
}

/**
* @see Collection::drop()
*/
public function drop()
{
$this->log(array('drop' => true));
$log = array('drop' => true);

return parent::drop();
$this->log($log);
$data = parent::drop();
$this->logAfter();
return $data;
}

/**
* @see Collection::ensureIndex()
*/
public function ensureIndex(array $keys, array $options = array())
{
$this->log(array(
$log = array(
'ensureIndex' => true,
'keys' => $keys,
'options' => $options,
));
);

return parent::ensureIndex($keys, $options);
$this->log($log);
$data = parent::ensureIndex($keys, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::find()
*/
public function find(array $query = array(), array $fields = array())
{
$this->log(array(
$log = array(
'find' => true,
'query' => $query,
'fields' => $fields,
));
);

return parent::find($query, $fields);
$this->log($log);
$data = parent::find($query, $fields);
$this->logAfter();
return $data;
}

/**
* @see Collection::findOne()
*/
public function findOne(array $query = array(), array $fields = array())
{
$this->log(array(
$log = array(
'findOne' => true,
'query' => $query,
'fields' => $fields,
));
);

return parent::findOne($query, $fields);
$this->log($log);
$data = parent::findOne($query, $fields);
$this->logAfter();
return $data;
}

/**
* @see Collection::getDBRef()
*/
public function getDBRef(array $reference)
{
$this->log(array(
$log = array(
'getDBRef' => true,
'reference' => $reference,
));
);

return parent::getDBRef($reference);
$this->log($log);
$data = parent::getDBRef($reference);
$this->logAfter();
return $data;
}

/**
* @see Collection::group()
*/
public function group($keys, array $initial, $reduce, array $options = array())
{
$this->log(array(
$log = array(
'group' => true,
'keys' => $keys,
'initial' => $initial,
'reduce' => $reduce,
'options' => $options,
));
);

return parent::group($keys, $initial, $reduce, $options);
$this->log($log);
$data = parent::group($keys, $initial, $reduce, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::insert()
*/
public function insert(array &$a, array $options = array())
{
$this->log(array(
$log = array(
'insert' => true,
'document' => $a,
'options' => $options,
));
);

return parent::insert($a, $options);
$this->log($log);
$data = parent::insert($a, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::remove()
*/
public function remove(array $query, array $options = array())
{
$this->log(array(
$log = array(
'remove' => true,
'query' => $query,
'options' => $options,
));
);

return parent::remove($query, $options);
$this->log($log);
$data = parent::remove($query, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::save()
*/
public function save(array &$a, array $options = array())
{
$this->log(array(
$log = array(
'save' => true,
'document' => $a,
'options' => $options,
));
);

return parent::save($a, $options);
$this->log($log);
$data = parent::save($a, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::update()
*/
public function update($query, array $newObj, array $options = array())
{
$this->log(array(
$log = array(
'update' => true,
'query' => $query,
'newObj' => $newObj,
'options' => $options,
));
);

return parent::update($query, $newObj, $options);
$this->log($log);
$data = parent::update($query, $newObj, $options);
$this->logAfter();
return $data;
}

/**
* @see Collection::validate()
*/
public function validate($scanData = false)
{
$this->log(array(
$log = array(
'validate' => true,
'scanData' => $scanData,
));
);

return parent::validate($scanData);
$this->log($log);
$data = parent::validate($scanData);
$this->logAfter();
return $data;
}

/**
Expand All @@ -283,6 +348,6 @@ public function validate($scanData = false)
*/
protected function wrapCursor(\MongoCursor $cursor, $query, $fields)
{
return new LoggableCursor($this, $cursor, $query, $fields, $this->numRetries, $this->loggerCallable);
return new LoggableCursor($this, $cursor, $query, $fields, $this->numRetries, $this->loggerCallable, $this->queryLogger);
}
}