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

Incorporate Server Timing API #990

Closed
westonruter opened this Issue Mar 6, 2018 · 9 comments

Comments

3 participants
@westonruter
Copy link
Member

westonruter commented Mar 6, 2018

To help facilitate debugging issues related to the performance of the preprocessor, we should leverage the new Server Timing API to report how long each sanitizer is taking to run, how long WordPress took to generate the page prior to the sanitizers running, etc.

Relates to #959.

@westonruter westonruter referenced this issue Mar 13, 2018

Open

Add performance profiling/testing #1017

0 of 5 tasks complete

@westonruter westonruter self-assigned this Mar 27, 2018

westonruter added a commit that referenced this issue Mar 30, 2018

westonruter added a commit that referenced this issue Mar 31, 2018

@westonruter westonruter added this to the v1.0 milestone Apr 4, 2018

juanchaur1 added a commit to juanchaur1/amp-wp that referenced this issue May 9, 2018

@westonruter westonruter added this to Ready for QA in v1.0 May 31, 2018

@kienstra kienstra moved this from Ready for QA to Ready for Merging in v1.0 Jun 6, 2018

@igkuz

This comment has been minimized.

Copy link

igkuz commented Jul 20, 2018

@westonruter hi! Is it possible to turn off Server Timing headers in response? For example, with default configs for PHP-FPM + Nginx as a reverse proxy, Nginx drops requests as headers are too big. Tunning the fastcgi buffers fixes the problem, but it would be great to make these headers optional.
I didn't find in code, how to turn off them...

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 25, 2018

@igkuz It's not currently possible to turn off the Server-Timing headers in the response. What are the Server-Timing headers you are seeing? How are they too big? I see just:

Server-Timing: amp_output_buffer;desc="AMP Output Buffer";dur=325.990915
Server-Timing: amp_dom_parse;desc="AMP DOM Parse";dur=4.322052
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.546932
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=6.416798
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.148058
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.276089
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.092030
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.113010
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.106812
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.201941
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.336885
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.022888
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.175953
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.034809
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=0.024796
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=38.730860
Server-Timing: amp_sanitize;desc="AMP_Tag_And_Attribute_Sanitizer";dur=81.669092
Server-Timing: amp_dom_serialize;desc="AMP DOM Serialize";dur=7.952929

Note that this is only for a response when the post-processor is invoked because the processed result is not cached yet. Once the post-processed response is cached, then the sanitizers are bypassed and none of the Server-Timing headers are sent.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 25, 2018

(As I paste that, I see something is wrong in that it is repeating AMP_Tag_And_Attribute_Sanitizer when it should be noting each separate sanitizer that is running. I'll fix this separately.)

@igkuz

This comment has been minimized.

Copy link

igkuz commented Jul 25, 2018

@westonruter I see the same and AMP_Tag_And_Attribute_Sanitizer is repeating for several times. And when the page is big enough it's a problem. We're using the latest beta build, you can check the output here http://goo.gl/jTqZCW.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 25, 2018

@igkuz The issue of duplicated sanitizer names in the headers will be fixed with #1283.

How many sanitizers are you using? While I expect there to be numerous instances of the Server-Timing header, each instance is relatively short considering how long cookie headers can be. I'm confused as to why Nginx is complaining. I've seen Nginx break when an individual header is too long, but not when there are numerous individual headers. Is there a default limit on the byte size of all response headers combined?

@igkuz

This comment has been minimized.

Copy link

igkuz commented Jul 25, 2018

@westonruter
from Nginx docs:
fastcgi_buffer_size:
By default, the buffer size is equal to one memory page. This is either 4K or 8K, depending on a platform. It can be made smaller, however.

So if you place for example fastcgi_buffer_size 32k; everything is ok. PHP-FPM settings are not far from default ubuntu apt repo package, the changes are not connected with buffers.

We added 4 Sanitizers to the array in our plugin.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 25, 2018

@igkuz I'm seeing a very large number of Server-Timing headers on your URL—60 in fact.

$ curl -i https://setka.io/blog/how-to-use-font-pairs/?amp | grep 'Server-Timing:' | wc -l
60

This is not expected. There should only be ~22 such headers since you added 4 sanitizers.

@igkuz

This comment has been minimized.

Copy link

igkuz commented Jul 26, 2018

@westonruter you're correct, it's 60 of them, we're checking our code also, but still having an opportunity to turn them off would be great.

@westonruter

This comment has been minimized.

Copy link
Member

westonruter commented Jul 26, 2018

@igkuz if you update the plugin to the state after #1283 is merged then you will be able to see the sanitizer names again.

@kienstra kienstra moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment