Add ability to prevent storing parsed CSS in transients #4177

westonruter · 2020-01-25T19:45:04Z

Note: This PR's true author is @schlessera!

Summary

Fixes #2092

Adds a new amp_parsed_css_transient_caching_allowed filter which allows a site to prevent storing parsed CSS in transients, by filtering the value to be false. This is important on sites which have highly variable CSS, resulting in a site filling up its wp_options table with transients.

Note that when transient storage is disabled, the parsed CSS is still cached in the object cache even when an external object cache is not available. This ensures that the same CSS appearing multiple times on a page will not have to be re-parsed.
Adds detection for a huge number of SELECT COUNT(*) FROM $wpdb->options WHERE option_name LIKE '_transient_amp-parsed-stylesheet%' and when a threshold is met, persists the fact that transient caching of stylesheets is disabled. This happens during a daily cron.
Adds Site Health warning when transient caching is disabled (and external object cache is not present).
Adds Site Health debugging information.
Adds a warning to the Network Plugins screen on every large sites, to inform the administrator that orphaned scheduled events will be left behind.

Site Health integration

Transient caching of stylesheets enabled

Transient caching of stylesheets disabled

Transient caching of stylesheets not applicable

Debug information

Network plugins screen warning

See #2092.

Checklist

My pull request is addressing an open issue (please create one otherwise).
My code is tested and passes existing tests.
My code follows the Engineering Guidelines (updates are often made to the guidelines, check it out periodically).

includes/sanitizers/class-amp-style-sanitizer.php

schlessera · 2020-03-19T06:00:06Z

⚠️ Force-pushing a rebase.

googlebot · 2020-03-19T16:06:16Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

schlessera · 2020-03-19T16:07:26Z

@googlebot I consent.

googlebot · 2020-03-19T16:07:32Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

schlessera · 2020-03-19T17:36:52Z

@westonruter To follow up with the discussion we just had in the sync call, we should talk about how best to handle this for now: https://github.com/ampproject/amp-wp/pull/4177/files#diff-4805c837d3390517773926fb26726bcdR306-R323

schlessera · 2020-03-19T17:37:17Z

Also, I'm open for any suggestions if you know a better way of testing the background task.

tests/php/test-class-monitor-css-transient-caching.php

src/BackgroundTask/MonitorCssTransientCaching.php

westonruter · 2020-03-19T23:17:52Z

amp.php

+// @todo Quick fix for now, no centralized flow yet to register services.
+foreach ( [
+	'AmpProject\AmpWP\BackgroundTask\MonitorCssTransientCaching',
+] as $background_task ) {
+	try {
+		( new $background_task() )->register();
+	} catch ( Exception $exception ) {
+		// @todo No logger yet, dump to error log?
+		$exception_class = get_class( $exception );
+		$error_message   = "Exception '{$exception_class}' thrown in background task '{$background_task}': {$exception->getMessage()}";
+		if ( class_exists( 'WP_CLI' ) ) {
+			WP_CLI::warning( $error_message );
+		} else {
+			error_log( $error_message ); // phpcs:ignore WordPress.PHP.DevelopmentFunctions.error_log_error_log
+		}
+	}
+}
+


To follow up with the discussion we just had in the sync call, we should talk about how best to handle this for now: https://github.com/ampproject/amp-wp/pull/4177/files#diff-4805c837d3390517773926fb26726bcdR306-R323

@schlessera I don't really have any better suggestion for the immediate term on where to put this, other than to plop it in amp.php like you've done. We'll need to look in the future about how better to organize the amp.php bootstrap as part of modularization effort.

The one thing that I think should be done, however, is to avoid calling this right here immediately when the bootstrap is being invoked. This will cause a DB write to occur and potentially race condition on a heavily-trafficked site. This would be adding one more place in addition to the frontend writes being called out in #3284.

Since register_activation_hook() doesn't work with multisite, what about using admin_init instead? This will ensure it only fires once a user has logged in. See: https://developer.wordpress.org/reference/functions/register_activation_hook/#comment-2100

Another option would be to put it inside of an amp_plugin_update action, but this still runs during frontend requests. As noted in #3284 (comment) it could also be moved to admin_init. In that way it would be similar to the trigger for the DB upgrade.

This will cause a DB write to occur and potentially race condition on a heavily-trafficked site.

AFAICT, right now, the DB writes are:

once per day in the register() task to schedule a new cron job if none is scheduled;

once per day in the process() that is run via cron.

Am I missing something?

Yes, there is the additional write to schedule the interval in the first place. That is, when register is called for the very first time upon upgrading, then wp_next_scheduled() will return false which will then cause wp_schedule_event() to be called:

amp-wp/src/BackgroundTask/CronBasedBackgroundTask.php

Lines 100 to 106 in d28a5ef

$timestamp = wp_next_scheduled( $event_name );

if ( $timestamp ) {

return $timestamp;

}

if ( ! wp_schedule_event( time(), $interval_name, $event_name ) ) {

This will then cause _set_cron_array() to be called, which will do a DB write. If wp_schedule_event() is limited to only happen at admin_init, then no cron update would happen on the frontend. When an event transpires, the next event would get scheduled during the cron request, not during a frontend request.

By putting this logic into an admin_init hook, this DB write to schedule the event won't happen on the frontend. Granted, this would mean splitting the register method into two: one to schedule and one to add the hooks. The schedule part could be done at admin_init whereas the hook addition would need to be done with every request (or at least when doing cron).

Maybe I'm being overly cautious about this, and there invariably a lot of other DB writes that happen during frontend requests (especially when transients are used without a persistent object cache). But I'm just trying to think of any ways to make this more robust.

I just realised that the schedule call should actually be done on plugin activation instead. That makes it more robust and ensures it is only run once, which is enough, as we are scheduling a recurring event.

@felixarntz But that will inevitably run into race conditions then, and we'll end up running the cron job multiple times per day, potentially disabling the transient caching much too soon.

Note: In this case, not running the cron job at all is highly preferable to running it multiple times because of a race condition.

Why would it not be reliable to cycle over all blogs (once!) on plugin activation (something that is under the control of the administrator, and listening to site creation?

@schlessera Potentially yes, but we can reduce the chance for this to a minimum, e.g. we could only schedule the cron job if an administrator is logged in. I would say it's highly unlikely that a site has so many administrators that more than one loads an admin page in the same given second right in the moment where this action is executed.

@schlessera

Why would it not be reliable to cycle over all blogs (once!) on plugin activation (something that is under the control of the administrator, and listening to site creation?

Yes we could do that but then you'll run into scaling issues with larger multisites.

schlessera · 2020-03-20T06:57:20Z

So I've added a very, very small "Services" subsystem now to get around the hacky instantiation that we had in the previous iteration of this PR.

This subsystem:

is very lightweight and straight-forward
doesn't use any magic of heavy engineering concepts
can be used gradually and does not need to be enforced

More importantly, though, it gives structure to the way we register, activate or deactivate code.

For the next piece of logic we want to add like this:

create a class that does its thing
add needed hooks into the register() method
if it needs to act on plugin activation, implement the HasActivation interface
if it needs to act on plugin deactivation, implement the HasDeactivation interface

No change to amp.php or other parts of the code needed anywhere, the change will be completely self-contained (which should be the goal of maintainable code).

Please let me know what you think!

schlessera · 2020-03-21T07:30:27Z

I added a screenshot with the warning to the description up above.

westonruter · 2020-03-21T20:57:20Z

Hummm. I'm seeing test failures/errors:

There was 1 error:

1) Test_Monitor_CSS_Transient_Caching::test_event_gets_scheduled_and_unscheduled
Error: Call to undefined method AmpProject\AmpWP\BackgroundTask\MonitorCssTransientCaching::activate()

/tmp/wordpress/src/wp-content/plugins/amp/tests/php/test-class-monitor-css-transient-caching.php:42

--

There were 2 failures:

1) Test_Monitor_CSS_Transient_Caching::test_event_can_be_processed
Failed asserting that false is not false.

/tmp/wordpress/tests/phpunit/includes/phpunit7/testcase.php:30
/tmp/wordpress/src/wp-content/plugins/amp/tests/php/test-class-monitor-css-transient-caching.php:66

2) Test_Monitor_CSS_Transient_Caching::test_transient_caching_is_disabled
false does not match expected type "array".

* Ensure that external object cache is disabled during tests. * Replace removed use of activate() method with call to schedule_event(). * Pass required attribute to deactivate() method.

…ling-parsed-css-transient-caching * 'develop' of github.com:ampproject/amp-wp: Fix passing of numbers to _n() Prevent case where validation error can be raised on already-removed node Fix MISSING_URL checks by inferring ALLOW_EMPTY from MANDATORY Tighten up validation error titles Remove markup from translations Make phpcs happy Remove DISALLOWED_TAG_MULTIPLE_CHOICES validation error code Replace quotes with code tags Make phpcs happy Add error titles for internal error codes Add error titles for error codes found in AMP specification Revert "Generate PHP class with error codes from AMP specification" Generate PHP class with error codes from AMP specification

…ext object cache

westonruter · 2020-03-22T01:07:25Z

src/HasActivation.php

+ *
+ * @package AmpProject\AmpWP
+ */
+interface HasActivation {


Note this interface is not currently used.

It's not being implemented, but it's used in \AmpProject\AmpWP\Services::class:

amp-wp/src/Services.php

Line 58 in 08506c2

if ( ! $service instanceof HasActivation ) {

pierlon

Just some minor things I'd like to bring attention to, otherwise it looks good!

pierlon · 2020-03-22T02:01:02Z

src/BackgroundTask/CronBasedBackgroundTask.php

+	 *
+	 * @var string[]
+	 */
+	const DEFAULT_INTERVAL_NAMES = [


This is currently unused.

Ah, good catch. When the cron-handling was still a bit more complex, this was used for validation. However, I ripped all of that out, and just assume someone using the class is using it correctly for now.

So this one can go.

pierlon · 2020-03-22T02:02:46Z

src/BackgroundTask/CronBasedBackgroundTask.php

+			return $actions;
+		}
+
+		$actions['deactivate'] = preg_replace( '#(?=</a>)#i', ' ' . self::get_warning_icon(), $actions['deactivate'] );


get_warning_icon() is being called statically although the function is not.

Fixed in 9ad9b10.

pierlon · 2020-03-22T02:22:25Z

src/Admin/SiteHealth.php

+			MonitorCssTransientCaching::DEFAULT_THRESHOLD
+		);
+
+		return "{$threshold} transients per day";


Should this be translated?

I don't think so. It's part of the Site Health info export.

pierlon · 2020-03-22T02:22:42Z

src/Admin/SiteHealth.php

+			MonitorCssTransientCaching::DEFAULT_SAMPLING_RANGE
+		);
+
+		return "{$sampling_range} days";


This as well, should it be translated?

I don't think so. It's part of the Site Health info export.

pierlon · 2020-03-22T02:24:57Z

src/HasActivation.php

+ *
+ * @package AmpProject\AmpWP
+ */
+interface HasActivation {


It's not being implemented, but it's used in \AmpProject\AmpWP\Services::class:

amp-wp/src/Services.php

Line 58 in 08506c2

if ( ! $service instanceof HasActivation ) {

src/BackgroundTask/CronBasedBackgroundTask.php

Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

pierlon

Great work done here @schlessera and @westonruter!

* Move functions to amp-helper-functions.php. * Add amp_bootstrap_plugin() function to contain former top-level calls

amp.php

Co-Authored-By: Weston Ruter <westonruter@google.com>

westonruter added this to the v1.5 milestone Jan 25, 2020

googlebot added the cla: yes Signed the Google CLA label Jan 25, 2020

westonruter mentioned this pull request Jan 25, 2020

Detect high cache miss rate for obtaining parsed CSS from variable stylesheets #2092

Closed

schlessera reviewed Mar 19, 2020

View reviewed changes

includes/sanitizers/class-amp-style-sanitizer.php Outdated Show resolved Hide resolved

Add filter to prevent storing parsed CSS in transients

ece53ed

schlessera force-pushed the add/disabling-parsed-css-transient-caching branch from 58c3ab7 to ece53ed Compare March 19, 2020 06:00

schlessera added 4 commits March 19, 2020 07:09

Rename use_cache to use_object_cache

217cc4b

Add infrastructure for managing background tasks

4d2bdba

Add logic to monitor CSS caching and disable if needed

2accca5

Make use of AMP Options Manager and skip transients as needed

a622435

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Mar 19, 2020

googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Mar 19, 2020

Add tests

6600b16

westonruter added 2 commits March 19, 2020 11:50

Use assertInternalType instead of assertIsInt for PHPUnit back-compat

164dac2

Fix test_amp_get_content_sanitizers for added allow_transient_caching

d28a5ef

westonruter commented Mar 19, 2020

View reviewed changes

tests/php/test-class-monitor-css-transient-caching.php Outdated Show resolved Hide resolved

westonruter commented Mar 19, 2020

View reviewed changes

schlessera added 3 commits March 20, 2020 06:34

Document filters

2b08f22

Refactor and simplify cron handling

d7e506f

Add srteamlined way of adding services, with activation and deactivation

ba094e1

schlessera requested a review from kienstra March 20, 2020 06:57

schlessera added 4 commits March 21, 2020 07:39

Use get_sites() instead of a direct SQL query

d229af9

Check against a capability, not a role

6542b7e

Add warnings to network plugins screen

eaf8259

Merge branch 'develop' into add/disabling-parsed-css-transient-caching

83d8a8d

westonruter added 5 commits March 21, 2020 14:18

Fix Test_Monitor_CSS_Transient_Caching

0362ea4

* Ensure that external object cache is disabled during tests. * Replace removed use of activate() method with call to schedule_event(). * Pass required attribute to deactivate() method.

Remove unused use statements

43affb0

Ensure warning icon uses color emoji and warning can be translated

27b3039

Split boolean state for transient caching to be tri-state indicating …

7dd5c78

…ext object cache

westonruter requested a review from schlessera March 21, 2020 23:47

westonruter commented Mar 22, 2020

View reviewed changes

pierlon suggested changes Mar 22, 2020

View reviewed changes

Fix calling non-static method non-statically

9ad9b10

pierlon reviewed Mar 22, 2020

View reviewed changes

src/BackgroundTask/CronBasedBackgroundTask.php Outdated Show resolved Hide resolved

Use non-static method invocations

a0995ff

Co-Authored-By: Pierre Gordon <16200219+pierlon@users.noreply.github.com>

pierlon approved these changes Mar 22, 2020

View reviewed changes

westonruter added 2 commits March 22, 2020 10:31

Add linting to ensure amp.php is compatibility with PHP 5.2

7d3163f

Make amp.php compatibility with PHP 5.2

686edc7

* Move functions to amp-helper-functions.php. * Add amp_bootstrap_plugin() function to contain former top-level calls

westonruter commented Mar 22, 2020

View reviewed changes

amp.php Outdated Show resolved Hide resolved

westonruter added the Changelogged label Mar 23, 2020

schlessera and others added 2 commits March 23, 2020 12:38

Revert using __DIR__ in bootstrap file

80d1bf3

Co-Authored-By: Weston Ruter <westonruter@google.com>

Remove unused constant

e53a3da

schlessera approved these changes Mar 23, 2020

View reviewed changes

westonruter merged commit e6e86dc into develop Mar 23, 2020

westonruter deleted the add/disabling-parsed-css-transient-caching branch March 23, 2020 06:03

westonruter mentioned this pull request Mar 23, 2020

Add cache pool abstraction to cache stylesheets #3300

Closed

westonruter mentioned this pull request Apr 6, 2020

Restrict doing plugin upgrade routine when not in admin #4538

Merged

3 tasks

westonruter mentioned this pull request Sep 19, 2020

Set maximum number of parsed CSS transients to store and proactively delete obsolete transients #5405

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to prevent storing parsed CSS in transients #4177

Add ability to prevent storing parsed CSS in transients #4177

westonruter commented Jan 25, 2020 •

edited

Loading

schlessera commented Mar 19, 2020 •

edited

Loading

googlebot commented Mar 19, 2020

schlessera commented Mar 19, 2020

googlebot commented Mar 19, 2020

schlessera commented Mar 19, 2020

schlessera commented Mar 19, 2020

westonruter Mar 19, 2020

westonruter Mar 19, 2020

schlessera Mar 20, 2020

westonruter Mar 20, 2020

schlessera Mar 20, 2020

schlessera Mar 20, 2020

schlessera Mar 20, 2020

schlessera Mar 20, 2020

felixarntz Mar 20, 2020

felixarntz Mar 20, 2020

schlessera commented Mar 20, 2020

schlessera commented Mar 21, 2020

westonruter commented Mar 21, 2020

westonruter Mar 22, 2020

pierlon Mar 22, 2020

pierlon left a comment

pierlon Mar 22, 2020

schlessera Mar 23, 2020

pierlon Mar 22, 2020

westonruter Mar 22, 2020 •

edited

Loading

pierlon Mar 22, 2020

westonruter Mar 22, 2020

pierlon Mar 22, 2020

westonruter Mar 22, 2020

pierlon Mar 22, 2020

pierlon Mar 22, 2020

pierlon left a comment

	$timestamp = wp_next_scheduled( $event_name );

	if ( $timestamp ) {
	return $timestamp;
	}

	if ( ! wp_schedule_event( time(), $interval_name, $event_name ) ) {

Add ability to prevent storing parsed CSS in transients #4177

Add ability to prevent storing parsed CSS in transients #4177

Conversation

westonruter commented Jan 25, 2020 • edited Loading

Summary

Site Health integration

Transient caching of stylesheets enabled

Transient caching of stylesheets disabled

Transient caching of stylesheets not applicable

Debug information

Network plugins screen warning

Checklist

schlessera commented Mar 19, 2020 • edited Loading

googlebot commented Mar 19, 2020

schlessera commented Mar 19, 2020

googlebot commented Mar 19, 2020

schlessera commented Mar 19, 2020

schlessera commented Mar 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schlessera commented Mar 20, 2020

schlessera commented Mar 21, 2020

westonruter commented Mar 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

westonruter Mar 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierlon left a comment

Choose a reason for hiding this comment

westonruter commented Jan 25, 2020 •

edited

Loading

schlessera commented Mar 19, 2020 •

edited

Loading

westonruter Mar 22, 2020 •

edited

Loading