-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core: add desktop score curves for TBT and LCP #10756
Conversation
p10: 2500, | ||
median: 4000, | ||
mobile: { | ||
// 25th and 13th percentiles HTTPArchive -> median and p10 points. |
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.
why is this 13?
is this the x percentile of all web traffic or just desktop? (same question for mobile control points). comment doesn't make it clear, but the link to bigquery specifies mobile so I think it's filtering device type?
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.
why is this 13?
#10715 changed the second control point from the PODR to p10, but in order to not change the curves, the p10 point ended up at a somewhat arbitrary percentile for each audit.
(this PR isn't changing any of the mobile numbers, though (?w=1 makes this a little clearer))
is this the x percentile of all web traffic or just desktop? (same question for mobile control points). comment doesn't make it clear, but the link to bigquery specifies mobile so I think it's filtering device type?
Right, the mobile and desktop data come from the percentiles in the respective http archive tables (*_mobile
and *_desktop
) mentioned in each comment.
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.
I think this was decided to leave the LCP points as they were at some point prior and this is just a noop whitespace change.
Commented at the same time as @brendankenny but he explained better :)
}, | ||
}, | ||
desktop: { | ||
// SELECT QUANTILES(lcp, 10), QUANTILES(lcp, 30) FROM [httparchive.pages.2020_04_01_desktop] |
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.
did you update the query here @brendankenny? I'm a little confused.
AFAICT it's supposed to return every ~11th quantile and then every ~3.3rd quantile but which ones were used? also I get validation errors that lcp
isn't a property in the pages
table
shouldn't we use 25th and 5th if we can and stick to the QUANTILES(lcp, 21)
query?
would also be nice to standard sql in our new queries moving forward and use APPROX_QUANTILES
but no biggie that's a separate topic :)
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.
AFAICT it's supposed to return every ~11th quantile and then every ~3.3rd quantile but which ones were used?
the HTTP Archive WPT desktop/mobile distributions are far enough away from the Lighthouse distributions that we can't really base it just on the desktop numbers so simply I don't think. As an example, the pages_mobile
numbers would give {p10: 4648, median: 5978}
for the mobile scoring control points here (vs our 2500/4000). That's why I mentioned comparing those curves to the Lighthouse curve, then I tried to pick analogous points to our mobile control points based on the pages_desktop
curve.
shouldn't we use 25th and 5th if we can and stick to the
QUANTILES(lcp, 21)
query?
But these are pretty close to my results ({p10: 1193, median: 2419}
) so let's ignore what I thought was useful and let's keep it somewhat standardized :)
would also be nice to standard sql in our new queries moving forward and use APPROX_QUANTILES but no biggie that's a separate topic :)
ha, I forgot that's actually the legacy SQL function and thought it was just a shorthand for APPROX_QUANTILES
and an offset :) I can update to that.
also I get validation errors that lcp isn't a property in the pages table
yeah, it's not in any of the summaries, this is going into the json payload and getting "$['_chromeUserTiming.LargestContentfulPaint']"
. I can include that as well.
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 is going into the json payload and getting "$['_chromeUserTiming.LargestContentfulPaint']". I can include that as well.
Gotcha, ya working queries would be 👌 (must imagine Paul's signature noise while reading that)
}, | ||
}, | ||
desktop: { | ||
// SELECT QUANTILES(tbt, 40), QUANTILES(tbt, 60) FROM [httparchive.pages.2020_04_01_desktop] |
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.
ah wait a second, are you trying to say we should use 40th and 60th percentiles? I don't see tbt
as a property either though :/
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.
ah wait a second, are you trying to say we should use 40th and 60th percentiles? I don't see tbt as a property either though :/
this is $._TotalBlockingTime
, but our mobile control points are way off from our standard method of choosing control points (see Deep's original comment above about the mobile control points).
Our normal process would give {p10: 0, median: 83}
for the desktop curve. We could do that but that seems way too harsh, so I chose points on the desktop curve adjusted somewhat similarly to the existing mobile ones.
Happy to change if you feel differently, though.
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.
I wasn't asking because of the specific quantiles chosen :)
I forgot that's actually the legacy SQL function and thought it was just a shorthand for APPROX_QUANTILES and an offset
Is the single sentence that explains 70% of my confusion. You were trying to use quantiles to return a single offset so 40th and 60th was the goal here rather than "give me 40 quantiles and then 60 quantiles". It all makes sense now!
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.
I wasn't asking because of the specific quantiles chosen :)
ha, ok, I see now too :) Yeah, I'll update to make it actually clear
const contextMobile = generateContext({throttlingMethod: 'simulate'}); | ||
|
||
const outputMobile = await LCPAudit.audit(artifactsMobile, contextMobile); | ||
expect(outputMobile.numericValue).toBeCloseTo(2758.2, 1); |
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.
if we're using lantern for this number we should use snapshots, if you just want to test the scoring stuff, maybe use throttlingMethod: 'provided'
?
const contextDesktop = generateContext({throttlingMethod: 'simulate'}); | ||
|
||
const outputDesktop = await LCPAudit.audit(artifactsDesktop, contextDesktop); | ||
expect(outputDesktop.numericValue).toBeCloseTo(2758.2, 1); |
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.
same here
|
||
it('adjusts scoring based on form factor', async () => { | ||
const artifactsMobile = generateArtifacts({trace, devtoolsLog, TestedAsMobileDevice: true}); | ||
const contextMobile = generateContext({throttlingMethod: 'simulate'}); |
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.
same here
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.
same here
yeah, lcp was just c/p from here and is fine to switch, but for TBT I was just trying to slow things down enough to get a scoring difference (with provided
both are 1). I'll find some other trace to test.
p10: 2500, | ||
median: 4000, | ||
mobile: { | ||
// 25th and 13th percentiles HTTPArchive -> median and p10 points. |
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.
I think this was decided to leave the LCP points as they were at some point prior and this is just a noop whitespace change.
Commented at the same time as @brendankenny but he explained better :)
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.
queries look great, much better than my sad QUANTILES(renderStart, 21)
too :)
LGTM
After lining up http archive mobile and desktop distributions with the Lighthouse distribution, doing some simple regressions, then crossing my eyes and transcribing cosmic ray damage to my retinas as it occurred, we have some new desktop curves for TBT and LCP.
They seem ok.
part of #9774