Skip to content
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

Reconfigure emulation flags #10910

Closed
paulirish opened this issue Jun 4, 2020 · 10 comments · Fixed by #11779
Closed

Reconfigure emulation flags #10910

paulirish opened this issue Jun 4, 2020 · 10 comments · Fixed by #11779
Assignees

Comments

@paulirish
Copy link
Member

paulirish commented Jun 4, 2020

TLDR: The emulation flags are awkward and there's implicit assumptions made. We should fix that and support our varied usecases with first-class settings. Key idea: get rid of emulatedFormFactor=none.


DevTools, Calibre, and WPT are three lighthouse clients that handle emulation previous to lighthouse running. In DevTools' case, only screen emulation is applied, and lighthouse is expected to apply its own network emulation. And calibre's case, both emulations are applied and the intent is for a lighthouse to not apply double emulation anywhere. LH on WPT supports a few approaches and everyone's been confused at some point.

We introduced the illustrious internalDisableDeviceScreenEmulation flag (#9377) to solve this. (However even with this, LH will still overwrite any previously set userAgentOverride). But if emulatedFormFactor (#6098) is none and external UA emulation is applied, you don't get correct scoring.

This is messy and I think that there's an opportunity to completely redefine this configuration to be more clear.

Goals

  1. no surprises wrt mobile or desktop scoring being used
  2. a more first-class config for this external-emulation setup
  3. continue to support the no-emulation-lh-is-running-against-a-real-phone scenario

Starting Proposal

  • Rename emulatedFormFactor to formFactor. This is the key flag and the value must be either mobile or desktop, none (née provided) is no longer a valid option. Our perf metrics and 3 SEO metrics change their behavior based on mobile v desktop, so it seems important to make this an explicit user contract.
  • Add two more flags: disableScreenEmulation and disableNetworkEmulation.
  • Remove internalDisableDeviceScreenEmulation. Throw if this or emulatedFormFactor are used.
  • Internally, we drop the guessed TestedAsMobileDevice bool and just use the formFactor enum of 'desktop'|'mobile'.

The combination of these allows us to support the below user stories with clear expectations. It also prevents the case of a using incorrect scoring accidentally.

User stories

I believe we have 4 user stories to support (right?)

  1. Typical LH (we apply emulation): formFactor is set.
  2. DevTools: formFactor is set via the mobile/desktop radio, and screenEmulation is false.
  3. Calibre and WPT: formFactor is set, and both screenEmulation: false, throttlingMethod: 'provided'. emulatedUserAgent is set as desired.
  4. LH on mobile device: --no-screenEmulation and --throttling.cpuSlowdownMultiplier=1. (--formFactor=mobile is the default already)

Extra thoughts and questions


References

@paulirish
Copy link
Member Author

paulirish commented Jun 4, 2020

cc @wildlyinaccurate I have a feeling you'd have been much happier with the above proposal, so you wouldn't have had to do all the investigation in catchpoint/WebPageTest.agent#297. But curious for your thoughts. :)

also cc @benschwarz @alekseykulikov @mattzeunert

@connorjclark
Copy link
Collaborator

connorjclark commented Jun 4, 2020

Related: I think we may need something like setEmulation cb in the Lighthouse runner API so that we can reset the emulation correctly for programmatic usages. See #10716 (comment)

@patrickhulce
Copy link
Collaborator

Rename emulatedFormFactor to formFactor. This is the key flag and the value must be either mobile or desktop, none (née provided) is no longer a valid option.

Sounds great, love it 👍

Add disableScreenEmulation

Basically required to do the above, SGTM 👍

Remove internalDisableDeviceScreenEmulation. Throw if this or emulatedFormFactor are used.

Sounds like good housekeeping given the above 👍

Internally, we drop the guessed TestedAsMobileDevice bool and just use the formFactor enum of 'desktop'|'mobile'.

Feels a little weird that formFactor is at this point just acting more like scoringMode: 'easy' | 'hard' instead of what the form factor actually was. Not sure I completely buy into dropping TestedAsMobileDevice for double checking it matched formFactor

Or with the proposal implemented, we may just log a warning to let them know we see it.

Speak of the 👿 Yeah let's do that instead :)

Add disableNetworkEmulation

This is the one I don't understand :) Sounds like it's just returning us to the old days of duplicate flags controlling the same thing.

@brendankenny
Copy link
Member

brendankenny commented Jun 15, 2020

Old comment from #9377 (review)

If we were designing our flags from scratch we definitely wouldn't want to overlap with emulatedFormFactor like this :/

The combo with provided makes emulatedFormFactor actually emulatedUAString, and if you're just trying to get your settings right, it's not clear why you'd want emulatedViewportMethod vs emulatedFormFactor and what each one does (and is Lighthouse providing provided or am I?).

See below for one suggestion to simplify this, but what if we just went all the way and added free-form emulation variables for UA string, metrics, and touchEnabled (in the style of throttling)? emulatedFormFactor would stay a set of presets that most would use, but it would be possible to override. This would also unlock what PaulKinlan is looking for re: feature phones.

Not sure if I completely agree with this, but just splitting up all our emulation options so that they don't have interactions (unexpected or otherwise) anymore does have a certain appeal. Most people will still use the presets, and the people heavily tweaking will want to heavily tweak anyways.

@paulirish
Copy link
Member Author

paulirish commented Jun 15, 2020

discussed. plan:

  1. lets expose screen emu properties (width, height, dpr) to config.
  2. let's expose UA to config. Allow custom user-agent via --extra-headers #8756
  3. determine disable* in relation to these.
  4. perhaps in the calibre/wpt "provided" throttling setup, we should have more useful text in runtime settings. do they want to set a string or .... ? HTML Report renderer incorrect labels for custom throttling & device #7053
    image

updated:

  1. probably dont need disableScreenEmulation and instead can pass false as screenEmulation config value
  2. probably need to determine inconsistencies between formFactor and emulation state. (testing as mobile but it looks like desktop)
  3. emulatedUserAgent is also falseable
  4. --preset=desktop
  5. extends: 'lighthouse:mobile', and extends: 'lighthouse:desktop', @patrickhulce
    • perhaps we drop lighthouse-default as it's a mobile thing and too many default assumptions?

@benschwarz
Copy link
Contributor

Thanks for the ping @paulirish ✌️

lets expose screen emu properties (width, height, dpr) to config.
let's expose UA to config.
determine disable* in relation to these.

SGTM.

perhaps in the calibre/wpt "provided" throttling setup, we should have more useful text in runtime settings. do they want to set a string or .... ?

Yeah, for completeness I think it'd be good to be able to set some properties that explain the test environment more deeply. I raised this on #7053 but never took it anywhere.

@paulirish
Copy link
Member Author

oh word! k we'll break off my number 4 into #7053. perfect.

@amannn
Copy link

amannn commented Sep 10, 2020

Thanks for your great work on lighthouse!

I've experienced some different scores when running lighthouse on CI than when running locally in regards to the total blocking time.

After some research, I think this is due to the cpuSlowdownMultiplier option as it's relative to the available CPU power. My CI runner already is a bit less capable, therefore I was seeing lower numbers there than when testing locally on my MacBook Pro.

I was wondering if there's a way to normalize this behaviour across environments, so the score would be the same, regardless of the machine? Obviously, there would be some minimum CPU requirements, but ideally from there on the result would be the same. I'm not sure if this is somehow possible practically?

@patrickhulce
Copy link
Collaborator

I think you'd be interested in the saga of #9085 and the resulting calibration documentation @amannn :)

tl;dr - we tried to do this, but it's very, very difficult and even benchmarks built by dedicated benchmark companies can't predict machine performance well enough to normalize Lighthouse scores, so a single contributor here using 20% of their time won't be able to solve it either :/

@amannn
Copy link

amannn commented Sep 11, 2020

@patrickhulce Oh right, that calibration guide is really helpful! I definitely understand that this is a hard problem.

Thank you for your help!

paulirish added a commit that referenced this issue Dec 16, 2020
An extra check I wanted to add on top of #11779

It was described back in #10910

> the TestedAsMobileDevice logic correctly handles real-mobile-device, but doesn't handle external UA emulation. We could detect there's external UA emulation being applied and throw if we don't see configuration that matches. Or with the proposal implemented, we may just log a warning to let them know we see i

We can only check this sort of mismatch after we've gathered the host UA, so it can't be done earlier.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants