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

Make Server-Timing header output more robust #736

Merged
merged 1 commit into from May 30, 2023
Merged

Conversation

swissspidy
Copy link
Member

@swissspidy swissspidy commented May 30, 2023

Summary

I activated the plugin on my local WP core Docker environment and filtered perflab_server_timing_use_output_buffer to enable output buffering, yet I did not see the Server-Timing header.

The shutdown hook is too late to send the header, but using the ob_start() callback for this works.

Relevant technical choices

Checklist

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

@swissspidy swissspidy added the [Type] Bug An existing feature is broken label May 30, 2023
@felixarntz
Copy link
Member

@swissspidy Can you share a bit more context? What leads to it being "too late"? I have never encountered the problem, so would like to understand better.

@felixarntz felixarntz added the Infrastructure Issues for the overall performance plugin infrastructure label May 30, 2023
@felixarntz felixarntz added this to the 2.4.0 milestone May 30, 2023
@swissspidy
Copy link
Member Author

When I run this in WP core‘s Docker environment, the callback in the shutdown hook is never fired, so it‘s not sending the header. I assume it‘s because the shutdown handler is called long after the page has been served.
Weirdly it‘s not an issue when using this with LocalWP, just with the core env.

@felixarntz
Copy link
Member

@swissspidy That's odd. I have been using this in the WP core Docker env several times without problems 🤔 Maybe it's something more specific than that?

@swissspidy
Copy link
Member Author

IDK 🤷 Maybe I can debug at contributor day or so

Copy link
Member

@westonruter westonruter left a comment

Choose a reason for hiding this comment

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

This is the approach that the output buffering approach that the AMP plugin. It has been robust.

The AMP plugin also sends Server-Timing response headers.

Comment on lines -238 to -239
// phpcs:ignore PHPCompatibility.Constants.NewConstants
defined( 'PHP_INT_MIN' ) ? PHP_INT_MIN : -1000
Copy link
Member

Choose a reason for hiding this comment

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

Curious why this action was configured to run early. Wouldn't it be better to run as late as possible?

Copy link
Member

Choose a reason for hiding this comment

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

It is run early based on the idea that the template output was already completed when shutdown runs. If shutdown did a bunch of other things (extremely unlikely but possible), then it would inflate the time in the wp-template metric.

@felixarntz
Copy link
Member

Will the removal of the ob_get_clean() call in this PR lead to issues? Not very familiar with how output buffers work under the hood, but I always assumed you should also "end" an output buffer that you start.

@westonruter
Copy link
Member

Any open buffers are automatically flushed when the page response is finished, per docs:

If output buffering is still active when the script ends, PHP outputs the contents automatically.

Copy link
Member

@felixarntz felixarntz left a comment

Choose a reason for hiding this comment

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

Tested this in my core dev environment, and it works as expected, also no change in outcomes. LGTM, thanks for the fix!

@felixarntz felixarntz merged commit dcac3ba into trunk May 30, 2023
16 of 21 checks passed
@felixarntz felixarntz deleted the fix-output-buffer branch May 30, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Infrastructure Issues for the overall performance plugin infrastructure [Type] Bug An existing feature is broken
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants