core(lhr): expose environment info by patrickhulce · Pull Request #5871 · GoogleChrome/lighthouse

patrickhulce · 2018-08-20T22:20:35Z

Summary
Before we can start automatically adjusting throttling settings, we need to be collecting and detecting more environment information. This PR adds a environment property (proposed name) to the LHR that will contain the host machine information, detected device class, perhaps more network diagnostic information, etc.

Right now it has 3 properties

hostUserAgent - This is just a copy of the lhr.userAgent property for now (assumed we can deprecate and remove in v4), the User Agent of the version of Chrome that Lighthouse is using.
networkUserAgent - This is the user agent that was sent over the network to the site. It's been frequently requested and confused with the hostUserAgent so makes sense to provide both.
benchmarkIndex - This is the ULTRADUMB™ benchmark value that will eventually be used to autoset throttling if it proves to be stable and reliable enough in our field testing (anecdotally it's already been much more volatile in the runs I've tried than it was on a refreshed Chrome page :/ we might need to have longer timespans and persist the value to user storage somewhere).

I've also added this information to the Runtime Settings section of the report.

Related Issues/PRs
closes #5665

brendankenny

nice! this seems like it'll be great to have to start being able to have a normalized view of lighthouse results

I like environment for the name. Maybe we should rename Util.getEnvironmentDisplayValues to get rid of the last references to the old environment stuff? Unless we want to include all the other values in there?

brendankenny · 2018-08-22T01:02:28Z

    }

+    export interface Environment {
+      hostUserAgent: string;


can you copy the above doc strings down for these too?

brendankenny · 2018-08-22T01:15:53Z

+        if (userAgentEntry && !baseArtifacts.NetworkUserAgent) {
+          // @ts-ignore - guaranteed to exist by the find above
+          baseArtifacts.NetworkUserAgent = userAgentEntry.params.request.headers['User-Agent'];
+        }


could also do baseArtifacts.NetworkUserAgent = await driver.getUserAgent() after GatherRunner.setupDriver() is called. It's shorter, but is it better to just look at the actual network traffic?

Given the number of ways that have popped up in issues as the way folks are trying to set the user agent string, I thought it was best to grab it directly from network records instead of assume it's done being manipulated after setupDriver

SG. I guess HostUserAgent might end up weird too if people are overriding things

brendankenny · 2018-08-22T01:16:55Z

+ * @see https://docs.google.com/spreadsheets/d/1E0gZwKsxegudkjJl8Fki_sOwHKpqgXwt8aBAfuUaB8A/edit?usp=sharing
+ *
+ * The benchmark creates a string of length 100,000 in a loop.
+ * The returned index is the number of numbers per second the string can be created.


number of times per second?

heh, yep fixed :)

brendankenny · 2018-08-22T01:17:47Z

+ *  - <75 is a budget Android phone, Alcatel Ideal, Galaxy J2, etc
+ */
+/* istanbul ignore next */
+function ultradumbBenchmark() {


I know we just had a whole thing with page-functions, but I kind of wish this was in its own file for easier running :) I guess we can always add yarn ultradumbBenchmark later

brendankenny · 2018-08-22T01:19:22Z

      LighthouseRunWarnings: [],
-      UserAgent: await options.driver.getUserAgent(),
+      HostUserAgent: await options.driver.getUserAgent(),
+      NetworkUserAgent: '', // updated later


NetworkUserAgent doesn't seem self-obvious to me but I don't have a great suggestion for an alternative. Will brainstorm :)

sg, I also tried RuntimeUserAgent but thought NetworkUserAgent was a bit more descriptive

yeah. I still have nothings, so that's fine with me. If @paulirish has ideas we can do a follow up PR quickly before we ship it :)

brendankenny · 2018-08-22T01:20:14Z

  }

+  /**
+   * @return {Promise<number>}


will you add a simple docstring here?

brendankenny · 2018-08-22T01:21:04Z

-      UserAgent: await options.driver.getUserAgent(),
+      HostUserAgent: await options.driver.getUserAgent(),
+      NetworkUserAgent: '', // updated later
+      BenchmarkIndex: await options.driver.getBenchmarkIndex(),


do we want to run the benchmark in the target page (vs about:blank or whatever we end up on)? Seems like what's going on in the page could affect the results

yeah I think we'll want to experiment with when this gets evaluated, for example at the moment in CLI case I think it's being affected by being run while Chrome is still being setup. The values I'm seeing are noticably lower than when I run it after page load.

good call moving it just after load blank which does the 300 ms waiting already much improves the estimate 👍

good call moving it just after load blank which does the 300 ms waiting already much improves the estimate 👍

ha, I was going to come back later and say maybe it doesn't make a difference and isn't worth it, so good to hear :)

I guess it's possible it'll cause more issues with moving off using about:blank in the future, but that's a problem for another day :)

brendankenny · 2018-08-22T01:22:55Z

-      assert.equal(results.UserAgent, 'Fake user agent', 'did not find expected user agent string');
-    });
+    const results = await GatherRunner.run([], options);
+    expect(Number.isFinite(results.BenchmarkIndex)).toBeTruthy();


I feel like you must be trolling here...

brendankenny

LGTM!

brendankenny · 2018-08-22T23:33:42Z

+
+const ultradumbBenchmark = require('../lib/page-functions').ultradumbBenchmark;
+
+console.log('Running ULTRADUMB™ benchmark 10 times...')


lint says you need a ; :)

patrickhulce · 2018-08-22T23:59:54Z

@paulirish will probably want your eyes on these names to do another pass before we actually ship, ship this :)

core(lhr): expose environment info

d88c6e1

patrickhulce requested review from brendankenny and paulirish as code owners August 20, 2018 22:20

brendankenny reviewed Aug 22, 2018

View reviewed changes

patrickhulce added 2 commits August 22, 2018 09:36

feedback

7fe2000

move benchmark

9179680

brendankenny approved these changes Aug 22, 2018

View reviewed changes

brendankenny reviewed Aug 22, 2018

View reviewed changes

;

3155bfc

patrickhulce merged commit 1d82597 into master Aug 23, 2018

patrickhulce deleted the benchmark branch August 23, 2018 00:00

patrickhulce mentioned this pull request Oct 3, 2018

core(throttling): add option to slowdown to a device class #6162

Closed

mattzeunert mentioned this pull request Feb 17, 2019

Add isMobile boolean to audit context / create Environment artifact #7043

Closed


		const ultradumbBenchmark = require('../lib/page-functions').ultradumbBenchmark;

		console.log('Running ULTRADUMB™ benchmark 10 times...')

Conversation

patrickhulce commented Aug 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce Aug 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patrickhulce commented Aug 22, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

patrickhulce commented Aug 20, 2018 •

edited

Loading

patrickhulce Aug 22, 2018 •

edited

Loading