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

Enable/Disable DB query logging commands #9264

Merged
merged 13 commits into from
Apr 19, 2017
Merged

Enable/Disable DB query logging commands #9264

merged 13 commits into from
Apr 19, 2017

Conversation

federivo
Copy link
Member

@federivo federivo commented Apr 15, 2017

Description

Added console commands for enabling/disabling the db query logging.

Fixed Issues (if relevant)

  1. Create new CLI command: Enable DB logging #9200: Create new CLI command: Enable dev settings, i.e DB logging, Profiler, Template Hints

Manual testing scenarios

  1. Run bin/magento dev:query-log:enable command.
  2. Clear configuration cache.
  3. Make any request to the Magento site and verify that the DB query log is being written.
  4. Run bin/magento dev:query-log:disable command.
  5. Clear configuration cache
  6. Make any request to the site and verify that the DB query log have no new log data.

@vrann vrann self-assigned this Apr 15, 2017
@vrann vrann added this to the April 2017 milestone Apr 15, 2017
*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$data = ['db_logger_alias' => 'quiet'];
Copy link
Contributor

Choose a reason for hiding this comment

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

  • please use constants for db_logger_alias and quiet
  • db_logger_alias is not the best name for the config parameter, because we used it to set the mode of the logger: "writing to file" vs "quiet". I would suggest something like "db logger mode"
  • for the same reason, "quiet" might be renamed to "disabled"

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

Writer $deploymentConfigWriter,
$name = null
)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

braces should be on the same line

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

*/
protected function execute(InputInterface $input, OutputInterface $output)
{
$data = ['db_logger_alias' => 'file'];
Copy link
Contributor

Choose a reason for hiding this comment

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

you already have constant for the "file" which can be used here. Though, I'd suggest to move it out from the command to the proxy or interface

Copy link
Contributor

Choose a reason for hiding this comment

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

just noticed that you have constant in proxy, please use it here

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

$data = ['db_logger_alias' => 'file'];
$this->deploymentConfigWriter->saveConfig([ConfigFilePool::APP_ENV => $data]);

// $data = [
Copy link
Contributor

Choose a reason for hiding this comment

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

dead code should be eliminated

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

/**
* File logger alias
*/
const FILE_LOGGER_ALIAS = 'file';
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use constant from the proxy instead of this one

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

/**
* Logger alias param name
*/
const PARAM_ALIAS = 'db_logger_alias';
Copy link
Contributor

Choose a reason for hiding this comment

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

per comment earlier please rename the value of the param from "db_logger_alias" to "db_logger_mode" or something similar.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

// $data = [
// 'db_logger' => [
// 'logger_alias' => 'file',
// 'log_all_queries' => false,
Copy link
Contributor

Choose a reason for hiding this comment

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

For simplicity, we might just default these paratemeters to:

  • log all queries => true
  • log query time => 0.001
  • log call stack => true
    This will be the minimum requirement.

Two other options requires more implementation:

  • parameters passed to the proxy in the same way as the alias for the logger through the DI and env.php
  • create virtual types for the file writer with the different configuration, i,e FileLogAllWithStack and FileLogNoStack

Copy link
Member Author

Choose a reason for hiding this comment

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

Chose to handle the params the same as the alias. Need your feedback on that approach.

$logAllQueries = false,
$logQueryTime = 0.05,
$logCallStack = false
)
Copy link
Contributor

Choose a reason for hiding this comment

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

braces should be on the same line

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed

* @param bool $logCallStack
*/
public function __construct(
File $file,
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Passed logger factories instead.

/**
* Logger alias param name
*/
const PARAM_ALIAS = 'db_logger_mode';
Copy link
Contributor

Choose a reason for hiding this comment

The 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' => [
'output' => 'file',
'log_everything' => 1,
'query_time_threshold' => '0.001',
'include_stacktrace' => 1
]

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

$this->setName(self::COMMAND_NAME)
->setDescription('Enable DB query logging');

$this->addArgument(
Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

/**
* input parameter log-query-time
*/
const INPUT_ARG_LOG_QUERY_TIME = 'log-query-time';
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove "log-" prefix on all these options names
rename option to query-time-threshold

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

/**
* input parameter log-all-queries
*/
const INPUT_ARG_LOG_ALL_QUERIES = 'log-all-queries';
Copy link
Contributor

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

Copy link
Member Author

Choose a reason for hiding this comment

The 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';
Copy link
Contributor

Choose a reason for hiding this comment

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

please rename to include-call-stack

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

break;
}

parent::__construct($logAllQueries, $logQueryTime, $logCallStack);
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that the $logCallStack option does not work. I think the reason behind it is that File and Quiet should be constructed with this parameter passed to the constructor.

This call to the parent method is not really needed because that's File or Quiet should be constructed with these parameters

Copy link
Member Author

Choose a reason for hiding this comment

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

You're right. Now it's working.

*/
namespace Magento\Framework\DB\Logger;

class LoggerProxy extends LoggerAbstract
Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@magento-team
Copy link
Contributor

@federivo thank you for your contribution. Your Pull Request has been successfully merged

@mslabko
Copy link
Member

mslabko commented Apr 21, 2017

Hi @federivo,
Thank you for your contribution! Here is some code we use internally for debugging sql\app which is nice to have in CLI command tool to make it multifunctional like a Swiss Army knife :)

Dedicated "Up For Grabs" GitHub issue - #9341

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants