-
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 FCP on 3G audit to JSON #7062
Conversation
*/ | ||
'use strict'; | ||
|
||
const Audit = require('../audit'); |
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.
audit.js
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.
done
@@ -140,6 +140,7 @@ const defaultConfig = { | |||
'without-javascript', | |||
'metrics/first-contentful-paint', | |||
'metrics/first-meaningful-paint', | |||
'metrics/first-contentful-paint-3g', |
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.
should we add this to the lr
config files instead of the default?
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.
yeah good call
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.
we only need it in mobile right @paulirish ?
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.
Was this change only applied to mobile? I was testing with this version and all the desktop scores went down dramatically for first contentful paint. Are we sure that this metrics is not accidentally included in the desktop fcp score calculation?
@brendankenny @paulirish @patrickhulce
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.
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 you're comparing to PSI you need to use the same settings PSI uses.
If you have the repo/code locally, node ./lighthouse-cli <url> --config-path=./lighthouse-core/config/lr-mobile-config.js
. Replace lr-mobile-config.js
with lr-desktop-config.js
for the desktop quivalent.
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.
Oh thank you. This is invaluable information.
I tried to run the command you have given me
node ./lighthouse-cli <url> --config-path=./lighthouse-core/config/lr-desktop-config.js
with the older version and compared the result to the new one.
I noticed something odd. The latest version is saying in the results
Emulated Desktop, Simulated Slow 4G network
whereas tag v3.2.1 says
Emulated Nexus 5X, Simulated Slow 4G network
Does that mean that in version v3.2.1 the configuration lr-desktop-config.js
was not applied?
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.
@patrickhulce seems this issue has put the internet in a frenzy. There are several threads in the PSI group.
Any ideas? It seems that one issue is that compressed assets are no longer being downloaded correctly when comparing to other tools. See this post for example.
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.
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.
@dgargi @patrickhulce thanks so much for the quick investigation. I filed #7151 and if you need more information, let me know. Thanks!
}, | ||
{ | ||
"startTime": 0, | ||
"name": "lh:computed:FirstContentfulPaint", |
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.
these duplicate keys are kind of a bummer
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.
yeah it is :(
I'm not sure it's worth sticking the options into the name though to differentiate 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.
I'm not sure it's worth sticking the options into the name though to differentiate either though :/
yeah, will probably just confuse things without much benefit. If someone cares about that they'll be more likely to be looking at things in the timeline viewer anyways
*/ | ||
static get defaultOptions() { | ||
return { | ||
// 75th and 95th percentiles HTTPArchive on Fast 3G -> multiply by 1.5 for RTT differential -> median and PODR |
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.
multiply what by 1.5?
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.
the 75th and 95th percentiles :)
the ->
is meant to be like a pipe
} | ||
} | ||
|
||
module.exports = FirstContentfulPaint3G; |
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.
tests?
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.
done
@@ -18,7 +18,7 @@ const config = { | |||
throughputKbps: 10 * 1024, | |||
cpuSlowdownMultiplier: 1, | |||
}, | |||
// Skip the h2 audit so it doesn't lie to us. See #6539 | |||
// Skip the h2 audit so it doesn't lie to us. See https://github.com/GoogleChrome/lighthouse/issues/6539 |
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.
❤️
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.
LGTM3G
*/ | ||
'use strict'; | ||
|
||
const Audit = require('../../../audits/metrics/first-contentful-paint-3g.js'); |
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.
can you use the more specific name for this (const FCP3G
or whatever :)
Object { | ||
"score": 1, | ||
"value": 1661, | ||
} |
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.
lol
Summary
Paul tells me we need a FCP value for 3G in the LR response. This uses some rough 3G numbers based on WPT's "Regular 3G" profile and the definition of 3G in network information API spec effective connection type 3G.