DB Query Monitor

[felipeelia]

Description of the Change

This PR adds a "submodule" to Support Monitor, logging, and sending heavy SQL queries.

It also remove some instance()->setup() calls, as instance() already calls setup() in its first execution (what was causing hooks to be added twice.)

Benefits

While updating plugins on staging we can detect any major changes in the database structure, so we can plan accordingly the same changes for production.

Verification Process

1. Installed the plugin in a WordPress test installation
2. Installed Support Monitor in another WP test install
3. Checked messages in the "system" group coming

Changelog Entry

Added: DB Query Monitor

[bengreeley]

@felipeelia I think this is a good start - I left some comments and suggestions, let me know if you have any questions

[bengreeley]

@felipeelia How will we be making sure $query doesn't contain any sensitive information such as user e-mail addresses or other information? We'll want to be really careful about sending that sort of sensitive information to Support Monitor where we could be potentially liable for storing that information if we were ever hacked. I'd recommend we do some sort of cleanup or see if you can get the version of the query that has placeholders like %s in place of actual data.

[felipeelia]

@bengreeley my bad on this one. Trying to get the version of the query with placeholders can be hard (or near impossible depending on the plugin), so cleaning it up would be probably easier. I'll work on that, thanks!

[felipeelia]

@bengreeley with the change I made, I'm curious to know your opinion about this topic. We are not sending the query anymore but only storing it in a transient. As the entire feature is only available in non-production environments, do you think we still need to escape the queries? (I don't want to make it insecure but also don't want to over-engineer it). If we decide to escape them, would you be okay with using a lib like this?

[bengreeley]

I wonder if you create a regex or search/replace to remove anything between '' and "" if that would be sufficient? I'm not as worried if we're storing it on the server, but we'll want to be aware that if we have queries that have sensitive user information that's available to anybody or any plugin, we'll want to think of a way to clean it up.As long as it's information that is freely available in any other WP table it shouldn't be a bit deal, but should be given adequate testing.

[felipeelia]

@bengreeley I've pushed a commit that does that. When I save a long post these are the queries being saved (the post update and the revision created):

Query: UPDATE `wp_posts` SET `post_author` = 1, `post_date` = ?, `post_date_gmt` = ?, `post_content` = ?, `post_content_filtered` = ?, `post_title` = ?, `post_excerpt` = ?, `post_status` = ?, `post_type` = ?, `comment_status` = ?, `ping_status` = ?, `post_password` = ?, `post_name` = ?, `to_ping` = ?, `pinged` = ?, `post_modified` = ?, `post_modified_gmt` = ?, `post_parent` = 0, `menu_order` = 0, `post_mime_type` = ?, `guid` = ? WHERE `ID` = 7
File: /var/www/html/wp-includes/post.php
Line: 4196
Count: 1

Query: INSERT INTO `wp_posts` (`post_author`, `post_date`, `post_date_gmt`, `post_content`, `post_content_filtered`, `post_title`, `post_excerpt`, `post_status`, `post_type`, `comment_status`, `ping_status`, `post_password`, `post_name`, `to_ping`, `pinged`, `post_modified`, `post_modified_gmt`, `post_parent`, `menu_order`, `post_mime_type`, `guid`) VALUES (1, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, 7, 0, ?, ?)
File: /var/www/html/wp-includes/post.php
Line: 4219
Count: 1

Does that look okay?

[felipeelia]

@bengreeley I've refactored the code a bit to display the queries in the site rather than sending them to Support Monitor and also added some docs. Do you mind giving it another look? Thank you very much for all the feedback so far!

ps.: I'm still considering this WIP as I have to test it with a multisite and also add a slack message in the case any heavy query was detected. Any thoughts on this, btw?

[bengreeley]

ps.: I'm still considering this WIP as I have to test it with a multisite and also add a slack message in the case any heavy query was detected. Any thoughts on this, btw?

@felipeelia To integrate with Slack, you'll want to look in the Support Monitor Dashboard repo and there will be checks performed during the message ingestion. I'd recommend to send this information with the 'systems' insight so you can piggy-back off that functionality and then in the message ingestion code you would want to check for the heavy query variable that was sent via the message.

Check out this code for reference: https://gitlab.10up.com/10up-internal/system-report/system-report-dashboard/-/blob/master/plugins/support-monitor/includes/functions/api/rest-api.php#L427

[felipeelia]

@bengreeley I've refactored the code to make the entire feature opt-in, rather than opt-out. You'll see that there are two core concepts: availability (the feature is only available in non-prod envs by default) and state (enabled/disabled). To have it working, it needs to be available AND enabled.

For the slack integration, I talked to @tott and we are seeing it as a "phase two", so let's ignore that for now.

Do you mind giving it yet another review and sharing your thoughts about it? When it looks okay, I need to change the @since x.x strings to the actual version.

Thanks!

[bengreeley]

I think the changes look better, thanks for making those @felipeelia ! We'll want to do thorough testing on some internal environments before using this on production environments and also test with Support Monitor to make sure that data is brought over correctly. You'll likely need to make some changes to that code to accommodate for this new field/data. There's a staging environment we can test on as well if we want to test these changes on a staging version of Support Monitor.

I'll hold off on approving this until it's been tested, but so far so good!

[felipeelia]

Thanks, @bengreeley! I'll reach out internally about the best environments to use to test it. Regarding changes needed in the Support Monitor, I'm already testing it locally and as the field is just added to the message, it worked out of the box :)