Fix Server-Timing compatibility with other plugins that do output buffering #1260
Conversation
|
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 If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| if ( $this->use_output_buffer() ) { | ||
| add_action( 'template_redirect', array( $this, 'start_output_buffer' ), PHP_INT_MIN ); | ||
| } else { | ||
| add_filter( 'template_include', array( $this, 'on_template_include' ), PHP_INT_MAX ); | ||
| } |
There was a problem hiding this comment.
I'm curious why we need to change the hook that this is attached to depending on if OB is being used? I also wonder why we just don't move the hook regardless of the OB status if there is a benefit, so that the timing will be consistent.
The other consideration here is that the wp-before-template metrics are calculated from the tempate_include hook as well (reference) and I think we want these to happen on the same hook in order to accurately report the wp-total value.
There was a problem hiding this comment.
I'm curious why we need to change the hook that this is attached to depending on if OB is being used? I also wonder why we just don't move the hook regardless of the OB status if there is a benefit, so that the timing will be consistent.
When output buffering is not enabled, the Server-Timing header should be sent as late as possible to give a chance for plugins to register their metrics. In this case, template_include filter with max priority is appropriate.
On the other hand, when output buffer is enabled then we still need to send the Server-Timing header at the very end, but the way we do so is different: we do so by starting the output buffer earlier so that it wraps the entire page resulting in its callback running at the very end.
If we always sent the Server-Timing header at template_redirect (even when not doing output buffering) then this would potentially exclude plugins from being able to register their metrics since they may occur between template_redirect and template_include.
The other consideration here is that the
wp-before-templatemetrics are calculated from thetempate_includehook as well (reference) and I think we want these to happen on the same hook in order to accurately report thewp-totalvalue.
I'm not sure the change impacts this wp-before-template metrics calculation since the change here is only changing when output buffering starts, not when calculation happens.
There was a problem hiding this comment.
Ah, right, the metric still gets recorded at the same time, it's just when we start the buffer that changes. Makes sense.
The interesting side effect here is that this allows us to add Server Timing headers to things like robots.txt, feeds, etc. which weren't previously possible.
| if ( $this->use_output_buffer() ) { | ||
| add_action( 'template_redirect', array( $this, 'start_output_buffer' ), PHP_INT_MIN ); | ||
| } else { | ||
| add_filter( 'template_include', array( $this, 'on_template_include' ), PHP_INT_MAX ); | ||
| } |
There was a problem hiding this comment.
Ah, right, the metric still gets recorded at the same time, it's just when we start the buffer that changes. Makes sense.
The interesting side effect here is that this allows us to add Server Timing headers to things like robots.txt, feeds, etc. which weren't previously possible.
Summary
When testing with WordPress core trunk (6.6-alpha), I noticed a
_doing_it_wrong()notice:In looking at Query Monitor, it identifies Optimization Detective in the stack trace:
With
git bisectI discovered the issue started with r57920 for Core-42441. Nevertheless, in looking at that commit I see nothing which would seem to be related to this. There seems to be some race condition happening due to the fact that the Server-Timing component is starting output buffering atPHP_INT_MAXwhile Optimization Detective is doing the same. In order for Optimization Detective to register its Server-Timing metric and for Server-Timing to be able to send the header successfully, the Server-Timing component (counter intuitively) needs to start its output buffer first so that it wraps the output buffer of Optimization Detective and and other plugins which may be starting the output buffer atPHP_INT_MAX. The callback for the first output buffer is executed last.So this PR changes the priority for the Server-Timing output buffer to start now at the minimum
template_redirectaction priority so that its output buffer callback is invoked last, allowing any other plugins that do output buffering and add server-timing metrics (e.g. Optimization Detective) to be able to do so in their own output buffer callbacks which run before the the Server-Timing callback is invoked.Note: To test this you need to (1) enable output buffering in the Server-Timing screen, and (2) access the site while logged-out of the admin or add a
add_filter( 'od_can_optimize_response', '__return_true' )filter since by default Optimization Detective does not run for admins.