Skip to content

Fix Server-Timing compatibility with other plugins that do output buffering #1260

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.

Sign up for GitHub

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

Merged
westonruter merged 3 commits into from
May 30, 2024
Merged

Fix Server-Timing compatibility with other plugins that do output buffering #1260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b66b38e
Update template_include priority for Server-Timing to fix OD compat i…
westonruter May 30, 2024
d1bebcb
Opt to start Server-Timing output buffer early
westonruter May 30, 2024
934e7a7
Move logic into add_hooks() method and add tests
westonruter May 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 28 additions & 5 deletions plugins/performance-lab/includes/server-timing/class-perflab-server-timing.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,25 @@ public function use_output_buffer(): bool {
return (bool) apply_filters( 'perflab_server_timing_use_output_buffer', $enabled );
}

/**
* Adds hooks to send the Server-Timing header.
*
* When output buffering is enabled, buffer as early as possible so that any other plugins that also do output
* buffering will be able to register Server-Timing metrics. The first output buffer callback to be registered
* is the last one to be called, so by starting the Server-Timing output buffer as soon as possible we can be
* assured that other plugins' output buffer callbacks will run before the Server-Timing one that sends the
* Server-Timing header.
*
* @since n.e.x.t
*/
public function add_hooks(): void {
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 );
}
Comment on lines +240 to +244

@joemcgill joemcgill May 30, 2024

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.

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.

@westonruter westonruter May 30, 2024

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.

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

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.

@joemcgill joemcgill May 30, 2024

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.

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.

}

/**
* Hook callback for the 'template_include' filter.
*
Expand All @@ -238,18 +257,22 @@ public function use_output_buffer(): bool {
* @return mixed Unmodified value of $passthrough.
*/
public function on_template_include( $passthrough = null ) {
if ( ! $this->use_output_buffer() ) {
$this->send_header();
return $passthrough;
}
$this->send_header();
return $passthrough;
}

/**
* Starts output buffering to send the Server-Timing header right before returning the buffer.
*
* @since n.e.x.t
*/
public function start_output_buffer(): void {
ob_start(
function ( $output ) {
$this->send_header();
return $output;
}
);
return $passthrough;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion plugins/performance-lab/includes/server-timing/load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ function perflab_server_timing(): Perflab_Server_Timing {
return $server_timing;
}

add_filter( 'template_include', array( $server_timing, 'on_template_include' ), PHP_INT_MAX );
$server_timing->add_hooks();
}

return $server_timing;
Expand Down
17 changes: 17 additions & 0 deletions tests/plugins/performance-lab/includes/server-timing/test-load.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,12 +16,29 @@ public function test_perflab_server_timing(): void {
$this->assertTrue( has_filter( 'template_include' ) );

$server_timing = perflab_server_timing();
$this->assertFalse( $server_timing->use_output_buffer() );
$this->assertSame( PHP_INT_MAX, has_filter( 'template_include', array( $server_timing, 'on_template_include' ) ), 'template_include filter not added' );
$this->assertFalse( has_action( 'template_redirect', array( $server_timing, 'start_output_buffer' ) ), 'template_redirect action added' );

$server_timing2 = perflab_server_timing();
$this->assertSame( $server_timing, $server_timing2, 'Different instance returned' );
}

/**
* @covers Perflab_Server_Timing::add_hooks
*/
public function test_perflab_server_timing_with_output_buffering(): void {
remove_all_actions( 'template_redirect' );
remove_all_filters( 'template_include' );

$server_timing = perflab_server_timing();
add_filter( 'perflab_server_timing_use_output_buffer', '__return_true' );
$this->assertTrue( $server_timing->use_output_buffer() );
$server_timing->add_hooks();
$this->assertFalse( has_filter( 'template_include', array( $server_timing, 'on_template_include' ) ), 'template_include filter added' );
$this->assertSame( PHP_INT_MIN, has_action( 'template_redirect', array( $server_timing, 'start_output_buffer' ) ), 'template_redirect action not added' );
}

public function test_perflab_server_timing_register_metric(): void {
$this->assertFalse( perflab_server_timing()->has_registered_metric( 'test-metric' ) );

Expand Down