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

LHv2 #120

Merged
merged 22 commits into from
Jun 20, 2017
Merged

LHv2 #120

merged 22 commits into from
Jun 20, 2017

Conversation

denar90
Copy link
Collaborator

@denar90 denar90 commented May 16, 2017

I've done some experiments with hiding TTI, showing TTFI and TTCI. Also integrated stuff with updated chrome launcher

Trace of nytimes
image

Stil can't make yarn and my fork works, have to manually build stuff in chrome launcher folder.

Also, we have to update the link to new sheets in doc example because of new metrics.

lib/index.ts Outdated
});
this.flags.port = this.launcher.port;
} catch(error) {
return error;
Copy link
Owner

Choose a reason for hiding this comment

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

probably want to do this.launcher.kill() in the catch

Copy link
Collaborator

Choose a reason for hiding this comment

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

+1

lib/index.ts Outdated
this.launcher = await launch({
port: this.flags.port,
chromeFlags: this.flags.chromeFlags,
handleSIGINT: true
Copy link
Owner

Choose a reason for hiding this comment

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

this one is the default so you can omit it.

package.json Outdated
@@ -21,7 +21,7 @@
"charm": "^1.0.2",
"google-auth-library": "^0.10.0",
"googleapis": "^16.0.0",
"lighthouse": "paulirish/lighthouse#c74ea959",
"lighthouse": "denar90/lighthouse#14cd4f5",
Copy link
Owner

Choose a reason for hiding this comment

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

you can do

"lighthouse": "canary",

or

"lighthouse": "2.0.0-alpha.6",

Choose a reason for hiding this comment

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

👍

@@ -28,15 +26,14 @@ class PWMetrics {
view: false,
expectations: false,
output: false,
// @todo remove when new lighthouse version be released, because -https://github.com/GoogleChrome/lighthouse/pull/1778
Copy link
Collaborator

Choose a reason for hiding this comment

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

@paulirish Will there be no way to disable cpu throttling in lighthouse v2?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The flag still present but
In LH v1 disableCpuThrottling is set up as true by default, we wanted it to be false.
In LH v2 disableCpuThrottling is false by default.

Copy link
Owner

Choose a reason for hiding this comment

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

yes what @denar90 said. we now have cpu throttling on as a default
and yeah you can easily disable that.

@denar90
Copy link
Collaborator Author

denar90 commented May 17, 2017

Only thing is left is error after running yarn

> pwmetrics@2.0.2 build /home/travis/build/paulirish/pwmetrics
> tsc

node_modules/lighthouse/chrome-launcher/utils.ts(22,25): error TS7016: Could not find a declaration file for module 'mkdirp'. '/home/travis/build/paulirish/pwmetrics/node_modules/mkdirp/index.js' implicitly has an 'any' type.

I still have to do cd node_modules/lighthouse/chrome-launcher && yarn then it works...

@pedro93
Copy link
Collaborator

pedro93 commented May 17, 2017

@paulirish @samccone Can this not be fixed by a postinstall step on lighthouse? I think it makes sense to have for the chrome-launcher and lighthouse-core modules (assuming we need any typescript files)

@paulirish
Copy link
Owner

I still have to do cd node_modules/lighthouse/chrome-launcher && yarn then it works...

yes. good point. noted that here: GoogleChrome/lighthouse#2251 (comment)

agree that it's kinda broken for typescript consuming apps right now.
cc @samccone

pedro93 and others added 5 commits June 13, 2017 19:20
Adds chrome-launcher
Bumps lighthouse version to v2.1
Updates lighthouse's version to v2.1
Adds chrome-launcher entry
WIP still does not work.
Adds chrome-launcher typing to Pwmetrics.launcher property and methods for type correctness.
lib/index.ts Outdated
await this.launcher.run();
return this.launcher;
return this.launcher = await launch({
port: this.flags.port,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this.flags.port is never set is it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

do we have to set default port? I thought chrome launcher has to take care of it...

Copy link
Collaborator

@pedro93 pedro93 Jun 14, 2017

Choose a reason for hiding this comment

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

I'm not sure, that particular line was there from the original PR code, there was a cyclic dependency on ports. I'll take a look soon but from the chrome-launcher docs it should use a default port.

Copy link
Collaborator

Choose a reason for hiding this comment

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

no need to set a default port :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tried it locally but I get this:

Error: connect ECONNREFUSED 127.0.0.1:9222
    at Object.exports._errnoException (util.js:1026:11)
    at exports._exceptionWithHostPort (util.js:1049:20)
    at TCPConnectWrap.afterConnect [as oncomplete] (net.js:1136:14)

Chrome opens a window with a tab but nothing happens.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1 faced the same issue. Using some other port - works perfectly.
pwmetrics http://example.com --port=8080

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you use a dynamic port (no port flag) you need to use the port value that is passed back from launcher to connect with.

I would be you or lighthouse has 9222 hard coded somewhere that you are not overriding.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes we missed that, I added the port to lighthouse. I think it's best let chrome-launcher find an available port and use that to prevent port-in-use errors. @samccone Lighthouse is port-agnostic too right? It should work on any available port correct?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@samccone thx, btw wdyt about @pedro93 idea?

Copy link
Owner

Choose a reason for hiding this comment

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

@pedro93 yup chrome-launcher automatically finds an available port and uses that. LH is port agnostic yup.

what's on the branch now is how i'd do it!

pedro93 and others added 3 commits June 14, 2017 16:21
Save chrome-launcher auto-generated ports to utilize it when starting lighthouse.
Copy link
Owner

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

I think this is good to go.

@denar90
Copy link
Collaborator Author

denar90 commented Jun 17, 2017

@pedro93
Copy link
Collaborator

pedro93 commented Jun 18, 2017

Because lighthouse v2 no longer has the TTI metric we need to pick a new to calculate the median run. See https://github.com/denar90/pwmetrics/blob/8519e2713987d5ed24af906e3a28fc71e0d421ef/lib/index.ts#L248 and https://github.com/denar90/pwmetrics/blob/8519e2713987d5ed24af906e3a28fc71e0d421ef/lib/index.ts#L253 I've just run into this problem. @paulirish What is the closest metric to TTI now? TTFI? Should we use an older more stable metric instead?

Replaces removed (with lighthouse v2) TTI metric as the metric to obtain the median run. 
WARNING Uses First Interactive (vBeta) instead, this metric may be unstable.
@pedro93
Copy link
Collaborator

pedro93 commented Jun 18, 2017

Running pwmetrics on https://github.com gives out the following error:

  ✘ Error: Unable to complete run 1 of 1 due to Audit error: The main thread was busy for the entire trace recording. First Interactive requires the main thread to be idle for several seconds. Audit key: first-interactive.

Using lighthouse version 2.1.0 installed globally via npm, with node v.8.0.0 this metric is correctly obtained. I don't think we should merge until we find out why we get this error.

Copy link
Collaborator

@pedro93 pedro93 left a comment

Choose a reason for hiding this comment

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

test-runner.sh must be updated to work with node versions 7 and 8. Tests that use the TTI metric must be fixed to use the new TTFI metric instead. Perhaps v6 too since thats what travisCI is using, or update travis to only test on node v7.
I agree with @denar90 that https://github.com/paulirish/pwmetrics/blob/master/lib/metrics.ts#L25 must be updated.

@denar90 denar90 changed the title LHv2 wip LHv2 Jun 19, 2017
@denar90
Copy link
Collaborator Author

denar90 commented Jun 19, 2017

Guys, I think we are ready to ship this one 🚢

@pedro93
Copy link
Collaborator

pedro93 commented Jun 19, 2017

Be my guest @denar90 :)

@pedro93
Copy link
Collaborator

pedro93 commented Jun 19, 2017

Should this be pwmetrics v3?

@paulirish
Copy link
Owner

paulirish commented Jun 19, 2017 via email

@denar90
Copy link
Collaborator Author

denar90 commented Jun 20, 2017

Sorry, no squash.

@denar90 denar90 merged commit 5ed7d0f into paulirish:master Jun 20, 2017
@denar90
Copy link
Collaborator Author

denar90 commented Jun 20, 2017

Aaaaand published 💥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants