Skip to content

Raise default threshold for disabling CSS caching#4513

Merged
westonruter merged 3 commits intodevelopfrom
raise-css-monitoring-threshold
Apr 2, 2020
Merged

Raise default threshold for disabling CSS caching#4513
westonruter merged 3 commits intodevelopfrom
raise-css-monitoring-threshold

Conversation

@schlessera
Copy link
Copy Markdown
Collaborator

@schlessera schlessera commented Apr 2, 2020 •
edited
Loading

Summary

Raises the default threshold for disabling the CSS caching system from 50 to 5000.

See https://wordpress.org/support/topic/1-5-1-slow-causes-high-request-time/ for more contextual information about the problem this solves.

Expected Behaviour

The CSS transient caching monitoring should only disable the caching on high-traffic sites with highly volatile CSS.

Steps to reproduce

Not directly reproducible yet, communicating via https://wordpress.org/support/topic/1-5-1-slow-causes-high-request-time/ to find out more.

Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • CSS transient caching is only disabled when the average count reaches 5000 per day.
  • For updates from 1.5.0/1.5.1 to 1.5.2, an already disabled CSS transient cache should be reenabled.

Implementation brief

  • Raise the constant with the daily threshold from 50 to 5000.
  • Run logic on amp_plugin_update to reset the disable option.

QA testing instructions

For the raised limit:

  • Go to Tools -> Site Health and click on the Info page at the top.
  • Scroll down to find the AMP section.
  • Verify: Threshold for monitoring stylesheet caching shows 5000 per day

For the reset on update:

  • Use version 1.5.0 of the AMP plugin.
  • Run the following command via wp shell: AMP_Options_Manager::update_option( 'amp_css_transient_monitor_disable_caching', true );.
  • Go to Tools -> Site Health and click on the Info page at the top.
  • Scroll down to find the AMP section.
  • Verify: Transient caching for stylesheets disabled shows true.
  • Update the plugin from version 1.5.0 to 1.5.2 or higher.
  • Go to Tools -> Site Health and click on the Info page at the top.
  • Scroll down to find the AMP section.
  • Verify: Transient caching for stylesheets disabled shows false.

Demo

Changelog entry

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).

@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 2, 2020
@schlessera
Copy link
Copy Markdown
Collaborator Author

schlessera commented Apr 2, 2020

@westonruter The option to reset is within the AMP_Options_Manager. What is the best way to reset that with the next release. An update_option call hooked into the plugin_update action...?

@schlessera schlessera added this to the v1.5.2 milestone Apr 2, 2020
@schlessera schlessera added Bug Something isn't working CSS labels Apr 2, 2020
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 2, 2020

@schlessera Yes, in the amp_plugin_update action. Alternatively, this could be plugged into the get_options method, like so:

--- a/includes/options/class-amp-options-manager.php
+++ b/includes/options/class-amp-options-manager.php
@@ -87,6 +87,11 @@ class AMP_Options_Manager {
 			$defaults['theme_support'] = amp_is_canonical() ? AMP_Theme_Support::STANDARD_MODE_SLUG : AMP_Theme_Support::TRANSITIONAL_MODE_SLUG;
 		}
 
+		// Reset premature disabling of transient caching.
+		if ( isset( $options['version'] ) && version_compare( $options['version'], '1.5.2', '<' ) ) {
+			unset( $options[ \AmpProject\AmpWP\Option::DISABLE_CSS_TRANSIENT_CACHING ] );
+		}
+
 		$options = array_merge( $defaults, $options );
 
 		// Migrate theme support slugs.

@schlessera schlessera self-assigned this Apr 2, 2020
@schlessera schlessera requested a review from westonruter April 2, 2020 17:14
westonruter
westonruter reviewed Apr 2, 2020
View reviewed changes
@schlessera @westonruter
Fix inverted logic in version check
8115e0d 
Co-Authored-By: Weston Ruter <westonruter@google.com>
westonruter
westonruter approved these changes Apr 2, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

This seems to work. I opened wp shell and did:

AMP_Options_Manager::update_option( 'amp_css_transient_monitor_disable_caching', true );

image

Then in wp shell I forced the upgrade routing to happen with the next page load:

AMP_Options_Manager::update_option( 'version', '1.5.1' );

And when I reloaded Site Health I then saw:

image

@westonruter westonruter merged commit 72929fb into develop Apr 2, 2020
@westonruter westonruter deleted the raise-css-monitoring-threshold branch April 2, 2020 19:19
westonruter added a commit that referenced this pull request Apr 2, 2020
@schlessera @westonruter
Raise default threshold for disabling CSS caching (#4513)
5935fde 
* Raise default threshold from 50 to 5000

* Reset option for disabling CSS cache on update

* Fix inverted logic in version check

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

Co-authored-by: Weston Ruter <westonruter@google.com>
@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 2, 2020

Build for testing: amp.zip (v1.5.2-alpha-20200402T192113Z-5935fdef5)

@amedina
Copy link
Copy Markdown
Member

amedina commented Apr 3, 2020

It would be good to create an Issue for each PR. This one seems to be missing one.

@amedina
Copy link
Copy Markdown
Member

amedina commented Apr 3, 2020 •
edited
Loading

As discussed with @westonruter:

  • If there is a support topic reported with a bug, linking to the support topic in the PR instead of creating an issue is fine
  • For bug fixes and improvements that come about while hacking around, it is ok to not file a separate issue, but in this case, it is important to describe clearly in the PR the issue being solved.
  • We should not submit PRs without associated issues that are not easy to describe standalone

@schlessera
Copy link
Copy Markdown
Collaborator Author

schlessera commented Apr 3, 2020

@amedina Noted!

As this one is already merged, I'll add the issue template to the PR information above with brief and QA instructions.

@westonruter
Copy link
Copy Markdown
Member

westonruter commented Apr 3, 2020

Also: if there is a PR and a corresponding issue, only the issue needs to be added to the board. If a PR has no linked issue, then it can be added to the board directly.

@amedina
Copy link
Copy Markdown
Member

amedina commented Apr 3, 2020

All the PRs in the Ready for QA are merged. Is that the pattern?

@amedina
Copy link
Copy Markdown
Member

amedina commented Apr 3, 2020

QA: The functionality is working as expected.

westonruter added a commit that referenced this pull request Apr 3, 2020
@westonruter
Merge tag '1.5.2'
00f36e8 
* tag '1.5.2':
  Bump 1.5.2
  Bump version to 1.5.1-RC1
  Cache response status and headers when fetching external stylesheets (#4509)
  Fix securing multi-line mustache templates (#4521)
  Add CSS monitoring time series to Site Health debugging info (#4519)
  Update hostname used for WordPress TV embeds to fix external HTTP requests (#4524)
  Fix processing of element child sanitization loop when invalid elements are replaced with children (#4512)
  Account for more YouTube URL formats (#4508)
  Update selected featured image ID on select (#4453)
  Raise default threshold for disabling CSS caching (#4513)
  Cast i-amphtml-intrinsic-sizer dimensions to integers (#4506)
  Only move meta tags to the head when required and add processing for meta[http-equiv] (#4505)
  Fix failing tests (#4507)
  Bump 1.5.2-alpha
@schlessera schlessera added the WS:Perf Work stream for Metrics, Performance and Optimizer label Aug 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@westonruter westonruter westonruter approved these changes

Assignees

@schlessera schlessera

Labels

Bug Something isn't working cla: yes Signed the Google CLA CSS WS:Perf Work stream for Metrics, Performance and Optimizer

Projects

None yet

Milestone

v1.5.2

Development

Successfully merging this pull request may close these issues.

4 participants

@schlessera @westonruter @amedina @googlebot