Include standalone plugin slugs in generator tag #949

felixarntz · 2024-01-18T23:18:42Z

Summary

The Performance Lab generator tag has historically only included the active modules. Now that most Performance Lab features will be made available via plugins, the tag should receive support for that, including both the active modules and active plugins.

With this PR, the tag will now look like:

Performance Lab %1$s; modules: %2$s; plugins: %3$s

Alternatively (or even additionally), every standalone plugin could come with a generator tag of its own. While that is simple to implement, it's probably overhead worth avoiding, having to put that code into every plugin. Keeping just one generator also makes sense from the perspective of the Performance Lab plugin acting as the central "orchestrator" for the WordPress Performance Team's features.

Relevant technical choices

The perflab_get_standalone_plugins_constants() has been expanded to support lookup of the constants from either modules or plugins. As a side benefit, this centralizes the logic nicely to maintain the list only in one function, as perflab_get_standalone_plugins() now makes use of that rather than having its own list.
Modules are already only included if their relevant standalone plugin isn't active too. So that alone guarantees there are no duplicates.

Checklist

PR has either [Focus] or Infrastructure label.
PR has a [Type] label.
PR has a milestone or the no milestone label.

swissspidy · 2024-01-18T23:26:26Z

Alternatively (or even additionally), every standalone plugin could come with a generator tag of its own. While that is simple to implement, it's probably overhead worth avoiding, having to put that code into every plugin. Keeping just one generator also makes sense from the perspective of the Performance Lab plugin acting as the central "orchestrator" for the WordPress Performance Team's features.

What if one installs these plugins without installing Performance Lab? Especially now that PL doesn‘t offer much on its own, I‘d expect many people will just install the individual plugins they want, as they don‘t necessarily need PL at all. IMO this case needs to be considered.

felixarntz · 2024-01-18T23:33:18Z

@swissspidy It's definitely going to happen, agreed. But the Performance Lab plugin still has a very large install number, and it's hard to predict whether people will uninstall it or keep it around. Part of the reason it was decided to keep it was to provide a single entry point to the performance team's features, and any end users interested in that will likely still keep PL active. In any case, I agree with you and we certainly wouldn't be able to conclude from the PL's generator tag alone how much usage the standalone plugins have.

My thinking though is that it's not worth mandating a generator tag in every standalone plugin. I agree with you there would be value in having it, but how about leaving it up to the individual standalone plugins on whether they implement their own generator tag? That would function entirely decoupled from the PL one anyway.

This way, the PL generator tag provides a "sort of catch all" solution, which helps monitor how many sites use PL with the relevant standalone plugins, which I think is a valuable insight on PL adoption itself. Separate generator tags could be implemented in the standalone plugins, which would then basically provide similar information as the respective plugin's active install count number - with the benefit of seeing more granular updates, while having the trade-off of being limited to smaller datasets like HTTP Archive.

WDYT?

swissspidy · 2024-01-19T00:10:08Z

how about leaving it up to the individual standalone plugins on whether they implement their own generator tag?

That‘s what I would suggest, but that that we actually do it. It‘s just a few lines of code anyway, repetition and maintenance overhead is tiny.

westonruter · 2024-01-19T01:37:38Z

Yeah, I would agree that the PL plugin shouldn't orchestrate the generator tag for the standalone plugins. They can each generate their own so that they can be detected even when the PL plugin is not installed.

mukeshpanchal27 · 2024-01-19T03:59:36Z

I also agree with @westonruter and @swissspidy. At some point, if we release a major chunk of modules into a plugin, the majority of folks may not install the Performance Lab plugin. In that case, having individual generator tags for each plugin would make it easier to identify on HTTP Archive.

felixarntz · 2024-01-19T17:13:32Z

I don't think this has to be an either or decision. Why not do both?

Benefits of Performance Lab generator tag covering plugins:
- Allows detecting usage of plugins in combination with Performance Lab, which is a relevant adoption metric by itself
- Uniform structure so no need to adjust queries/tooling regularly for new generator tags
- No chance of "missing" it for a plugin
Benefits of individual plugins' generator tags:
- Reliable coverage of the plugins regardless of whether PL is active too

So I think doing both makes most sense.

westonruter · 2024-01-19T18:48:17Z

Humm. I'm still not seeing the value of both. If a standalone plugin has a generator tag anyway, then having the standalone plugin mentioned in the PL generator tag is just redundancy, right? The standalone plugin generator tag would have to already be tracked regardless. If wanting to track adoption of PL plugin plus the standalone plugin, wouldn't this just involve looking for the two generator [meta] tags? For a uniform structure, wouldn't we just agree on a convention for the standalone plugins to construct their generator tags?

felixarntz · 2024-01-19T20:05:38Z

@westonruter

If a standalone plugin has a generator tag anyway, then having the standalone plugin mentioned in the PL generator tag is just redundancy, right? The standalone plugin generator tag would have to already be tracked regardless.

Yes and no. Those are two different things to track. A query that tracks Performance Lab adoption in combination with standalone plugins could work by itself, while a query to track the standalone plugins adoption would work differently.

If wanting to track adoption of PL plugin plus the standalone plugin, wouldn't this just involve looking for the two generator [meta] tags?

That would be possible, but more complicated. Plus it's not addressing the point on not mandating the use of a generator tag in every standalone plugin.

For a uniform structure, wouldn't we just agree on a convention for the standalone plugins to construct their generator tags?

The only way I can see that work is if we used a generator value like content="Performance Lab standalone plugin: {pluginslug}" for every standalone plugin, which feels a bit strange. But to make it uniform there would need to be something reusable like this that a query could look for, in order for us to not have to implement/modify the query for every new plugin.

I don't see any downsides in having both. Having both approaches allows tracking different things and is more robust than only having one or the other.

mukeshpanchal27 · 2024-01-22T11:33:48Z

For security reasons, many users remove the generator tags. In this case, users would need to add different filter codes to remove these tags if we consider adding the generator tag in multiple places.

For standalone plugins, we don't have any filter that can remove these generator tags. Last week, a user raised a PR for hiding these features here. Do we need to add filter that remove the generator tag? If yes then can I open a new issue for it?

swissspidy · 2024-01-22T12:25:07Z

Do we need to add filter that remove the generator tag? If yes then can I open a new issue for it?

Seems like a separate discussion. But why an extra filter if you can already do remove_action( 'wp_head', 'perflab_render_generator' );?

felixarntz · 2024-01-22T18:07:05Z

+1, I don't think there needs to be an extra filter. It needs to be possible (which it is by unhooking the action), while at the same time not something that we should encourage.

If you agree with the approach of having generator tags for the plugins in both PL and the standalone plugins, it would be great if you could proceed with the actual code review on this one. :)

load.php

westonruter · 2024-01-22T20:48:08Z

load.php

 */
-function perflab_get_standalone_plugins_constants() {
+function perflab_get_standalone_plugins_constants( $type = 'modules' ) {


It seems a bit strange that this function is named to get the "standalone plugins" but the default $type is not to get the standalone plugins at all, but rather the modules. Why not create a separate function like perflab_get_module_constants() instead of adding a $type arg?

Happy to do that if y'all agree, but that's a BC break, so I went the "safe" route first. It's probably fair to assume nobody uses these functions outside of PL, but if they did this would break.

On the other hand, part of the point is to have these two lists in one place, as they need to be adjusted when changing which standalone plugins are available, depending on which state they are in. So from that perspective, I'd prefer to keep this together, maybe under a more suitable name.

Let me know what you prefer.

Yeah, there's no other plugins using this function. So I think it's fine to break back-compat.

Otherwise, two new functions could be created to include version:

perflab_get_standalone_plugin_version_constants()

perflab_get_module_version_constants()

The existing function can then be marked as deprecated. But this is probably overkill.

SG regarding backward compatibility.

However those two names wouldn't be clear either. To clarify:

Everything listed here is only relevant for standalone plugins.

The first list is for modules that have an equivalent standalone plugin available.

The second list is for standalone plugins overall, regardless of whether they come from a module or are exclusively available as a standalone plugin.

Maybe just rename this function to perflab_get_standalone_plugin_version_constants(), and the argument could become $source instead of $type? Maybe then the intent would be more clear ("source" can be "modules" or "plugins").

I've updated this in 8181c68.

For now I just deprecated the other function. Call it me being overly cautious because of WP core. If you prefer to drop it, we can, but I don't really see a value in immediately removing it.

Co-authored-by: Weston Ruter <westonruter@google.com>

…re explanation via code comments.

mukeshpanchal27

Thanks, @felixarntz, for the PR. I've left one piece of feedback for your consideration. Other than that, it looks good to me.

mukeshpanchal27 · 2024-01-23T04:07:55Z

load.php

 *
 * @return array Map of module path to version constant used.
 */
 function perflab_get_standalone_plugins_constants() {
+	_deprecated_function( __FUNCTION__, 'Performance Lab n.e.x.t', "perflab_get_standalone_plugin_version_constants( 'modules' )" );


The reference to n.e.x.t will not change using the command npm run since -- -r 2.9.0. So, make sure to update it manually or we can update it now before the merge. What do you think?

felixarntz · 2024-01-24T23:48:25Z

Saw this and wanted to merge it, but now I'm out of here bye 😆👶

Include standalone plugin slugs in generator tag.

c52d759

felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall performance plugin infrastructure [Plugin] Performance Lab Issue relates to work in the Performance Lab Plugin only labels Jan 18, 2024

felixarntz added this to the PL Plugin 2.9.0 milestone Jan 18, 2024

felixarntz requested review from westonruter, joemcgill and swissspidy January 18, 2024 23:18

felixarntz requested a review from mukeshpanchal27 as a code owner January 18, 2024 23:18

Update additional test.

ccf3220

westonruter reviewed Jan 22, 2024

View reviewed changes

felixarntz and others added 2 commits January 22, 2024 14:43

Use more precise type doc.

ead22c8

Co-authored-by: Weston Ruter <westonruter@google.com>

Introduce new function instead of trying to reuse old one, and add mo…

8181c68

…re explanation via code comments.

felixarntz requested a review from westonruter January 23, 2024 00:26

westonruter approved these changes Jan 23, 2024

View reviewed changes

mukeshpanchal27 approved these changes Jan 23, 2024

View reviewed changes

swissspidy approved these changes Jan 23, 2024

View reviewed changes

Replace n.e.x.t with 2.9.0.

901a62e

felixarntz merged commit 0e7f285 into trunk Jan 24, 2024

felixarntz deleted the add/standalone-plugin-generator-tag-support branch January 24, 2024 23:48

westonruter mentioned this pull request Feb 15, 2024

Prepare 2.9.0 release #985

Closed

3 tasks

Include standalone plugin slugs in generator tag #949

Include standalone plugin slugs in generator tag #949

Uh oh!

Conversation

felixarntz commented Jan 18, 2024

Summary

Relevant technical choices

Checklist

Uh oh!

swissspidy commented Jan 18, 2024

Uh oh!

felixarntz commented Jan 18, 2024

Uh oh!

swissspidy commented Jan 19, 2024

Uh oh!

westonruter commented Jan 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mukeshpanchal27 commented Jan 19, 2024

Uh oh!

felixarntz commented Jan 19, 2024

Uh oh!

westonruter commented Jan 19, 2024

Uh oh!

felixarntz commented Jan 19, 2024

Uh oh!

mukeshpanchal27 commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

swissspidy commented Jan 22, 2024

Uh oh!

felixarntz commented Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

westonruter Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

felixarntz Jan 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

westonruter Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

felixarntz Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

felixarntz Jan 22, 2024

Choose a reason for hiding this comment

Uh oh!

felixarntz Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 left a comment

Choose a reason for hiding this comment

Uh oh!

mukeshpanchal27 Jan 23, 2024

Choose a reason for hiding this comment

Uh oh!

felixarntz commented Jan 24, 2024

Uh oh!

Uh oh!

westonruter commented Jan 19, 2024 •

edited

Loading

mukeshpanchal27 commented Jan 22, 2024 •

edited

Loading

felixarntz commented Jan 22, 2024 •

edited

Loading

felixarntz Jan 22, 2024 •

edited

Loading