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

Add optimization_detective_disabled query var to disable behavior #1193

Merged
merged 1 commit into from
May 6, 2024

Conversation

westonruter
Copy link
Member

Fixes #1069

This will allow us to add ?optimization_detective_disabled to any URL running with Optimization Detective in order to generate the page without the optimizations. This will be useful for us to observe in the wild the performance gains by comparing a page with the non-optimized version.

@westonruter westonruter added [Type] Enhancement A suggestion for improvement of an existing feature [Plugin] Optimization Detective Issues for the Optimization Detective plugin labels May 1, 2024
@westonruter westonruter requested a review from felixarntz as a code owner May 1, 2024 00:04
Copy link

github-actions bot commented May 1, 2024

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: westonruter <[email protected]>
Co-authored-by: mukeshpanchal27 <[email protected]>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@westonruter
Copy link
Member Author

@joemcgill What do you think of this being a common pattern we use across other Performance Lab plugins? Being able to disable the functionality of a plugin in the wild could be a great way to compare how effective it is at improving performance. Naturally this wouldn't make sense for core, but for a feature plugin it could be valuable.

@westonruter westonruter merged commit 70a4fdc into trunk May 6, 2024
29 checks passed
@westonruter westonruter deleted the add/optimization-detective-disable-query-param branch May 6, 2024 16:35
@joemcgill
Copy link
Member

What do you think of this being a common pattern we use across other Performance Lab plugins?

Are you proposing that we would be able to disable all of our feature plugins by passing a query param for each feature, or are you thinking of a single param that would disable all of the features? I guess as a way to collect some A/B data, this could be useful, though we wouldn't really be able to use those params in collected user metrics, like CrUX.

Also, something to keep in mind is that even though most full-page caches are likely already handling GET requests with query params separately, there may be some where the addition of the query param may still result in a previous cached result being served.

@westonruter
Copy link
Member Author

@joemcgill I was thinking each feature would have their own query parameter, so we can granularly turn off optimizations to see their relative impact. This would only help in a lab context, as you note, as we wouldn't have field metrics for any A/B comparison.

And yes, full page caches may interfere with this, so when attempting to disable a feature we'll need to make sure that it is actually disabled when we load the page.

@joemcgill
Copy link
Member

Makes sense to me. I think this is one of those cases where having a single implementation that automatically gets incorporated into all our plugins would be useful. We don't currently have anything like that set up for this repo but maybe it's time to set that up so we can handle use cases like this one and the need to support embedded plugins, like #1081. What do you think?

@westonruter
Copy link
Member Author

@joemcgill Yeah, I was thinking about something like that too. However, doing something as low level as #1081 may be too low. A plugin may have multiple aspects to the optimizations it performs (e.g. fetchpriority and lazy loading) so it would be useful to be able to target each feature discretely, rather than be able to turn off the entire plugin altogether. So this makes me think more like "feature flag" query parameters are the most useful here. Given that each plugin will have their own set of features, perhaps it doesn't make sense to have a unified API for managing them? Or perhaps it could just be a common query var that we use, for example:

?perflab_feature_disabled[]=foo&perflab_feature_disabled[]=bar&perflab_feature_disabled[]=baz

Then $_GET['perflab_feature_disabled'] would be ['foo', 'bar', 'baz'].

@joemcgill
Copy link
Member

I like the idea of a common pattern for feature flags. Let's try to be observant of when we see this type of pattern popping up in multiple plugins and open a new issue for consolidating logic/architecture when it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Plugin] Optimization Detective Issues for the Optimization Detective plugin [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide mechanism to easily turn off optimizations applied by Optimization Detective for lab testing
4 participants