core(runner): independent gather and audit functions by adamraine · Pull Request #13569 · GoogleChrome/lighthouse

adamraine · 2022-01-14T22:11:28Z

Step 2 of #13364

Splits Runner.run into two functions, Runner.gatherPhase and Runner.auditPhase. Right now, these two functions are always used together, but step 3 of #13364 will start using these functions separately in use flows.

Ref
#11313

brendankenny

excited for this, though less so for this new Runner() interface :)

I was a fan of the

/**
 * @template {LH.Config.Config | LH.Config.FRConfig} TConfig
 * @param {(runnerData: {config: TConfig, driverMock?: Driver}) => Promise<LH.Artifacts>} gatherFn
 * @param {{config: TConfig, driverMock?: Driver}} options
 * @return {Promise<LH.Artifacts>}
 */
static async gather(gatherFn, options) {}

/**
 * @template {LH.Config.Config | LH.Config.FRConfig} TConfig
 * @param {LH.Artifacts} artifacts
 * @param {{config: TConfig, computedCache: Map<string, ArbitraryEqualityMap>}} options
 * @return {Promise<LH.RunnerResult|undefined>}
 */
static async audit(artifacts, options) {}

approach

This reverts commit cd390a4.

adamraine · 2022-01-18T20:31:07Z

+    const gatherEntry = timingEntries.find(e => e.name === 'lh:runner:gather');
+    const auditEntry = timingEntries.find(e => e.name === 'lh:runner:audit');
+    const gatherTiming = gatherEntry?.duration || 0;
+    const auditTiming = auditEntry?.duration || 0;
+    return {entries: timingEntries, total: gatherTiming + auditTiming};


This might encounter issues in a flows as we move forward with #13364. I don't know if a flow step is guaranteed to query the correct timing entry from the logger.

@brendankenny this might be a reason to use a Runner instance instead. We could store timing information on that instance instead of querying the logger.

It's not causing any issues right now because log.takeTimingEntries clears everything between each flow step. I'll investigate this in the next part. There's probably a way to remember timings for each step without adding state to Runner.

FWIW it's not clear what total should mean going forward. Right now it's the total of the LH run that just completed, so if run with -A, the -G timings are in the entries array, but not included in the total. That would be changed here, and I kind of prefer the old way, but I'm not sure if the total timing of -A runs is important enough to care (if e.g. you're trying to optimizing something, you're likely looking at some other timing or tool).

In a user flow, is the amount of time to gather and audit a particular step the most meaningful interpretation of "total"? Seems ok.

Should add some total tests if we're making a decision.

Also it's worth pointing out that total was a placeholder while we figured out entries and only kept it around because it was kind of useful after entries landed, so the only bar to clear here is "kind of useful" :)

…house into fr-defer-audit-runner

brendankenny · 2022-01-20T20:45:16Z


    it('specifies the channel as lr', async () => {
-      const runStub = jest.spyOn(Runner, 'run');
+      const runStub = jest.spyOn(Runner, 'gather');


not that these spy tests are the best, but seems like these should be on the higher-level lighthouse() function now (from lighthouse-core/index.js)

One of the tests verifies that the config.settings.channel is set to lr before config reaches Runner. I think there is still value in testing Runner.gather rather than lighthouse().

brendankenny · 2022-01-20T21:33:16Z

+
+      const sentryContext = Sentry.getContext();
+      Sentry.captureBreadcrumb({
+        message: 'Run started',


this came up in #13519 (comment). With an independent gather and audit, there's no guarantee that this is the start of the run (or even called at all...is there any reason for snapshot to call Runner.gather()?).

Moving it to lighthouse-core/index.js makes sense for classic Lighthouse (that's what #12393 did), but that doesn't help the FR runners. This (and timings and errors) are the same orchestrator issue. Honestly all the runners make their own config, maybe they should all be doing their own copy of this kind of setup as well (or runner-helpers.js or whatever)

FWIW the FR runners should call Runner.gather every time, but there is no guarantee that the FR runners themselves will be run. The only question is what happens in user-flow.js. Should we consider each flow step an independent "run" from the sentry perspective, or should the entire flow be a single "run".

I thought from #13364 that they wouldn't be calling into gather() anymore, since it's kind of redundant (they already define their own gather function)? With multiple gathers, at least, -G and -A will have to be tweaked to work.

I thought from #13364 that they wouldn't be calling into gather() anymore

Using Runner.gather() in the FR runners is still useful because it gives us the timing entry, error handling, and the sentry call. As you said, we could add those things to each FR runner.

I did update #13364 to reflect that but didn't announce the change sorry.

With multiple gathers, at least, -G and -A will have to be tweaked to work.

This is true, but we don't need to get rid of the logic entirely if the other stuff in gather() is still useful. The issue should only be a problem for user flows which will have it's own save/load artifacts system.

brendankenny · 2022-01-20T21:33:57Z

-      }
-      await Sentry.captureException(err, {level: 'fatal'});
-      throw err;
+      throw Runner.createRunnerError(err, settings);


I believe there is a test that gathering errors are localized but probably want one on audit() now as well

I can't find a place where a "friendly" error makes it all the way up here. They should all be caught be the catch block in _runAudit.

brendankenny · 2022-01-20T21:57:22Z

+      // If `gather` is run multiple times before `audit`, the timing entries for each `gather` can pollute one another.
+      // Timing entries are stored in artifacts.Timing, so we can clear the timing entries here.
+      log.takeTimeEntries();


This behavior was intentional because takeTimeEntries also clears out any ongoing measures (like the old lh:runner:run which included gather+audit), so they wouldn't be included at the end of the run. Multiple gather steps do make for a problem if they're going to be turned into LHRs individually, though.

If there aren't going to be any measures that go over the entire run, then this works, but then the base artifact finalization should be able to call it directly.

If there aren't going to be any measures that go over the entire run, then this works, but then the base artifact finalization should be able to call it directly.

I tried that at first, it clears out the marker used by the log.timeEnd right above this. Had to clear the timings after it.

isn't it cleared out regardless by this call since log.takeTimeEntries() clears them and we don't retain the returned value?

Sorry, it clears lh:runner:gather not lh:runner:audit before its log.timeEnd is called.

That's what I meant as well, I think. That's the change in sample_v2 in the CI basics failure?

sorry you had to step into the mess that is collecting our timings inside the thing being timed for this PR (ask @connorjclark how the last time he made a change to them went...).

Yeah I'm currently trying to unravel the CI failure. Long story short I don't think this is the correct approach anymore :)

brendankenny · 2022-01-20T22:07:26Z

+    const gatherEntry = timingEntries.find(e => e.name === 'lh:runner:gather');
+    const auditEntry = timingEntries.find(e => e.name === 'lh:runner:audit');
+    const gatherTiming = gatherEntry?.duration || 0;
+    const auditTiming = auditEntry?.duration || 0;
+    return {entries: timingEntries, total: gatherTiming + auditTiming};


FWIW it's not clear what total should mean going forward. Right now it's the total of the LH run that just completed, so if run with -A, the -G timings are in the entries array, but not included in the total. That would be changed here, and I kind of prefer the old way, but I'm not sure if the total timing of -A runs is important enough to care (if e.g. you're trying to optimizing something, you're likely looking at some other timing or tool).

In a user flow, is the amount of time to gather and audit a particular step the most meaningful interpretation of "total"? Seems ok.

Should add some total tests if we're making a decision.

Also it's worth pointing out that total was a placeholder while we figured out entries and only kept it around because it was kind of useful after entries landed, so the only bar to clear here is "kind of useful" :)

paulirish

looks nice

core(runner): independent gather and audit functions

627356d

adamraine commented Jan 14, 2022

View reviewed changes

Comment thread lighthouse-core/fraggle-rock/gather/navigation-runner.js Outdated

+ runner state

cd390a4

vercel Bot deployed to Preview January 14, 2022 22:37 View deployment

brendankenny reviewed Jan 14, 2022

View reviewed changes

Comment thread lighthouse-core/fraggle-rock/gather/navigation-runner.js Outdated

Comment thread lighthouse-core/fraggle-rock/gather/navigation-runner.js Outdated

adamraine added 2 commits January 18, 2022 14:02

Revert "+ runner state"

90e31dc

This reverts commit cd390a4.

rn

f7f206d

vercel Bot deployed to Preview January 18, 2022 19:08 View deployment

shared opts

57935d6

vercel Bot deployed to Preview January 18, 2022 19:18 View deployment

adamraine added 2 commits January 18, 2022 14:20

log msg

34c1a02

fix tests

e631034

vercel Bot deployed to Preview January 18, 2022 20:25 View deployment

adamraine commented Jan 18, 2022

View reviewed changes

runner

2e01dfa

vercel Bot deployed to Preview January 18, 2022 20:54 View deployment

lr test

b9fffaa

vercel Bot deployed to Preview January 18, 2022 21:02 View deployment

sample

1f22fa6

vercel Bot deployed to Preview January 18, 2022 22:57 View deployment

Merge branch 'master' into fr-defer-audit-runner

e868c3e

vercel Bot deployed to Preview January 19, 2022 22:13 View deployment

adamraine marked this pull request as ready for review January 19, 2022 23:01

adamraine requested a review from a team as a code owner January 19, 2022 23:01

adamraine requested review from connorjclark and removed request for a team January 19, 2022 23:01

devtools-bot assigned connorjclark Jan 19, 2022

devtools-bot added the waiting4reviewer label Jan 19, 2022

connorjclark requested changes Jan 19, 2022

View reviewed changes

Comment thread lighthouse-core/runner.js Outdated

Comment thread lighthouse-core/runner.js Outdated

Comment thread lighthouse-core/runner.js Outdated

Comment thread lighthouse-core/test/runner-test.js Outdated

comments

e54eabb

adamraine added 3 commits January 20, 2022 10:28

Merge branch 'fr-defer-audit-runner' of github.com:GoogleChrome/light…

e4a2ca2

…house into fr-defer-audit-runner

sync

291ae9a

new func

69d5f98

vercel Bot deployed to Preview January 20, 2022 15:36 View deployment

adamraine mentioned this pull request Jan 20, 2022

Gather/Audit Mode in User Flows #13364

Closed

6 tasks

connorjclark approved these changes Jan 20, 2022

View reviewed changes

brendankenny reviewed Jan 20, 2022

View reviewed changes

Comment thread lighthouse-core/test/runner-test.js

timings

1757426

vercel Bot deployed to Preview January 20, 2022 21:38 View deployment

brendankenny reviewed Jan 20, 2022

View reviewed changes

paulirish reviewed Jan 21, 2022

View reviewed changes

adamraine mentioned this pull request Jan 24, 2022

core(runner): store gather timing on artifacts #13587

Merged

Merge branch 'master' into fr-defer-audit-runner

ec46509

vercel Bot deployed to Preview January 25, 2022 21:34 View deployment

core(runner): store gather timing on artifacts (#13587)

900f948

vercel Bot deployed to Preview February 1, 2022 19:52 View deployment

adamraine mentioned this pull request Feb 1, 2022

tests: timing smoke test #13614

Merged

Merge branch 'master' into fr-defer-audit-runner

7c0d9e2

vercel Bot deployed to Preview February 1, 2022 20:07 View deployment

Merge branch 'master' into fr-defer-audit-runner

1b05363

vercel Bot deployed to Preview February 1, 2022 21:22 View deployment

tests: timing smoke test (#13614)

7eec630

vercel Bot deployed to Preview February 2, 2022 20:05 View deployment

connorjclark reviewed Feb 2, 2022

View reviewed changes

Comment thread lighthouse-cli/test/smokehouse/report-assert.js

adamraine mentioned this pull request Feb 2, 2022

tests(smoke): test array _includes and lhr.timing #13619

Merged

Merge branch 'master' into fr-defer-audit-runner

6c961ee

vercel Bot deployed to Preview February 3, 2022 15:46 View deployment

adamraine merged commit bbe05ad into master Feb 3, 2022

adamraine deleted the fr-defer-audit-runner branch February 3, 2022 21:51

Conversation

adamraine commented Jan 14, 2022

Uh oh!

Uh oh!

brendankenny left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adamraine Jan 18, 2022 • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brendankenny Jan 20, 2022 • 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!

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!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adamraine Jan 18, 2022 •

edited

Loading

brendankenny Jan 20, 2022 •

edited

Loading