-
Notifications
You must be signed in to change notification settings - Fork 24
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
Add custom metric to measure generated content (size and percent) #108
Conversation
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.
Diverging a bit from the WPT metric but these changes should make it a bit easier to analyze at scale
While I agree with your suggestions @rviscomi I wonder if it's worth the divergence from the Catchpoint ones (which btw will be what will is generated from a manual run on webpagetest.httparchive.org)? One of the reasons we're adding these is that it was a confusing that they half available in our instance (in manual runs, but not the crawl). Maybe we should go with it as is for now and try to suggest upstream changes to theirs and, if that is accepted, then merge that change in? |
We are running a fork so I could just delete the catchpoint version of the
metrics. All of the server custom metrics should be disabled anyway to be
consistent with the crawl anyway.
…On Thu, Jan 18, 2024 at 12:40 PM Barry Pollard ***@***.***> wrote:
While I agree with your suggestions @rviscomi
<https://github.com/rviscomi> I wonder if it's worth the divergence from
the Catchpoint ones (which btw will be what will is generated from a manual
run on webpagetest.httparchive.org)?
One of the reasons we're adding these is that it was a confusing that they
half available in our instance (in manual runs, but not the crawl).
Maybe we should go with it as is for now and try to suggest upstream
changes to theirs and, if that is accepted, then merge that change in?
—
Reply to this email directly, view it on GitHub
<#108 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AADMOBIJY4U6CFEVONQMUQLYPFNAVAVCNFSM6AAAAABCAQ6FKGVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOJYHEZTGMRRGQ>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
That SGTM. It will still be inconsistent with webpagetest.org but I can live with that. @kevinfarrugia do you want to add to this PR? |
Yes, it makes sense to me. There are already some small changes, so I think it's fine to not have the same implementation. I'll just double check everything and push an update. |
Co-authored-by: Rick Viscomi <rviscomi@users.noreply.github.com>
Heads up that one of the tests in the GitHub action did not run. |
I took a look at the test results for https://almanac.httparchive.org/en/2022/ and it reports negative values. This is correct. The initial HTML response is larger than the one after JavaScript executes as JavaScript is used to update or remove elements, for example those containing the CSS classes |
@pmeenan If I understood correctly, removing the catchpoint version of the metric should be done from the webpagetest repo. If that is correct, is there anything else needed to include in this PR? |
I removed all of the server-side custom metrics from our fork (just now) so it will accurately reflect what will be measured in the crawl. |
@kevinfarrugia is it expected that the results for pesuar.com are In the top comment you noted that you got |
No, that is not expected. The results should be If I add the custom metric to webpagetest.org it works as expected. |
Weird ok. I can get the custom metric to succeed in a manual test:
|
@pmeenan are you concerned about the test failure or do you think we can safely merge this? (the test consistently failed on this page a few times) |
Likely safe to merge (worst case should be that the metric is missing). That said, it might be worth some error handling for the case where the response_body (or even request 0) doesn't exist. I THINK that if the custom metric throws it should just be empty but that's the only part that doesn't look safe. |
Custom metrics for https://almanac.httparchive.org/en/2022/WPT test run results: http://webpagetest.httparchive.org/results.php?test=240126_6D_5 Custom metrics for https://imkev.dev/WPT test run results: http://webpagetest.httparchive.org/results.php?test=240126_NY_6 {
"_generated-content": {
"percent": "0.0855",
"sizeInKB": "2.42"
}
} Custom metrics for https://pesuar.com/WPT test run results: http://webpagetest.httparchive.org/results.php?test=240126_4F_7 {} |
Adds support for
generated-content
custom metric that includes percentage and size in KB. This is helpful to recognize web pages that rely on JavaScript for generating most of the content, such as CSR apps.As an example, see
01_test
custom metric in the following runs:Test websites: