Matomo: Enabling SQL profiler on Response should be by setting enable_sql_profiler and not by debug

[nicoaravena]

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]

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]

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]

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.

[nicoaravena]

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]

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.