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

core(fr): convert js-usage gatherer #12450

Merged
merged 4 commits into from
May 7, 2021
Merged

core(fr): convert js-usage gatherer #12450

merged 4 commits into from
May 7, 2021

Conversation

adamraine
Copy link
Member

Currently split have debugger in sensitive instrumentation and profiler in normal instrumentation. Debugger can pick up on anonymous scripts run by Lighthouse although this gatherer ignores them.

Ref #11313

@adamraine adamraine requested a review from a team as a code owner May 6, 2021 20:07
@adamraine adamraine requested review from patrickhulce and removed request for a team May 6, 2021 20:07
@google-cla google-cla bot added the cla: yes label May 6, 2021
Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nicely done 👍

lighthouse-core/gather/gatherers/js-usage.js Outdated Show resolved Hide resolved
@@ -45,10 +46,20 @@ describe('JsUsage gatherer', () => {
const driver = new Driver(connectionStub);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

somewhat up to you when to tackle this, but we'll eventually need to migrate gathering tests to the new mock driver/sessions in https://github.com/GoogleChrome/lighthouse/blob/0b00b5c667bf40b5945632245bf8e803a9843b40/lighthouse-core/test/fraggle-rock/gather/mock-driver.js like the driver/gather-runner PRs have been doing

I think I agree with the argument for leaving these as-is for now and making a second pass when FR becomes the default, but wanted it on your radar so you're not surprised when the time eventually comes for it :)


/**
* @return {Promise<LH.Artifacts['JsUsage']>}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this artifact is also used by snapshots just to lookup the scriptId -> URL mapping, any thoughts on splitting the artifact or conditionally enabling the profiler to achieve those results?

don't need to do anything extra in this PR, but we should annotate it somehow that there's room for upgrading here (FR-COMPAT or in the inventory sheet in a "notes" column?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we get our scriptId -> URL mappings from the debugger not the profiler? I don't see how we can use the debugger in snapshot mode since we wait for ScriptParsed events. The profiler can give us similar results but will be subject to interference by scriptURL comments.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct it comes from the debugger. With most domains though, you can run Debugger.enable after a page has loaded and still receive many of the relevant events.

For example...

const puppeteer = require('puppeteer')

;(async () => {

const browser = await puppeteer.launch()
const page = await browser.newPage()

await page.goto('https://www.optyx.app', {waitUntil: ['networkidle0']})
await new Promise(r => setTimeout(r, 5000))

const session = await page.target().createCDPSession()
session.on('Debugger.scriptParsed', e => console.log(e.url))
await session.send('Debugger.enable')

await browser.close()
})()

will still log all of the page's javascript

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(that's partly how DevTools is still able to work when you open it after page load ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok. How can we guarantee all the ScriptParsed events are handled by the time we use them in getArtifact?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With most domains though

What determines if the events are emitted or not?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can we guarantee all the ScriptParsed events are handled by the time we use them in getArtifact?

Good question.

  1. Investigate if the domain enable command waits for the historical events to be emitted before returning (I don't know if this is the case, but it'd be cool if it were and I vaguely remember some discussion about this a number of years ago with DevTools. It's possible the discussion was "Patrick: Hey what if it worked like this? DevTools: lulz, no way", but it's possible 😅
  2. Repurpose a "network quiet" style wait period with a timeout.

Either way I don't think you need to solve it in this PR as it's an upgrade, but these are the sort of things we should be noting as we go, so we have a nice handy burndown ready to go when we make our "upgrade" pass over the gatherers :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What determines if the events are emitted or not?

I think it's based on whether Chromium had to keep this data around in memory anyway and/or DevTools making special exceptions for the important open DevTools on existing page use case and still get useful information. I also remember discussing at some point the idea that there'd be a lot more DevTools could do this with if it had a "Developer mode" signal or something, but I'm pretty disconnected from all of that at this point, so I'm not sure what's going on these days in this area.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll make the notes in the conversion doc and add a comment here. I think adding a special case for snapshot would be preferable to splitting this gatherer.

lighthouse-core/gather/gatherers/js-usage.js Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants