added backtrace to query logging #535
Conversation
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-824 We use Jira to track the state of pull requests and the versions they got |
| $this->queries[++$this->currentQuery] = array('sql' => $sql, 'params' => $params, 'types' => $types, 'executionMS' => 0); | ||
|
|
||
| $trace = debug_backtrace(); | ||
| if (function_exists('xdebug_get_function_stack')) { |
There was a problem hiding this comment.
copied from Symfony\Component\Debug\FlattenException::setTraceFromException - is there a place to share this code for non-exceptions?
| @@ -62,7 +62,33 @@ public function startQuery($sql, array $params = null, array $types = null) | |||
| { | |||
| if ($this->enabled) { | |||
| $this->start = microtime(true); | |||
| $this->queries[++$this->currentQuery] = array('sql' => $sql, 'params' => $params, 'types' => $types, 'executionMS' => 0); | |||
|
|
|||
| $trace = debug_backtrace(); | |||
There was a problem hiding this comment.
this should be inside the else to avoid useless processing
|
@dmecke this is seriously overboard. Just store |
| @@ -26,6 +26,7 @@ public function testLoggedQuery() | |||
| 'params' => null, | |||
| 'types' => null, | |||
| 'executionMS' => 0, | |||
| 'stacktrace' => null, | |||
There was a problem hiding this comment.
The test fails, because the stacktrace is already set with an actual array. How can I test this here without that failure?
There was a problem hiding this comment.
Anyone who can help me? Or should the stacktrace simply be removed from the test as we cannot know what should be in there?
There was a problem hiding this comment.
You can only do minimal assumptions on it, such as "it is an array", and "it is not empty"
There was a problem hiding this comment.
Ok, but how can I do that inside the assertEquals for the whole array? I mean that general assertEquals will now always fail because of that one new element...
I am sorry, but I am new to unit tests and such cases still gives me some headaches.
There was a problem hiding this comment.
@dmecke you simply can't use assertEquals here - it is too generic. Use multiple assertions instead.
|
Ok, I fixed the tests now, thx @Ocramius for your help! Now the travis build still fails for sqlite and mysqli, but this is the case in the master as well, so I think it's not a problem about this pull request. |
|
@dmecke The current master is not failing as far as I can see. But your PR branch uses a rather outdated codebase. Maybe the master tests were failing back then. Try to rebase your PR branch with current master and see what travis says. |
|
Hmm weird. There seems to be a problem with locked tables: https://travis-ci.org/doctrine/dbal/jobs/22262335#L418 |
|
I'm not that familiar with |
|
If you use IMO, the DebugStack should not keep the stacktrace in memory. If you want to log the stack trace of queries, you should do it with a custom logger which processes the stacktrace just after retrieving it, so that it does not keep it around |
|
Thus, if your use case is storing the backtrace of queries in the Symfony profiler, you will need to process it anyway, because it needs to be serializable to be stored anyway |
|
I agree with @stof. The chosen approach is more harmful than useful then and can lead to unpredicted behaviour. We should not merge this IMO. This should be done elsewise as suggested. |
|
You should foreach it and drop references to objects and propbably even call arguments. |
|
You can create your own Logger for this support. Closing as won't fix. |
|
Ok, thanks for your time. |
|
ref #719 |
|
Implemented in doctrine/DoctrineBundle#954 |
I've added a stacktrace to each logged query to be able to display it later on. This can help to find out why a query has been executed.
This one is needed for pull request doctrine/DoctrineBundle#275 in doctrine/doctrineBundle.