-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: use the same scoring curve for desktop in all channels #9911
Conversation
scoreMedian: 4000, | ||
}; | ||
static getDefaultScoreOptions(artifacts, context) { | ||
if (context.settings.emulatedFormFactor === 'mobile' || artifacts.HostDevice === 'mobile') { |
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 logic is right. @paulirish can you confirm?
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 logic is right. @paulirish can you confirm?
#9436 is scoring for desktop runs, which could either be emulated desktop or no emulation but actually running on a desktop. So don't we already have the artifact (setting aside the general usefulness of a HostFormFactor
) and this line could be if (artifacts.TestedAsMobileDevice) {
?
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.
Emulating desktop on a mobile device doesn't make much sense to me. Shouldn't those runs be scored with numbers tuned for a mobile device?
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.
Emulating desktop on a mobile device doesn't make much sense to me. Shouldn't those runs be scored with numbers tuned for a mobile device?
it doesn't make much sense why someone would be doing that to me either :) but if lantern doesn't correctly emulate desktop there it should be fixed at that level, not patched up at the scoring level
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.
Yah I agree that TestedAsMobileDevice
gives us the signal we want.
real mobile hardware + desktop emulation is stupid but that's solved with a warning telling the developer they're stupid rather than mis-scoring them.
Thank god we don't have first-class support for tablet with a mouse or whatever. :)
I still like having HostFormFactor in the artifacts tho.. even if we aren't using it here.
I guess that could be a separate PR, but no big deal.
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.
nice, I like this approach overall
types/artifacts.d.ts
Outdated
@@ -29,6 +29,8 @@ declare global { | |||
LighthouseRunWarnings: string[]; | |||
/** Whether the page was loaded on either a real or emulated mobile device. */ | |||
TestedAsMobileDevice: boolean; | |||
/** Device which Chrome is running on. */ | |||
HostDevice: 'desktop'|'mobile'; |
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.
🚲 🏠 HostFormFactor
?
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.
+1. What do others think?
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.
+1 to hostformfactor
@@ -54,12 +64,13 @@ class FirstContentfulPaint extends Audit { | |||
const devtoolsLog = artifacts.devtoolsLogs[Audit.DEFAULT_PASS]; | |||
const metricComputationData = {trace, devtoolsLog, settings: context.settings}; | |||
const metricResult = await ComputedFcp.request(metricComputationData, context); | |||
const scoreOptions = context.options || this.getDefaultScoreOptions(artifacts, context); |
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 we have this be performed by the runner? i.e. check if default options is a function that accepts arguments and invoke it for the default options and/or permanently convert it to a function instead of getter property?
I like this idea of passing in context to determine default options overall 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.
permanently convert it to a function instead of getter property?
That was another approach I started with. @paulirish and I thought it best to avoid changing the interface for default options, but I didn't consider supporting both a function and the (current) getter property, which I like.
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.
thought it best to avoid changing the interface for default options
If we had a bunch of users of our default options already I'd agree, but we're still in the early stages there so if we'd like to move IMO now would be the time rather than indefinitely support two modes :)
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.
That was another approach I started with. @paulirish and I thought it best to avoid changing the interface for default options, but I didn't consider supporting both a function and the (current) getter property, which I like.
If we had a bunch of users of our default options already I'd agree, but we're still in the early stages there so if we'd like to move IMO now would be the time rather than indefinitely support two modes :)
having a
static getDefaultOptions(context) {
if (artifacts.TestedAsMobileDevice) {
return {
scorePODR: 2000,
scoreMedian: 4000,
}
} else {
return {
scorePODR: 800,
scoreMedian: 1600,
}
}
}
}
static async audit(artifacts, context) {
const scoreOptions = context.options;
// ...
}
seems ergonomically (and functionally) equivalent to something like
static get defaultOptions() {
return {
mobile: {
scorePODR: 2000,
scoreMedian: 4000,
},
desktop: {
scorePODR: 800,
scoreMedian: 1600,
},
};
}
static async audit(artifacts, context) {
const scoreOptions = context.options[TestedAsMobileDevice ? 'mobile' : 'desktop'];
// ...
}
and avoids churn for anyone using audit options.
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.
but speaking of users of default options...is there any reason to keep score control points in the options? The main push for that was to be able to make a desktop config that defined its own score curves (#4873) to override the default ones, but if desktop scoring is being moved into the audits themselves, any reason to continue defining them circuitously like this instead of as regular properties?
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.
tho I am assuming some user actually uses this
Looks like the only people using it are folks that are reusing the desktop config.
but speaking of users of default options...is there any reason to keep score control points in the options?
I was thinking the same thing.
I'm in favor of moving scoring out out of options. (Roughly reverting #4927) If a user really wants different scoring, they can create a plugin that requires a core metric audit and then recomputes the score their way.
I'm happy to add friction there. I want audit options to be used for all sorts of other things but core perf metric scoring doesn't seem like something that should be part of our extensibility story.
@patrickhulce how sad would you be?
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.
Looks like the only people using it are folks that are reusing the desktop config.
A GitHub search would find any open source tools that use Lighthouse + these config options, but it doesn't tell us anything about how power users might be using configs in their infra to run Lighthouse.
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.
Setting it up with options allows folks to run with advanced throttling on different connection types and run with just a slightly modified config ala lr-desktop-config
. To say that removing scoring from options adds friction feels like quite an understatement. If we removed, they would need to completely reimplement every metric audit just to have meaningful scores of any connection type outside our two blessed ones, which sounds like an effective way to completely kill testing of other connection types.
We also originally structured it this way to offer metric values on different connection types. We had dreams once upon a time of offering multiple views within the same report and while no one has really talked about something like that recently, we get that for free when the inputs all come from options and it would be rather difficult to accomplish and/or require hard-coded copies without it. If #8374 had been able to land, I would have shipped my slow-3g performance plugin that does this :)
Nuking scoring in options kills both of these dreams, and I'm not personally convinced that a custom config isn't already enough friction to discourage it. AFAICT from my brief search there are exactly 0 usages of custom (EDIT: Paul already said this, so I think we interpret that finding differently, agree with connor that it's just power users then :) )scorePODR
in open source github where the values aren't just copied from our guidance or lr-desktop-config
.
In summary, I would be very, very sad to see this die.
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.
okay. :)
we'll keep the scoring parameters in audit options.
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 don't think it's a huge burden to make users do class Slow3gInteractive extends Interactive {static getScoreOptions {return myAlteredScoreOptions;}
. But we could also be rid of the (undocumented) passing of an audit's default options back into itself with a simple const {options = this.defaultOptions} = context
in the audit and still have the override behavior without runner
trickiness.
...but I can live with the status quo :)
But I would very much like to see no new options
magic for something we all agree no one uses to do something complicated when we can do it in a simple way :)
|
||
return { | ||
score: Audit.computeLogNormalScore( | ||
metricResult.timing, | ||
context.options.scorePODR, | ||
context.options.scoreMedian | ||
scoreOptions.scorePODR, |
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.
probably should still note this as a breaking change in release notes, but this WFM 👍
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.
agreed. we should adopt a way to signify breaking changes for release-notes-writing time.
@@ -22,7 +22,7 @@ describe('Performance: first-meaningful-paint audit', () => { | |||
const context = {options, settings: {throttlingMethod: 'provided'}, computedCache: new Map()}; | |||
const fmpResult = await FMPAudit.audit(artifacts, context); | |||
|
|||
assert.equal(fmpResult.score, 1); | |||
assert.equal(fmpResult.score, 0.96); |
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.
kinda weird to see all these scores change in the tests.
im assuming thats because TestedAsMobileDevice
is falsy by default in these tests.
that's a little odd as LH is is otherwise mobile by default.
we need something better here. options i can think of:
- add
{TestedAsMobileDevice: true}
into all these tests to make it explicit - change the ternary in the metric audits to check if
TestedAsMobileDevice === false ? 'desktop' : 'mobile'
because i guess it'd beundefined
in all these test scenarios?
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 considered both of those.
the first was annoying and a big change to the tests
the second is ehhhh. I would do this if only there was one single place this condition lived. but it's in many places
but
that's a little odd as LH is is otherwise mobile by default.
is so convincing that we should do one of these. I choose the second. is there a good place to move this code to a function?
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.
ehhhhh i just did 2)
Fixes #9436.
The first inclination was to add some overrides in
config.js
, kinda like this. But having the scoring curve numbers co-located with the audits is just so good, so let's try to keep them together.The next inclination was to modify the audit
context
to havehostDevice
. However, this was kind of awkward. For example, there's no great place to keep the logic of host agent checking, which is needed in creating the base artifacts and now here.So instead of avoiding making a new artifact, I suggest we embrace it. This will be useful for #9713 too.
Draft to get consensus. I only did one audit.