-
Notifications
You must be signed in to change notification settings - Fork 9.4k
Enable/Disable DB query logging commands #9264
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
Changes from 7 commits
565ba43
25adbf7
65f7b3c
d31bb17
a9a2257
b9afcf3
5d46ea8
cbfac52
3bb5d62
3f42615
09904a6
f3a5a90
8209744
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
|
||
namespace Magento\Developer\Console\Command; | ||
|
||
use Symfony\Component\Console\Command\Command; | ||
use Symfony\Component\Console\Input\InputInterface; | ||
use Symfony\Component\Console\Output\OutputInterface; | ||
use Magento\Framework\App\DeploymentConfig\Writer; | ||
use Magento\Framework\Config\File\ConfigFilePool; | ||
use Magento\Framework\DB\Logger\LoggerProxy; | ||
|
||
class QueryLogDisableCommand extends Command | ||
{ | ||
/** | ||
* command name | ||
*/ | ||
const COMMAND_NAME = 'dev:query-log:disable'; | ||
|
||
/** | ||
* @var Writer | ||
*/ | ||
private $deployConfigWriter; | ||
|
||
/** | ||
* QueryLogDisableCommand constructor. | ||
* @param Writer $deployConfigWriter | ||
* @param null $name | ||
*/ | ||
public function __construct( | ||
Writer $deployConfigWriter, | ||
$name = null | ||
) { | ||
parent::__construct($name); | ||
$this->deployConfigWriter = $deployConfigWriter; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function configure() | ||
{ | ||
$this->setName(self::COMMAND_NAME) | ||
->setDescription('Disable DB query logging'); | ||
|
||
parent::configure(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
* @throws \InvalidArgumentException | ||
*/ | ||
protected function execute(InputInterface $input, OutputInterface $output) | ||
{ | ||
$data = [LoggerProxy::PARAM_ALIAS => LoggerProxy::LOGGER_ALIAS_DISABLED]; | ||
$this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]); | ||
|
||
$output->writeln("<info>DB query logging disabled.</info>"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,109 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
|
||
namespace Magento\Developer\Console\Command; | ||
|
||
use Magento\Framework\DB\Logger\LoggerProxy; | ||
use Symfony\Component\Console\Command\Command; | ||
use Symfony\Component\Console\Input\InputArgument; | ||
use Symfony\Component\Console\Input\InputInterface; | ||
use Symfony\Component\Console\Output\OutputInterface; | ||
use Magento\Framework\App\DeploymentConfig\Writer; | ||
use Magento\Framework\Config\File\ConfigFilePool; | ||
|
||
class QueryLogEnableCommand extends Command | ||
{ | ||
/** | ||
* input parameter log-all-queries | ||
*/ | ||
const INPUT_ARG_LOG_ALL_QUERIES = 'log-all-queries'; | ||
|
||
/** | ||
* input parameter log-query-time | ||
*/ | ||
const INPUT_ARG_LOG_QUERY_TIME = 'log-query-time'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please remove "log-" prefix on all these options names There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
|
||
/** | ||
* input parameter log-call-stack | ||
*/ | ||
const INPUT_ARG_LOG_CALL_STACK = 'log-call-stack'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please rename to include-call-stack There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
/** | ||
* command name | ||
*/ | ||
const COMMAND_NAME = 'dev:query-log:enable'; | ||
|
||
/** | ||
* @var Writer | ||
*/ | ||
private $deployConfigWriter; | ||
|
||
/** | ||
* QueryLogEnableCommand constructor. | ||
* @param Writer $deployConfigWriter | ||
* @param null $name | ||
*/ | ||
public function __construct( | ||
Writer $deployConfigWriter, | ||
$name = null | ||
) { | ||
parent::__construct($name); | ||
$this->deployConfigWriter = $deployConfigWriter; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
protected function configure() | ||
{ | ||
$this->setName(self::COMMAND_NAME) | ||
->setDescription('Enable DB query logging'); | ||
|
||
$this->addArgument( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please change arguments to options, so that it will allow named parameters vs positional. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
self::INPUT_ARG_LOG_ALL_QUERIES, | ||
InputArgument::OPTIONAL, | ||
'Log all queries. Options: "true" or "false"', | ||
'true' | ||
); | ||
|
||
$this->addArgument( | ||
self::INPUT_ARG_LOG_QUERY_TIME, | ||
InputArgument::OPTIONAL, | ||
'Log query time.', | ||
'0.001' | ||
); | ||
|
||
$this->addArgument( | ||
self::INPUT_ARG_LOG_CALL_STACK, | ||
InputArgument::OPTIONAL, | ||
'Log call stack. Options: "true" or "false"', | ||
'true' | ||
); | ||
|
||
parent::configure(); | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
* @throws \InvalidArgumentException | ||
*/ | ||
protected function execute(InputInterface $input, OutputInterface $output) | ||
{ | ||
$data = [LoggerProxy::PARAM_ALIAS => LoggerProxy::LOGGER_ALIAS_FILE]; | ||
|
||
$logAllQueries = $input->getArgument(self::INPUT_ARG_LOG_ALL_QUERIES); | ||
$logQueryTime = $input->getArgument(self::INPUT_ARG_LOG_QUERY_TIME); | ||
$logCallStack = $input->getArgument(self::INPUT_ARG_LOG_CALL_STACK); | ||
|
||
$data[LoggerProxy::PARAM_LOG_ALL] = (int)($logAllQueries != 'false'); | ||
$data[LoggerProxy::PARAM_QUERY_TIME] = number_format($logQueryTime, 3); | ||
$data[LoggerProxy::PARAM_CALL_STACK] = (int)($logCallStack != 'false'); | ||
|
||
$this->deployConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]); | ||
|
||
$output->writeln("<info>DB query logging enabled.</info>"); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,105 @@ | ||
<?php | ||
/** | ||
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
namespace Magento\Framework\DB\Logger; | ||
|
||
class LoggerProxy extends LoggerAbstract | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. LoggerProxy should not extend the abstract, instead it should implement the LoggerInterface and delegate all calls to the class which extends the abstract, such as File or Quiet There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
{ | ||
/** | ||
* Logger alias param name | ||
*/ | ||
const PARAM_ALIAS = 'db_logger_mode'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please group Deployment configuration options related to logging together and rename them: 'db_logger' => [ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
|
||
/** | ||
* Logger log all param name | ||
*/ | ||
const PARAM_LOG_ALL = 'db_logger_all'; | ||
|
||
/** | ||
* Logger query time param name | ||
*/ | ||
const PARAM_QUERY_TIME = 'db_logger_query_time'; | ||
|
||
/** | ||
* Logger call stack param name | ||
*/ | ||
const PARAM_CALL_STACK = 'db_logger_stack'; | ||
|
||
/** | ||
* File logger alias | ||
*/ | ||
const LOGGER_ALIAS_FILE = 'file'; | ||
|
||
/** | ||
* Quiet logger alias | ||
*/ | ||
const LOGGER_ALIAS_DISABLED = 'disabled'; | ||
|
||
/** | ||
* @var LoggerAbstract | ||
*/ | ||
private $logger; | ||
|
||
/** | ||
* LoggerProxy constructor. | ||
* @param File $file | ||
* @param Quiet $quiet | ||
* @param bool $loggerAlias | ||
* @param bool $logAllQueries | ||
* @param float $logQueryTime | ||
* @param bool $logCallStack | ||
*/ | ||
public function __construct( | ||
File $file, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please avoid passing dependency which will never be used. It is better to pass Factory instead, which will create the right logger based on alias the first time logger is requested. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. Passed logger factories instead. |
||
Quiet $quiet, | ||
$loggerAlias, | ||
$logAllQueries = true, | ||
$logQueryTime = 0.001, | ||
$logCallStack = true | ||
) { | ||
switch ($loggerAlias) { | ||
case self::LOGGER_ALIAS_FILE: | ||
$this->logger = $file; | ||
break; | ||
default: | ||
$this->logger = $quiet; | ||
break; | ||
} | ||
|
||
parent::__construct($logAllQueries, $logQueryTime, $logCallStack); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I noticed that the This call to the parent method is not really needed because that's File or Quiet should be constructed with these parameters There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right. Now it's working. |
||
} | ||
|
||
/** | ||
* Adds log record | ||
* | ||
* @param string $str | ||
* @return void | ||
*/ | ||
public function log($str) | ||
{ | ||
$this->logger->log($str); | ||
} | ||
|
||
/** | ||
* @param string $type | ||
* @param string $sql | ||
* @param array $bind | ||
* @param \Zend_Db_Statement_Pdo|null $result | ||
* @return void | ||
*/ | ||
public function logStats($type, $sql, $bind = [], $result = null) | ||
{ | ||
$this->logger->logStats($type, $sql, $bind, $result); | ||
} | ||
|
||
/** | ||
* @param \Exception $e | ||
* @return void | ||
*/ | ||
public function critical(\Exception $e) | ||
{ | ||
$this->logger->critical($e); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please rename to include-all-queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done