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

NEW Add a trace comment for queries in dev mode #11065

Merged

Conversation

GuySartorelli
Copy link
Member

@GuySartorelli
Copy link
Member Author

mariadb failure is unrelated.

@GuySartorelli
Copy link
Member Author

Rebased to resolve that old mariadb failure

Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Try installing debugbar - it also shows where sql originated from, though in some cases there were different results

Either this is correct and debugbar is wrong, or vice versa

Also there were some cases where debugbar was getting the source from and this wasn't

Possibly just copy whatever debugbar does?

Also probably exclude running this code if debugbar is running, as there's no need to output where the call originates from twice

src/Dev/SapphireTest.php Outdated Show resolved Hide resolved
src/ORM/Connect/DBQueryBuilder.php Outdated Show resolved Hide resolved
src/ORM/Connect/DBQueryBuilder.php Outdated Show resolved Hide resolved
@GuySartorelli
Copy link
Member Author

GuySartorelli commented Feb 7, 2024

Try installing debugbar - it also shows where sql originated from, though in some cases there were different results
Either this is correct and debugbar is wrong, or vice versa

Looks like both were wrong 😅
I've made the tracing more robust here. See lekoala/silverstripe-debugbar#152 for the problem in debugbar.

Also there were some cases where debugbar was getting the source from and this wasn't

I've identified a couple of scenarios where that happens - one is Permission::permissions_for_member() and the other is Versioned::determineVersionNumberByStage(). Both of those skip this code, by making truly raw SQL queries. In those cases, I don't think we should try to be clever and inject stuff into the query - if it's a truly raw query like this it should be left untouched. Probably we should update those queries to use the ORM but that's not in scope for this card.

Also probably exclude running this code if debugbar is running, as there's no need to output where the call originates from twice

I'm not going to add coupling from framework to debugbar. I'll was going to open a card in debugbar to strip these comments since it has its own way of pointing to the query origin - but now that this PR introduces config to opt into the trace comment, developers should just not opt in if they're using debugbar.

@GuySartorelli GuySartorelli force-pushed the pulls/5/trace-queries branch 2 times, most recently from f48e21c to f1eecdd Compare February 7, 2024 23:31
Copy link
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

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

Noticed a handful of queries not getting comments when running ?showqueries=1 inside the cms

0008: SELECT "Code" FROM "Permission" WHERE "Type" = 1 AND "GroupID" IN (2) UNION SELECT "Code" FROM "PermissionRoleCode" PRC INNER JOIN "PermissionRole" PR ON PRC."RoleID" = PR."ID" INNER JOIN "Group_Roles" GR ON GR."PermissionRoleID" = PR."ID" WHERE "GroupID" IN (2) 0.0003s 

0009: SELECT "Code" FROM "Permission" WHERE "Type" = -1 AND "GroupID" IN (2) 0.0001s 

0014: SELECT "ID", "Version" FROM "SiteTree" 0.0001s 

0015: SELECT "ID", "Version" FROM "SiteTree_Live" 0.0001s 

Also I think this is worth being to be able to enable via an environment variable so that platform engineers can switch it on more easily

@GuySartorelli GuySartorelli force-pushed the pulls/5/trace-queries branch 5 times, most recently from a3bc8b6 to e07fcb4 Compare February 8, 2024 02:42
@GuySartorelli
Copy link
Member Author

I've removed the withEnvironmentType() stuff now, since we're using config and environment variables to control whether comments are included or not.

@GuySartorelli
Copy link
Member Author

Noticed a handful of queries not getting comments when running ?showqueries=1 inside the cms

I addressed that in #11065 (comment)

Also I think this is worth being to be able to enable via an environment variable so that platform engineers can switch it on more easily

Done

@emteknetnz emteknetnz merged commit 5e53dbc into silverstripe:5 Feb 8, 2024
15 checks passed
@emteknetnz emteknetnz deleted the pulls/5/trace-queries branch February 8, 2024 03:29
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