feat: configurable page timeout #1672

patrickhulce · 2017-02-09T00:35:24Z

paulirish · 2017-02-09T01:09:16Z

lighthouse-core/gather/driver.js

   * @return {!Promise}
   * @private
   */
-  _waitForFullyLoaded(pauseAfterLoadMs) {
+  _waitForFullyLoaded(pauseAfterLoadMs, maxTimeoutMs) {


naming: i prefer the clarity of the old name.. how about maxWaitForLoadedMs ?

paulirish · 2017-02-09T01:12:24Z

lighthouse-core/gather/driver.js

@@ -477,11 +478,15 @@ class Driver {
    const waitForLoad = _options.waitForLoad || false;
    const disableJS = _options.disableJavaScript || false;
    const pauseAfterLoadMs = (_options.flags && _options.flags.pauseAfterLoad) || PAUSE_AFTER_LOAD;
+    let maxWaitMs = MAX_WAIT_FOR_FULLY_LOADED;
+    if (_options.flags && _options.flags.timeout) {
+      maxWaitMs = 1000 * _options.flags.timeout;


x = y * 1000

will do, anything magic I'm not aware of or just style? I know in this case it doesn't matter but I developed a habit of putting the type to coerce to first when dealing with potentially unknown types to avoid {} + '' vs '' + {} scenarios :)

ah i see. totally a style thing.. I'm just used to seeing the integers on the right side.

but good call on doing this for protection with +.

nbd either way

paulirish · 2017-02-09T01:14:41Z

lighthouse-cli/bin.ts

@@ -114,6 +116,7 @@ Example: --output-path=./lighthouse-results.html`
  .default('output', Printer.GetValidOutputOptions()[Printer.OutputMode.pretty])
  .default('output-path', 'stdout')
  .default('port', 9222)
+  .default('timeout', 25)


since the MAX_WAIT_FOR_FULLY_LOADED is still in driver, seems like we should let this value fall through as undefined? wdyt

we double default disableCpuThrottling as well and the extra yargs hints to users won't hurt IMO

main benefit of having it here is the --help printing, but if we want a default it has to happen internally as well for anyone using lighthouse as a module.

What about exposing on Driver as a constant, so it would still be single source of truth .default('timeout', Driver.MAX_WAIT_FOR_FULLY_LOADED)?

A bit overkill to bring in driver just for the constant to me. I don't think it's even that crazy that programmatic access and CLI access could have different sensible defaults at some point either, but I'm fine with removing if we like.

I think in general (at least from a documentation perspective) it's better to make sure they're the same, but in this case my main problem would be development in driver, where you'd have to remember why changes to MAX_WAIT_FOR_FULLY_LOADED aren't doing anything when the CLI is run.

brendankenny · 2017-02-09T01:20:16Z

lighthouse-cli/bin.ts

@@ -62,7 +62,8 @@ const cliFlags = yargs
    'list-trace-categories',
    'config-path',
    'perf',
-    'port'
+    'port',
+    'timeout'


timeout makes me think of a timeout for the whole run or something. Maybe some non-awkward version of page-load-timeout?

yeah I started with that but it felt so clunky and didn't make a ton of sense to have any other timeout, what about adopting the max-wait-for-load name from within if we change?

Thinking about this now, I wouldn't think timeout was the whole run, and there are better tools to accomplish that if you do want to limit that on the command line.

The only other thing I would guess as a user of this would be that timeout would apply to the entirety of each pass, as in not just page load but (∑beforePass()es + page load + ∑afterPass()es), to make sure a particular pass doesn't go on forever. As long as our gatherers are relatively bounded, though, these are roughly the same thing.

(I guess custom gatherers could go crazy, but that's on them)

I think I'm good with going with --timeout, but since it's such prime flag real estate (and not necessarily self-descriptive) maybe @paulirish and @ebidel want to weigh in.

I think we should be more specific. --timeout feels too generic and it's clear from this discussion, that it's ambiguous what it could mean :) page-load-timeout or max-wait-for-load would be cool.

alright sounds good, I'll go with max-wait-for-load for consistency

patrickhulce · 2017-02-09T01:27:08Z

One more thing to note, this can get dangerous if you always set it very high. Testing the verge leads to ugly OOM crashes if you actually try and wait for it when a particularly nasty ad loads...

brendankenny · 2017-02-09T01:29:32Z

what does WPT do in those cases? Is it possible to cleanly wrap up the trace before load has finished?

patrickhulce · 2017-02-09T01:37:41Z

what does WPT do in those cases? Is it possible to cleanly wrap up the trace before load has finished?

AFAICT there's nothing preventing it from also suffering from the OOM case, it just has a timeout that's a little more reasonable that's more likely to contain the events we care about and doesn't process the traces in the same process (does it incrementally in a separate python process)

patrickhulce · 2017-02-09T22:23:06Z

PTAL :)

brendankenny

looking good

brendankenny · 2017-02-11T01:29:43Z

lighthouse-cli/bin.ts

@@ -62,7 +62,8 @@ const cliFlags = yargs
    'list-trace-categories',
    'config-path',
    'perf',
-    'port'
+    'port',
+    'timeout'


Thinking about this now, I wouldn't think timeout was the whole run, and there are better tools to accomplish that if you do want to limit that on the command line.

The only other thing I would guess as a user of this would be that timeout would apply to the entirety of each pass, as in not just page load but (∑beforePass()es + page load + ∑afterPass()es), to make sure a particular pass doesn't go on forever. As long as our gatherers are relatively bounded, though, these are roughly the same thing.

(I guess custom gatherers could go crazy, but that's on them)

I think I'm good with going with --timeout, but since it's such prime flag real estate (and not necessarily self-descriptive) maybe @paulirish and @ebidel want to weigh in.

brendankenny · 2017-02-11T01:31:07Z

lighthouse-cli/bin.ts

@@ -75,6 +77,7 @@ const cliFlags = yargs
    'config-path': 'The path to the config JSON.',
    'perf': 'Use a performance-test-only configuration',
    'port': 'The port to use for the debugging protocol. Use 0 for a random port',
+    'timeout': 'The timeout in seconds to wait for the page to load before continuing',


should we add a warning about large traces and OOM? If you're frustrated with slow page loads (#1464), a large number might be the first thing you try. A full warning may be more appropriate for the readme but more people will see something brief written here.

brendankenny · 2017-02-11T01:31:58Z

lighthouse-cli/bin.ts

@@ -114,6 +117,7 @@ Example: --output-path=./lighthouse-results.html`
  .default('output', Printer.GetValidOutputOptions()[Printer.OutputMode.pretty])
  .default('output-path', 'stdout')
  .default('port', 9222)
+  .default('timeout', Driver.MAX_WAIT_FOR_FULLY_LOADED / 1000)


are seconds the usual thing for command line timeouts? (obvs most of the js world tends towards ms)

When it's a CLI interface for something whose order of magnitude is nearly always measured in seconds I've seen seconds more often than ms, but with a name change to more reflect that it's an internal code setting I think a move to ms is more reasonable.

brendankenny · 2017-02-11T01:35:07Z

lighthouse-core/gather/driver.js

@@ -477,11 +480,15 @@ class Driver {
    const waitForLoad = _options.waitForLoad || false;
    const disableJS = _options.disableJavaScript || false;


while you're in here, do you want to get rid of _options and make options a default {}

brendankenny · 2017-02-11T01:36:44Z

lighthouse-cli/bin.ts

@@ -278,7 +282,8 @@ function saveResults(results: Results,

 function runLighthouse(url: string,
                       flags: {port: number, skipAutolaunch: boolean, selectChrome: boolean, output: any,
-                         outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean},
+                         outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean
+                         timeout: number},


is this just for documentation? I wouldn't think TS would care since we don't use it here

Yeah just for documentation.

paulirish · 2017-02-14T00:33:40Z

lighthouse-cli/bin.ts

@@ -143,6 +147,10 @@ if (!cliFlags.outputPath && cliFlags['output-path']) {
  cliFlags.outputPath = cliFlags['output-path'];
 }

+if (!cliFlags.maxWaitForLoad && cliFlags['max-wait-for-load']) {


how would this be possible? AFAIK, yargs creates the former and makes sure they match at this point.

The bug as mentioned in the above identical check makes this happen when the default is used.

How do we feel about updating yargs? Current is 6.6.0 :(

What was the google3 dependency on yargs that kept us here, are they game to update?

suspect that's it. Just complaining out loud that that restriction continues to hold us back.

Let's bump yargs from our 3.30.0 to 3.32.0.
It has the fix for these: yargs/yargs@51c0063

done, from the looks of it google3 is still on lighthouse ~1.1 so we likely will have quite a few other issues to iron out when they want to upgrade anyway...

ebidel · 2017-02-14T19:58:57Z

lighthouse-cli/bin.ts

@@ -75,6 +77,7 @@ const cliFlags = yargs
    'config-path': 'The path to the config JSON.',
    'perf': 'Use a performance-test-only configuration',
    'port': 'The port to use for the debugging protocol. Use 0 for a random port',
+    'max-wait-for-load': 'The timeout in milliseconds to wait for the page to load before continuing. WARNING: Very high values can lead to large traces and instability',


nit: The timeout (in milliseconds) to wait before the page is considered done loading and the run should continue. WARNING:...`

ebidel · 2017-02-14T20:13:32Z

lighthouse-core/gather/driver.js

+    const waitForLoad = options.waitForLoad || false;
+    const disableJS = options.disableJavaScript || false;
+    const pauseAfterLoadMs = (options.flags && options.flags.pauseAfterLoad) || PAUSE_AFTER_LOAD;
+    const maxWaitMs = (options.flags && options.flags.maxWaitForLoad) ||


should we support --max-wait-for-load = 0?

I would say that would lead to a useless lighthouse run that wouldn't return any results so filling in default is fine with me :)

ebidel · 2017-02-14T20:15:28Z

readme.md

@@ -149,6 +149,11 @@ Configuration:
  --list-trace-categories       Prints a list of all required trace categories and exits   [boolean]
  --config-path                 The path to the config JSON.
  --perf                        Use a performance-test-only configuration                  [boolean]
+  --port                        The port to use for the debugging protocol. Use 0 for a
+                                random port.                                         [default: 9222]
+  --max-wait-for-load           The timeout in milliseconds to wait for the page to


you'll want to update this description too to match the CLI help

denar90 · 2017-02-16T07:49:22Z

Thanks guys 🎉

feat: configurable page timeout

9da9013

paulirish reviewed Feb 9, 2017

View reviewed changes

brendankenny reviewed Feb 9, 2017

View reviewed changes

feedback

e47b102

patrickhulce force-pushed the page_timeout branch from 2bdfc93 to e47b102 Compare February 9, 2017 01:45

brendankenny mentioned this pull request Feb 9, 2017

tests: use test-specific about:blank timer #1674

Closed

brendankenny requested changes Feb 11, 2017

View reviewed changes

denar90 mentioned this pull request Feb 11, 2017

Release 2.0 paulirish/pwmetrics#55

Closed

12 tasks

more feedback

b87ef4b

paulirish reviewed Feb 14, 2017

View reviewed changes

denar90 mentioned this pull request Feb 14, 2017

Use lighthouse metricsDefinitions paulirish/pwmetrics#50

Merged

ebidel reviewed Feb 14, 2017

View reviewed changes

update max-wait-for-load description

4012ca7

ebidel reviewed Feb 14, 2017

View reviewed changes

ebidel approved these changes Feb 14, 2017

View reviewed changes

update readme text

a5b1cf2

paulirish approved these changes Feb 15, 2017

View reviewed changes

bump yargs

67a85dc

paulirish merged commit 59bc945 into master Feb 16, 2017

paulirish deleted the page_timeout branch February 16, 2017 00:07

		@@ -477,11 +480,15 @@ class Driver {
		const waitForLoad = _options.waitForLoad \|\| false;
		const disableJS = _options.disableJavaScript \|\| false;

feat: configurable page timeout #1672

feat: configurable page timeout #1672

Conversation

patrickhulce commented Feb 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickhulce commented Feb 9, 2017

brendankenny commented Feb 9, 2017

patrickhulce commented Feb 9, 2017

patrickhulce commented Feb 9, 2017

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denar90 commented Feb 16, 2017