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

Jump to bottom

Enabling SQL profiler on Response should be by setting enable_sql_profiler and not by debug #14219

Open
nicoaravena opened this issue Mar 18, 2019 · 5 comments
Open

Enabling SQL profiler on Response should be by setting enable_sql_profiler and not by debug #14219

nicoaravena opened this issue Mar 18, 2019 · 5 comments
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Milestone
Backlog (Help wan...

Comments

@nicoaravena
Copy link

nicoaravena commented Mar 18, 2019 •
edited

Hi there! I've been working on a fork of piwik (2.x) and I notice that when you enable the debug option, the database profiler it start to work and searching the origin of this I found in core/Tracker/Response.php this:

class Response
{
    private $timer;

    private $content;

    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()) {
            $this->timer = new Timer();

            TrackerDb::enableProfiling();
        }
    }
...

The problem is that the function isDebugModeEnabled() search for a global where it depends on the debug value in the config.php.ini and it should be validated by the setting enable_sql_profiler (as I mention in the title)

My propose is this:

    public function init(Tracker $tracker)
    {
        // remove ob_start();
        if ((bool) TrackerConfig::getConfigValue('enable_sql_profiler')) {
            $this->timer = new Timer();

            TrackerDb::enableProfiling();
        }
    }

Also notice that ob_start(); cause some problems with the printDebug function (specifically with the CLI), it is completely necessary? I think this could be removed also.

I forgot to say that I search in both branch version (2.x-dev and 3.x-dev) and this is untouched between those version changes.
@tsteur
Copy link
Member

tsteur commented Mar 18, 2019

The global is set depending on the debug tracker config setting. Not sure why it should be changed or what the benefit would be?

@nicoaravena
Copy link
Author

I think you misunderstood me, I was not saying to change the global debug, what I mean to say is that debugging is not always mandatory when you don't want to profile queries, because it increment process time when you are testing requests with a big records.

That is why I recommend to change the if condition.

Maybe this apply too if ($tracker->isDebugModeEnabled() && (bool) TrackerConfig::getConfigValue('enable_sql_profiler'))

@tsteur
Copy link
Member

tsteur commented Mar 18, 2019

Makes sense. The tracker doesn't have such a setting currently. Feel free to create a PR and add the setting [Tracker] enable_sql_profiler = 1.

Btw: we really don't recommend using 2.X anymore. It's not considered secure.

@tsteur tsteur added Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement. c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. labels Mar 18, 2019
@tsteur tsteur modified the milestones: 3.9.0, Backlog (Help wanted) Mar 18, 2019
@nicoaravena
Copy link
Author

What about the existent [Debug] enable_sql_profiler = 1 in `global.ini.php? This should be used for this purpose imo.

Also, as a team we are trying to upgrade as soon as we can.

@tsteur
Copy link
Member

tsteur commented Mar 18, 2019

We're trying to keep settings for tracker separate cause you may not want to activate the profiler for tracker when you enable the tracker for the UI / archiver eg for performance reasons. Enabling the profiler for tracker could result in quite some extra load on the DB I suppose.

@nicoaravena nicoaravena reopened this Mar 19, 2019
@simivar simivar mentioned this issue Mar 29, 2019
Merged
@tsteur tsteur mentioned this issue Apr 11, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: Platform For Matomo platform changes that aren't impacting any of our APIs but improve the core itself. Help wanted Beginner friendly issues or issues where we'd highly appreciate community's help and involvement.
Projects
None yet
Milestone
Backlog (Help wanted)
Development

No branches or pull requests
2 participants
@tsteur @nicoaravena