-
Notifications
You must be signed in to change notification settings - Fork 109
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
Account for 64 KiB limit for sending beacon data #1851
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. |
I updated the Optimization Detective Admin UI plugin to add a The
Here is the page in question: https://weston.ruter.net/2009/12/12/google-tts/ This page has a larger URL Metric size because I got a lot of comments on this old post, so there are 42 images on the page (due to the Gravatars). |
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.
LGTM!
@felixarntz I added one more thing here (6f96d20) to enforce this 64 KiB limit on the server in the REST API when processing a URL Metric storage request. Since no URL Metric should be larger than 64 KiB it makes sense to me that we should just plain not even accept it. This would be a way to harden against malicious payloads to pollute the |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## trunk #1851 +/- ##
==========================================
+ Coverage 65.97% 66.27% +0.29%
==========================================
Files 88 88
Lines 6895 6956 +61
==========================================
+ Hits 4549 4610 +61
Misses 2346 2346
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@westonruter Thanks, that makes sense. LGTM! |
This comes out of a discovery from #1311 (comment):
I was not aware of this restriction on sending beacon data, but it makes sense. The browser shouldn't be sending huge payloads on pages that the user has closed. Nevertheless, this is a complication for Optimization Detective because a URL Metric can contain a lot of data. When there are 100 images on a page, this results in a URL Metric JSON body of that is ~53,371 bytes or 83% of 64 KiB limit. When there are 200 images, then the URL Metric is ~106,024 bytes, or 166% of 64 KiB limit. However, this 64 KiB limit is not for a single beacon but for all beacons combined. Therefore, if there are various analytics providers attempting to also send a lot of data, then this 64 KiB limit could be breached even when only 50% (32 KiB) of the limit is being used.
So this PR starts to address this issue by:
We can reduce the precision in the numbers to reduce the size somewhat. When there are 100 images, rounding the numbers to the nearest whole number reduces the size from 83% down to 68% of the budget. Other strategies could be employed, like coming up with a compression algorithm to replace common JSON strings. But we can explore that later as we start monitoring how close we are getting to the maximum. I need to look at my own site to see how close the captured URL metrics are to the 64 KiB limit for some real world data. Update: See #1851 (comment).
Screenshots
When there are 3 images:
When there are 100 images:
When there are 200 images: