Add impact of sizes errors metrics

HTTPArchive / custom-metrics

Custom metrics to use with WebPageTest agents

Apache License 2.0

19 stars 22 forks source link

Add impact of sizes errors metrics #30

Closed eeeps closed 2 years ago

eeeps commented 2 years ago

Progress on https://github.com/HTTPArchive/almanac.httparchive.org/issues/2884

As discussed in Slack starting around https://httparchive.slack.com/archives/C017HPKG614/p1652902287349589

I have successfully tested this on a couple of test pages, using WPT. Will run it against a handful of actual pages tomorrow; requesting some external code reviews from people familiar with how Chrome's selection algorithm works right now.

rviscomi commented 2 years ago

I might not have time for a deep code review, but in the meantime @eeeps could you look at some of the linter errors?

eeeps commented 2 years ago

While running this through a suite of actual pages, I found an issue where if w descriptors in the srcset were wrong (e.g.—on netflix.com's hero right now!), this metric attributed that error to the sizes attribute.

Addressing that now...

eeeps commented 2 years ago

Ok, here are some WPT results, showing behavior in different contexts:

css-tricks.com: small sizes errors that do affect srcset selection
harvard.edu: large sizes errors that do not affect srcset selection (because we were already picking the largest resource)
netflix.com: Erroneous w descriptors on hero image that are accounted for separately
And a few pages that show no disruption to existing reporting, in contexts where these metrics don't matter:
- nytimes.com (no w descriptors)
- shutterstock.com (no w descriptors)
- mailchimp.com (shows interaction with lazy loading — we don’t get metrics, even though there are sizes, as expected)

rviscomi commented 2 years ago

@tunetheweb I'm not sure if an HA admin needs to be around to merge PRs but if so could you keep an eye on this while I'm away this week?

tunetheweb commented 2 years ago

Aye. Though travelling myself this week. But much shorter plane flights than you!

@eeeps if you could get the feedback addressed today that would be great!

eeeps commented 2 years ago

@tunetheweb @yoavweiss I think I addressed all of Yoav's feedback but I would like to re-run a few validation tests to make sure that I didn't break anything in the process. I need to run at the moment, I'll try to get back to it in a few hours and give @tunetheweb a final thumbs up.

Thank you!!

eeeps commented 2 years ago

Some updated test results, with @rviscomi and @yoavweiss 's changes:

Diffing vs the earlier results, the only changes I see are:

changed content on the pages
"wDescripor" → "wDescriptor"

Which is as I'd expect. So.... I think we're good to merge, @tunetheweb