Implement garbage collection of URL metrics including at uninstallation #1055
Conversation
westonruter
added
[Type] Enhancement
westonruter
force-pushed
the
add/optimization-detective-garbage-collection
branch
from
3b9f27c to
a52e0ce
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| public static function schedule_garbage_collection() { | ||
| if ( ! is_user_logged_in() ) { | ||
| return; | ||
| } | ||
|
|
||
| // Unschedule any existing event which had a differing recurrence. | ||
| $scheduled_event = wp_get_scheduled_event( self::GC_CRON_EVENT_NAME ); | ||
| if ( $scheduled_event && self::GC_CRON_RECURRENCE !== $scheduled_event->schedule ) { | ||
| wp_unschedule_event( $scheduled_event->timestamp, self::GC_CRON_EVENT_NAME ); | ||
| $scheduled_event = null; | ||
| } | ||
|
|
||
| if ( ! $scheduled_event ) { | ||
| wp_schedule_event( time(), self::GC_CRON_RECURRENCE, self::GC_CRON_EVENT_NAME ); | ||
| } | ||
| } |
There was a problem hiding this comment.
| add_action( 'admin_init', array( __CLASS__, 'schedule_garbage_collection' ) ); | ||
| add_action( self::GC_CRON_EVENT_NAME, array( __CLASS__, 'delete_stale_posts' ) ); |
There was a problem hiding this comment.
| public static function delete_all_posts() { | ||
| global $wpdb; | ||
|
|
||
| // phpcs:disable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching | ||
|
|
||
| // Delete all related post meta for URL Metrics posts. | ||
| $wpdb->query( | ||
| $wpdb->prepare( | ||
| " | ||
| DELETE meta | ||
| FROM $wpdb->postmeta AS meta | ||
| INNER JOIN $wpdb->posts AS posts | ||
| ON posts.ID = meta.post_id | ||
| WHERE posts.post_type = %s; | ||
| ", | ||
| self::SLUG | ||
| ) | ||
| ); | ||
|
|
||
| // Delete all URL Metrics posts. | ||
| $wpdb->delete( | ||
| $wpdb->posts, | ||
| array( | ||
| 'post_type' => self::SLUG, | ||
| ) | ||
| ); | ||
|
|
||
| // phpcs:enable WordPress.DB.DirectDatabaseQuery.DirectQuery, WordPress.DB.DirectDatabaseQuery.NoCaching | ||
| } |
There was a problem hiding this comment.
plugins/optimization-detective/class-od-url-metrics-post-type.php
Outdated
Show resolved
Hide resolved
plugins/optimization-detective/class-od-url-metrics-post-type.php
Outdated
Show resolved
Hide resolved
| foreach ( $query->posts as $post ) { | ||
| if ( self::SLUG === $post->post_type ) { // Sanity check. | ||
| wp_delete_post( $post->ID, true ); | ||
| } | ||
| } |
There was a problem hiding this comment.
I wonder how costly this is, given it can be 100 posts. It may be more efficient to run custom DB queries like you do in the delete_all_posts() method, using ID IN (... ), rather than lots of individual queries plus all those hooks.
There was a problem hiding this comment.
In this case I was thinking it is worthwhile to run the hooks since the plugin code is not uninstalled and there should be potential side effects.
plugins/optimization-detective/class-od-url-metrics-post-type.php
Outdated
Show resolved
Hide resolved
| add_action( 'admin_init', array( __CLASS__, 'schedule_garbage_collection' ) ); | ||
| add_action( self::GC_CRON_EVENT_NAME, array( __CLASS__, 'delete_stale_posts' ) ); |
There was a problem hiding this comment.
It's a bit odd adding general hooks in the same method where the post type is registered. Two alternative approaches I think would be cleaner:
- Either break out those two hook additions into a separate method, e.g.
add_hooks(). - Or use this method as the single "add hook" entry point (could be called
add_hooks(), or you stick withregister()), and move theregister_post_type()call into a distinctregister_post_type()method, which this entry point method then adds as aninitaction callback.
I think I prefer the second approach. This way you have one dedicated method to register the post type, but the only method that needs to be called to bootstrap what the class does is the one entry point method which adds the three hooks.
plugins/optimization-detective/class-od-url-metrics-post-type.php
Outdated
Show resolved
Hide resolved
…om/WordPress/performance into add/optimization-detective-garbage-collection * 'feature/image-loading-optimization' of https://github.com/WordPress/performance:
|
@westonruter Pending #1055 (comment), this LGTM. :) |
| require_once __DIR__ . '/class-od-data-validation-exception.php'; | ||
| require_once __DIR__ . '/class-od-url-metric.php'; | ||
| require_once __DIR__ . '/class-od-url-metrics-group.php'; | ||
| require_once __DIR__ . '/class-od-url-metrics-group-collection.php'; | ||
| require_once __DIR__ . '/class-od-storage-lock.php'; |
There was a problem hiding this comment.
I guess all of these should go into storage then too.
There was a problem hiding this comment.
@westonruter Maybe. OD_Storage_Lock probably yes, for the URL metric classes not necessarily - though I don't feel strongly about it. Those are the central infrastructure classes used for the entire functionality, IMO they aren't just storage related as e.g. the post type class is.
I'm okay with either, though please align the test files structure with whatever you go with here as well (e.g. right now the post type class tests are not in the storage folder even though that class is in the storage folder). Nit-picking... :)
westonruter
force-pushed
the
add/optimization-detective-garbage-collection
branch
from
b5623fd to
814f57d
Compare
westonruter
merged commit 25bebc1
into
feature/image-loading-optimization
Summary
This PR implements two things:
uninstall.phpwhich deletes all of the URL Metrics and unschedules the garbage collection event.Fixes #895
Relevant technical choices
Note that the first commit in this PR (21a5cec) refactors the post type logic from a set of functions to a single class
OD_URL_Metrics_Post_Type. To facilitate review, it may be helpful to look at that commit separately from the ones that follow: 21a5cec...1a9532c.This implements garbage collection of URL Metrics posts which haven't been modified in a month. By default, a URL Metric has a freshness TTL of 1 day. This means that after 1 day, the detection logic will attempt to gather a new URL Metric to refresh the one that is stale. There is also a sample size of 3 for each URL Metrics group (divided by the breakpoints), so if there are 3 breakpoints there would be 4 groups each of which seeks to have a total of 3 fresh URL Metrics. So there could be a maximum of 12 updates to a given URL Metrics post on a given day. If there are no visitors to a given URL for an entire month, and thus the URL Metrics post for that URL hasn't been modified in a month, then it is eligible to be deleted. The garbage collection process is configured to run on a daily basis.
An
uninstall.phpis also added which deletes all URL Metrics posts from the current site and other sites on a multisite network (up to 100). The post deletion here is done in batch using$wpdb->delete()for the sake of speed. We used the same approach in the AMP plugin.