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

Support ability to sort queries by alphabetical order #1669

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AlexJump24
Copy link
Contributor

Hello,

In a previous PR meant to thank you for the brilliant package that has been so useful over the years!

I'm putting in this PR in the hope it will be useful to help resolve a problem I've been facing when looking through the queries tab on the debugbar to debug duplicate N+1 queries. On the particular project I was debugging there were a lot of duplicates and I was finding it difficult to determine which queries were appearing the most as duplicates and how many there were of them.

So I had an idea to maybe support the ability of sorting the queries collector by a custom sort, in this case alphabetical order. Testing this help really helped me quickly identify which queries were having the most N+1 issues.

A few thought processes I had.

  1. I wasn't a huge fan of having a string key in the config, when everything is pretty much driven by booleans, however I thought there would be potential to define multiple custom sort options going forward e.g. sort by query execution time if you want to look through queries in order that way.
  2. When initially testing I was simply ordering the query but of course this just ordered directly via the selected columns, I feel the most suitable option here would be to sort via everything after the table name. Here I have grabbed everything after the word FROM which from my understanding should always be present as the Laravel query builder requires a table? Initial testing on a few projects this seemed to work nicely and grouped duplicate queries alongside one another in their execution order.
  3. Naming was hard, and happy to change things if required.
  4. I have left the default sort to be execution_order which completely bypasses the usort so there is no performance degradation by default.
image

Hopefully this is something that is suitable to add into the package as I think it would add a lot of value debugging and as previously mentioned open scope to sorting in order ways.

Cheers.

@parallels999
Copy link
Contributor

Wouldn't this affect the waterfall timeline?

@AlexJump24
Copy link
Contributor Author

Wouldn't this affect the waterfall timeline?

I've given this a quick test and the timeline appears to be the same, I may be missing something though 👍🏼

@parallels999
Copy link
Contributor

I mean, each query have startTime/endTime, the graph is generated based on that, if the order is changed, there would no longer be a waterfall.

$sql = (string) $query->sql;
$time = $query->time / 1000;
$endTime = microtime(true);
$startTime = $endTime - $time;

@parallels999
Copy link
Contributor

maximebf/php-debugbar#410
Also could be sorted by

  • Memory use
  • Duration

@AlexJump24
Copy link
Contributor Author

I mean, each query have startTime/endTime, the graph is generated based on that, if the order is changed, there would no longer be a waterfall.

$sql = (string) $query->sql;
$time = $query->time / 1000;
$endTime = microtime(true);
$startTime = $endTime - $time;

I think this can't happen as the sort doesn't affect the start time/end time and in the TimeDataCollector there the waterfall order is driven via the usort that sorts via the start time https://github.com/maximebf/php-debugbar/blob/master/src/DebugBar/DataCollector/TimeDataCollector.php#L228

@parallels999
Copy link
Contributor

parallels999 commented Sep 12, 2024

the waterfall order is driven via the usort that sorts via the start time https://github.com/maximebf/php-debugbar/blob/master/src/DebugBar/DataCollector/TimeDataCollector.php#L228

public function collect()

QueryCollector overwrite collect(), I have no idea why you bring it to the table.

I didn't mean that, but never mind, I'm not the one who does the merges anyway.

It seems that you have not used the timeline option in QueryCollector, set 'duration_background' => true,and se the magic, also 'timeline' => true,

'duration_background' => true, // Show shaded background on each query relative to how long it took to execute.

'timeline' => false, // Add the queries to the timeline

@AlexJump24
Copy link
Contributor Author

Not 100% sure what you are trying to say still but now I realise it makes no sense to keep doing it on the addQuery rather than the collect. I have been testing with the two configuration options set to true and pre and post change the timeline is rendering exactly the same.

Thanks.

@parallels999
Copy link
Contributor

When you say "timeline", you are looking at Timelime Tab? or at The Queries Tab?
because all the time I have been talking about the queries tab, there is a timeline waterfall on queries tab

@AlexJump24
Copy link
Contributor Author

When you say "timeline", you are looking at Timelime Tab? or at The Queries Tab? because all the time I have been talking about the queries tab, there is a timeline waterfall on queries tab

I see what you mean now thanks I had never even realised that was a thing on there! Yeah you are correct it is affected on there, although I guess that would be expected in this context if you changed the sort. Would the waterfall be needed if sorted, as you said I was looking at the timeline tab where it is still visible. If not resolvable no problem I will just tweak things when I require but thought it would be a useful potential feature even if my implementation doesn't end up being suitable.

@parallels999
Copy link
Contributor

Also there is database events, like transaction,commit,rollback and others,
these separate query blocks based on their execution time.
What would happen to them in this order?

public function collectTransactionEvent($event, $connection)
{
$this->transactionEventsCount++;
$source = [];
if ($this->findSource) {
try {
$source = $this->findSource();
} catch (\Exception $e) {
}
}
$this->queries[] = [
'query' => $event,

$events->listen(
\Illuminate\Database\Events\TransactionBeginning::class,
function ($transaction) {
$this['queries']->collectTransactionEvent('Begin Transaction', $transaction->connection);
}
);
$events->listen(
\Illuminate\Database\Events\TransactionCommitted::class,
function ($transaction) {
$this['queries']->collectTransactionEvent('Commit Transaction', $transaction->connection);
}
);
$events->listen(
\Illuminate\Database\Events\TransactionRolledBack::class,
function ($transaction) {
$this['queries']->collectTransactionEvent('Rollback Transaction', $transaction->connection);
}
);
$events->listen(
'connection.*.beganTransaction',
function ($event, $params) {
$this['queries']->collectTransactionEvent('Begin Transaction', $params[0]);
}
);
$events->listen(
'connection.*.committed',
function ($event, $params) {
$this['queries']->collectTransactionEvent('Commit Transaction', $params[0]);
}
);
$events->listen(
'connection.*.rollingBack',
function ($event, $params) {
$this['queries']->collectTransactionEvent('Rollback Transaction', $params[0]);
}
);
$events->listen(
function (\Illuminate\Database\Events\ConnectionEstablished $event) {
$this['queries']->collectTransactionEvent('Connection Established', $event->connection);
if (app('config')->get('debugbar.options.db.memory_usage')) {
$event->connection->beforeExecuting(function () {
$this['queries']->startMemoryUsage();
});
}
}
);

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.

2 participants