-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
New Queries Panel with Improved EXPLAIN output #1648
Conversation
great job. looking at the animated gif, I got the feeling that something like "permanently allow sending data to mysqlexplain.com for this host" would be a great thing to have. that way a dev could one-time opt-in and would not need to click "OK" on every use. |
Yeah, thats possible. I just wanted to keep the config stuff minimal. I'll add it if @barryvdh thinks its a good idea to do. |
it could be remembered via cookie or localstorage in the browser, so no config needed (and it would have the benefit it is per user not per laravel instance)
👍 |
Works great over here, nice addition @tpetry! |
Looks good. First impressions (without installing):
|
Sure that would work. Its just that I wanted to omit to create a new config option. All the existing projects will probably never re-publish their config so they are not aware of any new option. And the goal of this PR is to help developers fix their performance issues. So the more that see this in their output, the more may use it and the less slow apps we hopefully have. So I've piggybacked it to the explain option because everyone who activates this wants to currently fix an issue. So it fits perfectly. The other option would be to activate it for any query by default (and deactivate it with a config value) as it doesn't have any runtime impact and is definetely something helpfull for anyone that expands the query details. |
yeah alternatively we could make an explain type/method/style option that defaults to either option (eg 'both', 'visual' or 'raw' or similar). I think visual explain is a bit less impact (no additional queries), but a bit more data. |
I think providing both is best. Because there are still some devs that know how to read the EXPLAIN output and don't want to or need any other tool to help them. And honestly, it looks nice having the raw data and the option to get a more helpfull optional representation. The impact of the visual mode is tiny. Its only adding the query and bindings to a statement's information. Compared to a real EXPLAIN its much less. |
Yeah but that's why I'm not sure adding both is the best option vs just adding the visual one. |
As discussed in DM; we might be able to do something to run the regular explain queries on-demand also, to avoid the overhead altogether :) |
With the new version, EXPLAIN is not run by default anymore for every SQL queries as before. The runtime impact was quite high when hundreds or thousands of queries had been executed. The lower runtime impact has an additional benefit. Currently, the ability to run EXPLAINs is controlled by a configuration setting that is default off. So EXPLAIN could now change to default on in the future as the impact should be minimal. 🔥 |
THanks! |
} else { | ||
$ul.append($li.clone().append(values[i])); | ||
$ul.append($li.clone().text(values[i])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pr-ing the needed change, I'll completely missed that when building it 🙈
👏 @tpetry Great job, thank you very much! Looking forward to trying it out. |
Unfortunately seems like "Visual Explain" does not work in MariaDB SQLSTATE[HY000]: General error: 1791 Unknown EXPLAIN format name: 'TREE' (SQL: EXPLAIN FORMAT=TREE |
Yes, visual explain for MariaDB is not supported by mysqlexplain.com yet. I've made a PR to hide this option now for MariaDB. |
So I've completely rewritten the queries panel (with as few changes as possible to the
QueryCollector
as possible. My goal was to replace the current EXPLAIN information with something more usefull: good output of the existing data and visual explains - because most devs are not database experts.If you want to try out this PR:
config/debugbar.php
the explain option is activated.Improvements
🔥Visual Explain
I've added the option to get visual explain plans for MySQL and PostgreSQL.
Explain Output
The EXPLAIN information was before added under the query with minimal information. And everything got very hard to read once a query had more than e.g. 20 EXPLAIN lines. Now everything is hidden by default and visible with all details in the query details.
Additionally I had to remove the SQLite EXPLAIN output. It showed the machine code how a SQLite query is executed. That's not useable to anyone. Nobody can read it! There sadly doesn't exist anything yet to make SQLite query plans readable - only the SQLite cli client can show something useful but this feature is not available to PHP.
UX Improvements
I copied the design and behaviour of the existing debugbar widget as closely as possible and only made tiny changes where the user experience could be improved a lot:
Raw SQL
The old code used a custom implementation to show a query with all bindings embedded into the query. The implementation has some issues that some bindings would not be correctly applied in some edge cases. Happily, this is a feature that Laravel provides - because I've submitted it. So when at least Laravel 10.15 is used, the frameworks implementation with more sophisticated logic is used. This also resolves PR #1627.
Note
The custom implementation can be completely removed because of this change when Laravel 9 support is dropped at some point in the future.
Changes
I tried to make as few changes as possible to the existing implementation. Most changes are done to the
QueryCollector
because:addQuery
method already transformed the incoming data into a representation that is needed for displaying the data but the original information was required within thecollect
method. So the transformation has been moved to thecollect
method.collect
method has been removed and implemented more intelligently on the js side.src/Resources/queries/widget.js
closely resembles the behaviour of the original widget implementation - except of the features document above. I did my best to make the code much more easy to read and understand compared to the original implementation.