FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Implement logic to measure specific hook execution time with Server-Timing controlled by a WP Admin screen by felixarntz · Pull Request #784 · WordPress/performance · GitHub

/ performance Public

Implement logic to measure specific hook execution time with Server-Timing controlled by a WP Admin screen - #784

Merged
felixarntz merged 22 commits into
trunkfrom
add/server-timing-hook-ui
Aug 9, 2023
ConversationCommits (22)ChecksFiles changed
Merged

Implement logic to measure specific hook execution time with Server-Timing controlled by a WP Admin screen#784
felixarntz merged 22 commits into
trunkfrom
add/server-timing-hook-ui

Conversation

felixarntz commented Jul 18, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

Summary

This PR implements a WP Admin screen Tools > Server-Timing which allows providing action and filter names. Any hook names provided will be measured and exposed in the Server-Timing header in the frontend.

While the Server-Timing API from #553 has been a powerful foundation, it can sometimes be cumbersome to measure specific hooks performance, since it always requires writing custom code. Gists like this one can be used for that, but even with such thing it may be easier to just add the action or filter to a UI instead of having to modify a plugin's code. This PR makes it really easy and gives site owners control about specific hooks to measure.

The new admin screen with example input

Relevant technical choices

  • Adds a new WordPress option/setting to store Server-Timing configuration.
  • Adds a new WP Admin screen with textareas to provide action and filter names.
  • Uses the data from the new option/setting to measure the duration of those hooks, using similar logic as previously implemented in the aforementioned Gist.

Testing

  1. Go to Tools > Server-Timing.
  2. Add some hook names (actions and/or filters), one per line. For example those from the screenshot above.
  3. Save and verify the hook names are still shown in the textareas, in a sanitized way.
    • For example, try to add spaces or (other) invalid characters and check that they're stripped after submitting.
  4. Load the frontend.
  5. Type console.table( performance.getEntries()[0].serverTiming ) in the browser's dev tools console.
  6. Verify that you see measurements for the actions/filters you provided, via wp-action-{$action} / wp-filter-{$filter} metrics.
    • Note that metrics will only show up if the respective action/filter was actually run during the request.
    • If you provide hook names that are only run after the template is being parsed (e.g. wp_enqueue_scripts action), they won't show up by default. You'll need to activate output buffering (e.g. via this Gist plugin) so that those can be exposed as well.

Checklist

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

felixarntz added [Type] Feature A new feature within an existing module Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release labels Jul 18, 2023
spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread admin/server-timing.php
add_action( "load-{$hook_suffix}", 'perflab_load_server_timing_page' );
}

return $hook_suffix;

spacedmonkey Jul 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What are you returning this for? This is hooked into an action right?

felixarntz Jul 21, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Right, this would only be useful for test coverage, which I'm planning to add.

spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread server-timing/load.php Outdated
Comment on lines +38 to +55
function perflab_sanitize_server_timing_setting( $value ) {
if ( ! is_array( $value ) ) {
return array();
}

// Ensure that every element is an indexed array of hook names.
return array_filter(
array_map(
static function( $hooks ) {
if ( ! is_array( $hooks ) ) {
$hooks = explode( "\n", $hooks );
}
return array_filter( array_map( 'sanitize_key', $hooks ) );
},
$value
)
);
}

spacedmonkey Jul 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This could do with a unit test IMO.

felixarntz Jul 21, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yeah, I haven't added unit tests yet, but definitely planning to do so.

spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread server-timing/load.php Outdated
spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread server-timing/defaults.php Outdated
spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread server-timing/defaults.php Outdated
spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread server-timing/defaults.php Outdated
spacedmonkey reviewed Jul 19, 2023
View reviewed changes
Comment thread server-timing/defaults.php Outdated

swissspidy commented Jul 24, 2023

Copy link
Copy Markdown
Member

Just from looking at the screenshot alone it is not clear what exactly would be measured. Is it the time since the last hook (i.e. do_action(wp_loaded) *start* ... *stop* do_action(init), or the duration of all the functions hooked into it (i.e. *start* ... do_action( xyz ); *stop*). Looking at https://gist.github.com/felixarntz/63c05392dbf7d51cc7f8f4a424b1ff39, it seems to be the latter, but would be nice to clarify that in the UI with a clearer description.

mukeshpanchal27 reviewed Jul 27, 2023
View reviewed changes

mukeshpanchal27 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @felixarntz for the PR, If anyone add duplicate value in Actions or Filters then it shows error.

For ex.

init
wp_loaded
init

felixarntz commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

Thanks @swissspidy @mukeshpanchal27 for the feedback.

  • In 6b5d12a, I've pushed a clarification for how the hooks are measured.
  • In 61f5ccc, I've pushed a fix to ensure no duplicate hooks can be provided.

I'll work on some test coverage now, mostly relevant for the WP Admin UI.

felixarntz commented Aug 3, 2023

Copy link
Copy Markdown
Member Author

In 585911d and 6db1e85 I've added test coverage, so this PR is now ready for a full review.

felixarntz marked this pull request as ready for review August 3, 2023 18:17
felixarntz requested a review from JustinyAhin as a code owner August 3, 2023 18:17
felixarntz added this to the PL Plugin 2.6.0 milestone Aug 3, 2023
westonruter reviewed Aug 7, 2023
View reviewed changes
Comment thread server-timing/load.php Outdated
* Any duplicates across a group of hooks are removed.
*/
return array_filter(
array_map(

westonruter Aug 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Should this be checking whether the keys are those which are allowed, i.e. benchmarking_actions and benchmarking_filters?

felixarntz Aug 7, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good idea, updated in 7f99021

Comment thread server-timing/load.php Outdated
Comment on lines +177 to +185
},
$hooks
)
)
)
);
},
$value
)

westonruter Aug 7, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

There is a lot of nesting happening here. Not a problem, but just makes it a bit hard on the eyes. Not a blocker.

felixarntz Aug 7, 2023

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Reduced the nesting a bit in 7f99021

felixarntz requested a review from westonruter August 7, 2023 19:59

westonruter commented Aug 7, 2023

Copy link
Copy Markdown
Member

For some reason when I add a filter to get Server-Timing for, e.g. the_content, I'm getting a PHP notice:

Notice: Function Perflab_Server_Timing::register_metric was called incorrectly. The method must be called before or during the perflab_server_timing_send_header action. Please see Debugging in WordPress for more information. in /var/www/html/wp-includes/functions.php on line 5865

I don't get it for when I add actions, however. 😕

felixarntz commented Aug 7, 2023

Copy link
Copy Markdown
Member Author

@westonruter That's because the the_content hook runs after the header is output. So hooks like that can only be used if output buffering is enabled. Maybe we open a separate PR to add a checkbox to allow controlling that as well? That would make that bit easier as well, without the need for another one-liner plugin.

westonruter commented Aug 7, 2023

Copy link
Copy Markdown
Member

@westonruter That's because the the_content hook runs after the header is output. So hooks like that can only be used if output buffering is enabled. Maybe we open a separate PR to add a checkbox to allow controlling that as well? That would make that bit easier as well, without the need for another one-liner plugin.

Oh, of course. Same as mentioned in #784 (comment).

What do you think about the presence of any Server-Timing hooks to automatically cause output buffering to be enabled automatically? That would prevent others from getting tripped up the same way I did.

felixarntz commented Aug 7, 2023 •
edited
Loading

Copy link
Copy Markdown
Member Author

@westonruter I think it would be better to have an explicit setting for it. To clarify the limitation when output buffering is not enabled, I have added a message for it in fd41db1 (see also the updated screenshot in the PR description).

Once we add a setting control for it, we can consider toggling that with JS based on the checkbox.

swissspidy reviewed Aug 8, 2023
View reviewed changes
Comment thread admin/server-timing.php Outdated
static function() {
?>
<p>
<?php esc_html_e( 'In this section, you can provide hook names to include measurements for them in the Server-Timing header.', 'performance-lab' ); ?>

swissspidy Aug 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since the Server-Timing header is not something that should be translated, I'd recommend using sprintf with a placeholder here (+ a translator comment of course).

swissspidy reviewed Aug 8, 2023
View reviewed changes
Comment thread admin/server-timing.php Outdated
<br>
<?php
echo wp_kses(
__( 'Since the Server-Timing header is sent before the template is loaded, only hooks before the <code>template_include</code> filter can be measured.', 'performance-lab' ),

swissspidy Aug 8, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I'd use sprintf with a placeholder for template_include so that it does not get translated

swissspidy approved these changes Aug 8, 2023
View reviewed changes

swissspidy left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, just two i18n notes

mukeshpanchal27 reviewed Aug 8, 2023
View reviewed changes

mukeshpanchal27 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @felixarntz for the updates.

I still reproduce #784 (review) error. Could you please take a look. Thanks.

felixarntz commented Aug 8, 2023

Copy link
Copy Markdown
Member Author

@mukeshpanchal27

I still reproduce #784 (review) error. Could you please take a look. Thanks.

Maybe it was because you had those settings saved from before? The actual behavior from having duplicate hooks was not changed, but I added sanitization to strip duplicate values. So can you please try to go to the Tools screen again and re-save? It should result in the duplicates being stripped.

westonruter commented Aug 8, 2023

Copy link
Copy Markdown
Member

Once we add a setting control for it, we can consider toggling that with JS based on the checkbox.

I'd like to propose Output Buffering as a new module. This module would be to explore implementation of Core-43258/Core-58285 while also providing an easy way to implement the toggle. It will give us an area to explore use of output buffering to make Performance enhancements as well, such as applying LCP enhancements to arbitrary elements not rendered using the normal WordPress mechanisms (e.g. hard-coded images and page builders). cc @joemcgill @kt-12

felixarntz mentioned this pull request Aug 8, 2023
Merged
3 tasks
mukeshpanchal27 approved these changes Aug 9, 2023
View reviewed changes

mukeshpanchal27 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Thanks @felixarntz, I test in fresh installation and it working for me.

felixarntz merged commit bfb4ae9 into trunk Aug 9, 2023
felixarntz deleted the add/server-timing-hook-ui branch August 9, 2023 18:04
mukeshpanchal27 mentioned this pull request Aug 10, 2023
Closed

westonruter commented Aug 10, 2023

Copy link
Copy Markdown
Member

Maybe we open a separate PR to add a checkbox to allow controlling that as well? That would make that bit easier as well, without the need for another one-liner plugin.

I'll work on this.

westonruter mentioned this pull request Aug 12, 2023
Merged
3 tasks

westonruter commented Aug 12, 2023

Copy link
Copy Markdown
Member

Maybe we open a separate PR to add a checkbox to allow controlling that as well? That would make that bit easier as well, without the need for another one-liner plugin.

I'll work on this.

See #801

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

spacedmonkey spacedmonkey left review comments
westonruter westonruter approved these changes
mukeshpanchal27 mukeshpanchal27 approved these changes
JustinyAhin Awaiting requested review from JustinyAhin
swissspidy Awaiting requested review from swissspidy

Assignees

No one assigned

Labels

Infrastructure Issues for the overall performance plugin infrastructure no milestone PRs that do not have a defined milestone for release [Type] Feature A new feature within an existing module

Projects

None yet

Milestone

PL Plugin 2.6.0

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL