-
Notifications
You must be signed in to change notification settings - Fork 110
Disable gzip compression of URL Metric by default #1924
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
Conversation
Co-authored-by: Felix Arntz <[email protected]>
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. |
Content-Encoding
header instead of Content-Type
to indicate gzip compression, use fetch()
with keepalive
instead of navigator.sendBeacon()
, and add od_gzip_url_metric_store_request_payloads
filter
@@ -722,6 +722,7 @@ export default async function detect( { | |||
urlMetric.elements.push( elementData ); | |||
elementsByXPath.set( elementData.xpath, elementData ); | |||
} | |||
breadcrumbedElementsMap.clear(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This didn't make sense at the end of the function. So I moved it up here after it is last used.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1924 +/- ##
==========================================
- Coverage 71.39% 71.38% -0.01%
==========================================
Files 86 86
Lines 7043 7042 -1
==========================================
- Hits 5028 5027 -1
Misses 2015 2015
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
@westonruter I saw this coming by but don't have capacity to review it right now. I can try Monday morning, but especially since it's a bug fix, I think it would be reasonable to milestone it for the following beta instead - unless it's a critical bug that would affect lots of sites? |
@felixarntz Monday morning should be good. I'm going to deploy to my site and test in Chrome, Safari, and Firefox to make sure that URL Metrica are gathered as expected. Since the gzip compression is a new feature in the release, the bugfix to ensure |
This PR is probably a dead end based on what I've found in #1928 |
This reverts commit 9af3fad.
Content-Encoding
header instead of Content-Type
to indicate gzip compression, use fetch()
with keepalive
instead of navigator.sendBeacon()
, and add od_gzip_url_metric_store_request_payloads
filter
As opposed to keeping the code but disabling it by default, I've opened #1929 to revert the code altogether. |
This is a follow-up to:
Most recently, it now addresses #1928 by disabling gzip compression by default since doing so at the
pagehide
event results in errors in Safari and thedetect()
function's execution being aborted at the point that compression is performed.This PR also does the following, which were the original scope of this PR:
Content-Encoding
header instead ofContent-Type
to indicate gzip compressionfetch()
withkeepalive
instead ofnavigator.sendBeacon()
od_gzip_url_metric_store_request_payloads
filter (which now isfalse
by default)In the former, gzip compression was introduced for the URL Metric JSON data being submitted. This drastically reduces the payload size which is important because there is a 64 KiB limit for beacons. However, when I look at the request in DevTools I see the following:
Notice how the request payload is displayed in binary in DevTools. This got me to thinking whether using the
application/gzip
was correct. At first I thought maybeapplication/json+gzip
would be a solution, but that isn't standard. Then I remembered theContent-Encoding
request header. It allows for theContent-Type
to remainapplication/json
(which seems important!) but then to also send aContent-Encoding: gzip
when the request body is compressed with gzip.Nevertheless, it is not possible to set the
Content-Encoding
header when usingnavigator.sendBeacon()
however. But, there is a new equivalent tonavigator.sendBeacon()
by usingfetch()
with thekeepalive: true
option. This was previously discovered in #1311 (comment). This is Baseline Widely Available (caniuse). Andfetch()
withkeepalive
does allow you to include arbitrary request headers (as well as use HTTP methods other thanPOST
). Additionally, according to umami-software/umami#1160,navigator.sendBeacon()
is blocked by ad blockers like uBlock Origin, so switching tofetch()
withkeepalive
could result in additional URL Metrics being collected.I conferred with Gemini which confirmed that using
Content-Type: application/json
withContent-Encoding: gzip
is the correct and standard approach: https://g.co/gemini/share/ba550d36273eWith all this said, Chrome DevTools still doesn't pretty-print the gzip-encoded JSON. But this seems like a limitation with DevTools.
Nevertheless, if it so happens that the server is unable to accept gzip-compressed JSON request bodies, or if during development you want to be able to see the uncompressed data being sent, this PR introduces
od_gzip_url_metric_store_request_payloads
filter which can prevent gzip compression from being used, regardless of whethergzdecode()
is available in PHP. This isfalse
by default since.This PR also updates the detection script to log out the compression percentage when gzip is being used. Lastly, it moves the
OD_REST_URL_Metrics_Store_Endpoint::decompress_rest_request_body()
method to be a separateod_decompress_rest_request_body()
function per #1865 (comment).