Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added TraceLogger to get backtrace for executed queries #719

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
125 changes: 125 additions & 0 deletions lib/Doctrine/DBAL/Logging/TraceLogger.php
@@ -0,0 +1,125 @@
<?php
/*
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
* "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
* LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
* A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
* OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
* SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
* LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
* DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
* THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
* (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
* OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
*
* This software consists of voluntary contributions made by many individuals
* and is licensed under the MIT license. For more information, see
* <http://www.doctrine-project.org>.
*/

namespace Doctrine\DBAL\Logging;

/**
* Includes backtrace and executed SQLs in a Debug Stack.
*
* @link www.doctrine-project.org
* @author Pierre du Plessis <pdples@gmail.com>
*/
class TraceLogger implements SQLLogger
{
/**
* Executed SQL queries.
*
* @var array
*/
public $queries = array();

/**
* If the logger is enabled (log queries) or not.
*
* @var boolean
*/
public $enabled = true;

/**
* @var float|null
*/
public $start = null;

/**
* @var integer
*/
public $currentQuery = 0;

/**
* {@inheritdoc}
*/
public function startQuery($sql, array $params = null, array $types = null)
{
if ($this->enabled) {
$backtrace = $this->getBactrace();

$this->start = microtime(true);
$this->queries[++$this->currentQuery] = array('sql' => $sql, 'params' => $params, 'types' => $types, 'executionMS' => 0, 'trace' => $backtrace);
}
}

/**
* {@inheritdoc}
*/
public function stopQuery()
{
if ($this->enabled) {
$this->queries[$this->currentQuery]['executionMS'] = microtime(true) - $this->start;
}
}

private function getBactrace()
Copy link
Member

Choose a reason for hiding this comment

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

Should be "Backtrace" with a "k"

{
$backtrace = debug_backtrace(DEBUG_BACKTRACE_PROVIDE_OBJECT | DEBUG_BACKTRACE_IGNORE_ARGS);

foreach ($backtrace as $key => $debug) {
Copy link
Member

Choose a reason for hiding this comment

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

Just store the debug_backtrace() result, no further action needed.

Copy link
Author

Choose a reason for hiding this comment

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

Just storing the trace can have huge drawbacks for memory usage (as @stof explained in #535 (comment)).

The idea of this logger is to see where a query originated from. Just storing the trace means each user will need to go through the trace and figure out where the query was executed from. Where in this case, all the processing is already done, and the result can just be displayed in debugging (Which is also why it only prints the last 10 frames of the trace, because you won't need more info than that to see where a query is called from, and it also strips out any calls to internal classes when building the trace)

Copy link
Member

Choose a reason for hiding this comment

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

This is a debugging feature: memory/performance are NOT a problem while debugging :-)

Copy link
Member

Choose a reason for hiding this comment

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

And yes: we'd still want to see internal calls, as that exposes queries triggered by (for example) eager/lazy loading happening in unexpected locations of the codebase.

Copy link
Author

Choose a reason for hiding this comment

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

Well yeah, but if you attach this to the Symfony toolbar f.e, then it will slow down your page with each request, and it will keep the entire stacktrace where you are only interested in parts of the trace

Copy link
Member

Choose a reason for hiding this comment

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

So is there any conclusion on this? How should we proceed?

Copy link
Member

Choose a reason for hiding this comment

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

How about making it optional and configurable?

Choose a reason for hiding this comment

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

👍 for optional and configurable with a simple boolean, and I'd make it true by default to satisfy everyone

Copy link
Author

Choose a reason for hiding this comment

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

@greg0ire Can you please explain which part you mean to make optional and configurable? This logger is already enabled by default, and can be disabled by setting the property enabled to false. Is there another part that you want configurable?

Copy link
Member

Choose a reason for hiding this comment

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

The part I mean is the part where you include the backtrace in the $queries array.

if (!$this->isInternalClass($debug['class'])) {
$trace = array_slice($backtrace, $key - 1, 10);

return $this->formatTrace($trace);
}
}

return array();
}

private function formatTrace(array $trace)
{
$backtrace = array();

foreach ($trace as $index => $line) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not simply storing the trace and that's it? Dumping a trace is a separate concern

$backtrace[$index] = '';

if (isset($trace[$index + 1]['class'])) {
$backtrace[$index] .= $trace[$index + 1]['class'];
} else {
$backtrace[$index] .= get_class($line['object']);
}

$backtrace[$index] .= '::';

if (isset($trace[$index + 1])) {
$backtrace[$index] .= $trace[$index + 1]['function'];
} else {
$backtrace[$index] .= $line['function'];
}

if (isset($line['line'])) {
$backtrace[$index] .= ' (L' . $line['line'] . ')';
}
}

return $backtrace;
}

private function isInternalClass(&$class)
{
return substr($class, 0, strpos($class, '\\')) === 'Doctrine';
}
}
65 changes: 65 additions & 0 deletions tests/Doctrine/Tests/DBAL/Logging/TraceLoggerTest.php
@@ -0,0 +1,65 @@
<?php

namespace Doctrine\Tests\DBAL\Logging;

require_once __DIR__ . '/../../TestInit.php';

class TraceLoggerTest extends \Doctrine\Tests\DbalTestCase
{
/**
* @var \Doctrine\DBAL\Logging\TraceLogger
*/
private $logger;

public function setUp()
{
$this->logger = new \Doctrine\DBAL\Logging\TraceLogger();
}

public function tearDown()
{
unset($this->logger);
}

public function testLoggedQuery()
{
$this->logger->startQuery('SELECT column FROM table');

$this->assertEquals(
array(
1 => array(
'sql' => 'SELECT column FROM table',
'params' => null,
'types' => null,
'executionMS' => 0,
'trace' => array(
"ReflectionMethod::invokeArgs",
"PHPUnit_Framework_TestCase::runTest (L905)",
Copy link
Member

Choose a reason for hiding this comment

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

This test is too fragile.

Copy link
Author

Choose a reason for hiding this comment

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

Will update the test. Would it be sufficient to just test if the trace is an array and contains a specified amount of values, without actually checking what those values are?

Copy link
Member

Choose a reason for hiding this comment

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

It should probably just assert that an instance of the TraceLoggerTest was in the trace: can't rely on line numbers or length of the trace, as PHPUnit cannot ensure that, nor the engine (because of weird opcode optimizations that are coming up in PHP7)

"PHPUnit_Framework_TestCase::runBare (L775)",
"PHPUnit_Framework_TestResult::run (L643)",
"PHPUnit_Framework_TestCase::run (L711)",
"PHPUnit_Framework_TestSuite::run (L751)",
"PHPUnit_Framework_TestSuite::run (L751)",
"PHPUnit_TextUI_TestRunner::doRun (L423)",
"PHPUnit_TextUI_Command::run (L186)",
"PHPUnit_TextUI_Command::run (L138)"
)
),
),
$this->logger->queries
);

$this->logger->stopQuery();
$this->assertGreaterThan(0, $this->logger->queries[1]['executionMS']);
}

public function testLoggedQueryDisabled()
{
$this->logger->enabled = false;
$this->logger->startQuery('SELECT column FROM table');
$this->assertEquals(array(), $this->logger->queries);

$this->logger->stopQuery();
$this->assertEquals(array(), $this->logger->queries);
}
}