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.

Sign up for GitHub

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

Jump to bottom

New enable_sql_profiler INI setting for Tracker #14288

Merged
merged 2 commits into from Apr 11, 2019
Merged

New enable_sql_profiler INI setting for Tracker #14288

merged 2 commits into from Apr 11, 2019

Conversation

simivar
Copy link
Contributor

@simivar simivar commented Mar 29, 2019

@Findus23 Findus23 added the Needs Review PRs that need a code review label Mar 29, 2019
@Findus23
Copy link
Member

I feel like this might get really confusing with the existing enable_sql_profiler setting:

matomo/config/global.ini.php

Lines 142 to 145 in f690829

; if set to 1, all the SQL queries will be recorded by the profiler
; and a profiling summary will be printed at the end of the request
; NOTE: you must also set [log] log_writers[] = "screen" to enable the profiler to print on screen
enable_sql_profiler = 0

@simivar
Copy link
Contributor Author

simivar commented Mar 29, 2019

That was @tsteur proposition from #14219. I feel like it can be confusing too but do you have any idea how it should be name? I can't think of anything myself.

@tsteur
Copy link
Member

tsteur commented Mar 30, 2019

@Findus23 using the sql profiler in tracking you don't want to use when you only want to use the SQL profiler for archiving or web.

tsteur
tsteur reviewed Apr 11, 2019
View reviewed changes
core/Tracker/Response.php
@@ -25,7 +25,7 @@ public function init(Tracker $tracker)
{
ob_start(); // we use ob_start only because of Common::printDebug, we should actually not really use ob_start

if ($tracker->isDebugModeEnabled()) {
if ($tracker->isDebugModeEnabled() && TrackerConfig::getConfigValue('enable_sql_profiler')) {
Copy link
Member

Choose a reason for hiding this comment

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

@simivar I'm wondering maybe it can be useful to enable enable_sql_profiler without enabling $tracker->isDebugModeEnabled()? Not sure... If we make it independent, we would also need to adjust the code in line 90 where we print the profiling results. I suppose it's fine though. Maybe let's just leave it like this for now.

@tsteur
Adjust message
0e584f3 
@simivar fyi removed the need for `[log]` as it should be 99% enabled. It's just important they also enable tracker debug.
tsteur added a commit to matomo-org/developer-documentation that referenced this pull request Apr 11, 2019
@tsteur
Document new SQL profiler setting
a4d5f0a 
refs matomo-org/matomo#14288
@tsteur tsteur mentioned this pull request Apr 11, 2019
Merged
@tsteur tsteur merged commit 1d9bcc8 into matomo-org:3.x-dev Apr 11, 2019
@tsteur
Copy link
Member

tsteur commented Apr 11, 2019

Thanks for this @simivar Also created https://github.com/matomo-org/developer-documentation/pull/302/files to adjust the docs.

mattab pushed a commit to matomo-org/developer-documentation that referenced this pull request May 20, 2019
@tsteur @mattab
Document new SQL profiler setting (#302)
4411aed 
refs matomo-org/matomo#14288
@mattab mattab changed the title Add enable_sql_profiler setting to Tracker New enable_sql_profiler INI setting for Tracker Jun 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tsteur tsteur tsteur left review comments

Assignees
No one assigned
Labels
Needs Review PRs that need a code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@simivar @Findus23 @tsteur