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

Use lighthouse metricsDefinitions #50

Merged
merged 13 commits into from
Feb 24, 2017
Merged

Use lighthouse metricsDefinitions #50

merged 13 commits into from
Feb 24, 2017

Conversation

denar90
Copy link
Collaborator

@denar90 denar90 commented Feb 3, 2017

lib/metrics.js Outdated
const resolvedMetric = {
title: metric.name,
name: metric.id,
value: metric.getTs(audits)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Earlier we used e.g. value.timings. fCP or another, but not a timestamp.
@paulirish how do you think better make timestamp conversion ? Can we make it?

Copy link
Owner

Choose a reason for hiding this comment

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

so generally the flow is: get the timestamp for everything and then subtract off navStart. (and i think you'll also have to do a / 1000 to go from microseconds to milliseconds)

and it feels like this sorta changes the { timings, timestamps } return value at the end.
a few ways to structure the data but i'd be okay with something like...

{ 
  metrics: [
    { 
       name: 'Time to interactive',
       id: 'tti',
       timing: 2643.232,
       timestamp: 238932240584
    }, 
    // ... the other metrics...
   ],
   generatedTime: res.generatedTime,
   lighthouseVersion: res.lighthouseVersion,	
   initialUrl: res.initialUrl,
   url: res.url
}

but i'm open for other solutions.

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 reason of using metrics definition here is reducing prepareData.
If we take look at e.g. tti results, it has timings and timestamps

 "time-to-interactive": {
        "score": 100,
        "displayValue": "526.5ms",
        "rawValue": 526.5,
        "optimalValue": "5,000ms",
        "extendedInfo": {
            "value": {
                "timings": {
                    "fMP": 508.2,
                    "visuallyReady": 526.508,
                    "timeToInteractive": 526.508
                },
                "timestamps": {
                    "fMP": 114185594921,
                    "visuallyReady": 114185613239,
                    "timeToInteractive": 114185613239
                },
                "expectedLatencyAtTTI": 16,
                "foundLatencies": [
                    {
                        "estLatency": 16,
                        "startTime": "526.5"
                    }
                ]
            },
            "formatter": "null"
        },
        "name": "time-to-interactive",
        "category": "Performance",
        "description": "Time To Interactive (alpha)",
        "helpText": "Time to Interactive identifies the time at which your app appears to be ready enough to interact with. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/time-to-interactive)."
    },

Right now we can operate this data as we wish, but using API we can't reach timings because it provide us with timestamps, name and id.
What I see here, we can either extend lighthouse API with adding one more method getTimings or leave it as it is right now...

Copy link
Owner

Choose a reason for hiding this comment

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

There could be getMetricTiming method.. but it'd have to take the navStart as well as the metric. something like:

static getMetricTiming(navStart, metric) {
  return (metric.getTs() - navStart.getTs()) / 1000;
}

I'm fine with adding that as a utility method to pwmetrics-events. But also fine with us handling that over here.

Over here we'd tweak things a tad:

const timings = [];
const navStart = metricsDefinitions.find(def => def.id === metricsIds.NAVSTART); 

metricsDefinitions.forEach(metric => {
    try {
      const ts = metric.getTs(audits);
      const resolvedMetric = {
        name: metric.name,
        id: metric.id,
        timestamp: metric.getTs(audits),
        timing: (metric.getTs(audits) - navStart.getTs(audits)) / 1000;
      };
// ...

I favor this second approach a bit. How does that look to you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a dumb thing, forgot about minus navStart 😞
Lets keep it here.
Thanks, it should work perfectly 👍

lib/metrics.js Outdated
const resolvedMetric = {
title: metric.name,
name: metric.id,
value: metric.getTs(audits)
Copy link
Owner

Choose a reason for hiding this comment

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

so generally the flow is: get the timestamp for everything and then subtract off navStart. (and i think you'll also have to do a / 1000 to go from microseconds to milliseconds)

and it feels like this sorta changes the { timings, timestamps } return value at the end.
a few ways to structure the data but i'd be okay with something like...

{ 
  metrics: [
    { 
       name: 'Time to interactive',
       id: 'tti',
       timing: 2643.232,
       timestamp: 238932240584
    }, 
    // ... the other metrics...
   ],
   generatedTime: res.generatedTime,
   lighthouseVersion: res.lighthouseVersion,	
   initialUrl: res.initialUrl,
   url: res.url
}

but i'm open for other solutions.

package.json Outdated
@@ -15,7 +15,7 @@
"charm": "^1.0.2",
"google-auth-library": "^0.10.0",
"googleapis": "^16.0.0",
"lighthouse": "1.4.1",
"lighthouse": "GoogleChrome/lighthouse",
Copy link
Owner

Choose a reason for hiding this comment

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

yeah. we'll ship a release soon. :)

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.

oops. wrong button!

@denar90 denar90 mentioned this pull request Feb 8, 2017
12 tasks
@denar90 denar90 changed the title WIP use lighthouse metricsDefinitions Use lighthouse metricsDefinitions Feb 11, 2017
package.json Outdated
@@ -16,7 +16,7 @@
"charm": "^1.0.2",
"google-auth-library": "^0.10.0",
"googleapis": "^16.0.0",
"lighthouse": "1.4.1",
"lighthouse": "1.5.0",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

🎉

@@ -2,80 +2,80 @@
// Licensed under the Apache License, Version 2.0. See LICENSE
'use strict';

const metricsDefinitions = require('lighthouse/lighthouse-core/lib/traces/pwmetrics-events.js').metricsDefinitions;

const metricsIds = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@paulirish what do you think about making this stuff part of lighthouse?

@denar90
Copy link
Collaborator Author

denar90 commented Feb 11, 2017

Travis failed but tests passed 😛

@denar90
Copy link
Collaborator Author

denar90 commented Feb 12, 2017

Found out where problem is (why CI is failing). Looks like audits results are not provided (extendedInfo in undefined). It happens only on new browser, if it's already opened -there are no problem.

@paulirish should I make some trace run and report to lighthouse?

Here some log of audits result

{ 'first-meaningful-paint': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: Cannot read property \'ts\' of undefined',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'first-meaningful-paint',
     category: 'Performance',
     description: 'First meaningful paint',
     helpText: 'First meaningful paint measures when the primary content of a page is visible. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-meaningful-paint).' },
  'speed-index-metric': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: No screenshots found in trace',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'speed-index-metric',
     category: 'Performance',
     description: 'Perceptual Speed Index',
     helpText: 'Speed Index shows how quickly the contents of a page are visibly populated. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/speed-index).' },
  'estimated-input-latency': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: No screenshots found in trace',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'estimated-input-latency',
     category: 'Performance',
     description: 'Estimated Input Latency',
     helpText: 'The score above is an estimate of how long your app takes to respond to user input, in milliseconds. There is a 90% probability that a user encounters this amount of latency, or less. 10% of the time a user can expect additional latency. If your score is higher than Lighthouse\'s target score, users may perceive your app as laggy. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/estimated-input-latency).' },
  'time-to-interactive': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: Cannot read property \'ts\' of undefined',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'time-to-interactive',
     category: 'Performance',
     description: 'Time To Interactive (alpha)',
     helpText: 'Time to Interactive identifies the time at which your app appears to be ready enough to interact with. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/time-to-interactive).' },
  'user-timings': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: Cannot read property \'ts\' of undefined',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'user-timings',
     category: 'Performance',
     description: 'User Timing marks and measures',
     helpText: 'Consider instrumenting your app with the User Timing API to create custom, real-world measurements of key user experiences. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/user-timing).' },
  screenshots: 
   { score: 0,
     displayValue: '',
     rawValue: 0,
     error: undefined,
     debugString: undefined,
     optimalValue: undefined,
     extendedInfo: { formatter: 'null', value: [] },
     name: 'screenshots',
     category: 'Performance',
     description: 'Screenshots of all captured frames',
     helpText: undefined },
  'critical-request-chains': 
   { score: false,
     displayValue: '5',
     rawValue: false,
     error: undefined,
     debugString: undefined,
     optimalValue: 0,
     extendedInfo: { formatter: 'criticalRequestChains', value: [Object] },
     name: 'critical-request-chains',
     category: 'Performance',
     description: 'Critical Request Chains',
     helpText: 'The Critical Request Chains below show you what resources are required for first render of this page. Improve page load by reducing the length of chains, reducing the download size of resources, or deferring the download of unnecessary resources. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/critical-request-chains).' },
  'unused-css-rules': 
   { score: false,
     displayValue: '29KB (~140ms) potential savings',
     rawValue: false,
     error: undefined,
     debugString: undefined,
     optimalValue: undefined,
     extendedInfo: { formatter: 'table', value: [Object] },
     name: 'unused-css-rules',
     category: 'CSS',
     description: 'Uses 90% of its CSS rules',
     helpText: 'Remove unused rules from stylesheets to reduce unnecessary bytes consumed by network activity. [Learn more](https://developers.google.com/speed/docs/insights/OptimizeCSSDelivery)' },
  'link-blocking-first-paint': 
   { score: false,
     displayValue: '2 resources delayed first paint by 641ms',
     rawValue: false,
     error: undefined,
     debugString: undefined,
     optimalValue: undefined,
     extendedInfo: { formatter: 'table', value: [Object] },
     name: 'link-blocking-first-paint',
     category: 'Performance',
     description: 'Avoids `<link>` that delay first paint',
     helpText: 'Link elements are blocking the first paint of your page. Consider inlining critical links and deferring non-critical ones. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/blocking-resources).' },
  'script-blocking-first-paint': 
   { score: false,
     displayValue: '3 resources delayed first paint by 1755ms',
     rawValue: false,
     error: undefined,
     debugString: undefined,
     optimalValue: undefined,
     extendedInfo: { formatter: 'table', value: [Object] },
     name: 'script-blocking-first-paint',
     category: 'Performance',
     description: 'Avoids `<script>` in head that delay first paint',
     helpText: 'Script elements are blocking the first paint of your page. Consider inlining critical scripts and deferring non-critical ones. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/blocking-resources).' },
  'uses-optimized-images': 
   { score: true,
     displayValue: '',
     rawValue: true,
     error: undefined,
     debugString: undefined,
     optimalValue: undefined,
     extendedInfo: { formatter: 'table', value: [Object] },
     name: 'uses-optimized-images',
     category: 'Images',
     description: 'Has optimized images',
     helpText: 'Images should be optimized to save network bytes. The following images could have smaller file sizes when compressed with [WebP](https://developers.google.com/speed/webp/) or JPEG at 80 quality. [Learn more about image optimization](https://developers.google.com/web/fundamentals/performance/optimizing-content-efficiency/image-optimization).' },
  'uses-responsive-images': 
   { score: true,
     displayValue: '',
     rawValue: true,
     error: undefined,
     debugString: undefined,
     optimalValue: undefined,
     extendedInfo: { formatter: 'table', value: [Object] },
     name: 'uses-responsive-images',
     category: 'Images',
     description: 'Has appropriately sized images',
     helpText: 'Image sizes served should be based on the device display size to save network bytes. Learn more about [responsive images](https://developers.google.com/web/fundamentals/design-and-ui/media/images) and [client hints](https://developers.google.com/web/updates/2015/09/automating-resource-selection-with-client-hints).' } }

@pedro93
Copy link
Collaborator

pedro93 commented Feb 14, 2017

I've solved the minor branch conflits. @denar90 Whats the status on the CI tests? Shall we try to fix upstream in lighthouse?

@denar90
Copy link
Collaborator Author

denar90 commented Feb 14, 2017

I think this problem related to #31 and #28
It will be fixed in GoogleChrome/lighthouse#1672

We should handle better extendedInfo emptiness. I think we can add error message which will say smth like
No info was got for first meaningful paint. Try to use "--max-wait-for-load" flag to increase timeout in milliseconds to wait for the page to load before continuing.

@paulirish
Copy link
Owner

@denar90 are you using lighthouse from master? You should update it. The "Cannot read property 'ts' of undefined" error i wouldn't be expecting from lighthouse any longer.

@paulirish
Copy link
Owner

also the debugString properties currently carry whatever reason there's a problem, so we could output that. max-wait-for-load might help but only in 20% of cases, i'd expect. (Happy to communicate it anyway)

@paulirish
Copy link
Owner

are you using lighthouse from master?

ah, this PR says its lighthouse 1.5.0 which should be fine (though a typo in package.json). can you verify you have 1.5.0 in node_modules and still get debugStrings like the above?

package.json Outdated
"lighthouse": "1.4.1",
"median": "^0.0.2",
"lighthouse": "1.5.0",
"median": "^0.0.2"
Copy link
Owner

Choose a reason for hiding this comment

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

missing a comma

@denar90
Copy link
Collaborator Author

denar90 commented Feb 15, 2017

Here you are 😃

node node_modules/lighthouse/lighthouse-cli/bin.js --version => 1.5.0

Console of audits

Launching Chrome
{ 'first-meaningful-paint': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: Cannot read property \'ts\' of undefined',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'first-meaningful-paint',
     category: 'Performance',
     description: 'First meaningful paint',
     helpText: 'First meaningful paint measures when the primary content of a page is visible. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/first-meaningful-paint).' },
  'speed-index-metric': 
   { score: null,
     displayValue: '',
     rawValue: null,
     error: true,
     debugString: 'Audit error: No screenshots found in trace',
     optimalValue: undefined,
     extendedInfo: undefined,
     name: 'speed-index-metric',
     category: 'Performance',
     description: 'Perceptual Speed Index',
     helpText: 'Speed Index shows how quickly the contents of a page are visibly populated. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/speed-index).' },
...

@@ -28,7 +28,6 @@ gulp.task('pwmetrics', function() {
expectations: true
},
expectations: {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Are we ok to use just ?

expectations: {
...
}

instead of

expectations: {
  metrics: {
  ...
  }
}

@pedro93
Copy link
Collaborator

pedro93 commented Feb 15, 2017

That would match the config file structure, just check that the pwmetrics constructor no longer expectations.metrics

@denar90
Copy link
Collaborator Author

denar90 commented Feb 16, 2017

@pedro93 but it originally was there, so I'm just bothering if @paulirish ok with current expectations config structure...

@paulirish
Copy link
Owner

@denar90 yeah i'm fine with breaking API for this.

@denar90
Copy link
Collaborator Author

denar90 commented Feb 16, 2017

@denar90 are you using lighthouse from master? You should update it. The "Cannot read property 'ts' of undefined" error i wouldn't be expecting from lighthouse any longer.

@paulirish regarding to that. This problem appears with more then 1 run. I've debugged a bit and caught exception here
https://github.com/GoogleChrome/lighthouse/blob/1.5.0/lighthouse-core/gather/computed/trace-of-tab.js#L61

Looks like frameEvents don't have firstPaint at all.
btw chrome version where all stuff is run Version 58.0.3014.0 canary (64-bit)

[
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791076878,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "navigationStart",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 557647
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791077306,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "fetchStart",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 557990
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791362583,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "responseEnd",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 813318
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791366821,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "unloadEventStart",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 776881
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791367044,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "unloadEventEnd",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 777097
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791370556,
        "ph": "R",
        "cat": "blink.user_timing,rail",
        "name": "domLoading",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 780602
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791413821,
        "ph": "R",
        "cat": "blink.user_timing,rail",
        "name": "domInteractive",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 820908
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791414274,
        "ph": "R",
        "cat": "blink.user_timing,rail",
        "name": "domContentLoadedEventStart",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 821361
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791414304,
        "ph": "R",
        "cat": "blink.user_timing,rail",
        "name": "domContentLoadedEventEnd",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 821391
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791418752,
        "ph": "R",
        "cat": "blink.user_timing,rail",
        "name": "domComplete",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 825804
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791419220,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "loadEventStart",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 826273
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791419240,
        "ph": "R",
        "cat": "blink.user_timing",
        "name": "loadEventEnd",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 826293
    },
    {
        "pid": 98304,
        "tid": 775,
        "ts": 426791560882,
        "ph": "R",
        "cat": "blink.user_timing,rail",
        "name": "firstLayout",
        "args": {
            "frame": "0x1fcc23cc1de8"
        },
        "tts": 965655
    }
]

@paulirish
Copy link
Owner

@denar90 ah that's super useful. thanks for debugging it. :)

are you using chrome headless? what OS is this? and can you share the URL?

@denar90
Copy link
Collaborator Author

denar90 commented Feb 16, 2017

I have canary (v58.0.3014.0) and chrome itself (v56.0.2924.87).
osx el capitan 10.11.5 (yeap I know that it should be upgraded 😄 )

and can you share the URL?

which one? (sorry)

http://example.com - our most lovely 😃

@denar90
Copy link
Collaborator Author

denar90 commented Feb 16, 2017

Don't hesitate to ask either additional info or additional debug or whatever :)

@paulirish
Copy link
Owner

This PR lgtm.

Travis is failing based on the expectations. Maybe those should be relaxed?
Also the new expectations in the gulp recipe seem too aggressive. I'd rather see those like the other.

feel free to merge when you sort out the travis bit. :)

@denar90
Copy link
Collaborator Author

denar90 commented Feb 23, 2017

Thx 👍

@denar90
Copy link
Collaborator Author

denar90 commented Feb 24, 2017

I'm merging this one. Looks like travis still failing but #42 should fix it. One more thing. I wanna run recipes as separate build from tests.

@denar90 denar90 merged commit 56c1387 into master Feb 24, 2017
@denar90
Copy link
Collaborator Author

denar90 commented Feb 24, 2017

Looks like I was wrong. After merging PR into master, I merged cri-timeout into current branch. But it looks not fixed... 😞 Tomorrow will look at it(
https://travis-ci.org/paulirish/pwmetrics/jobs/204810095

@paulirish
Copy link
Owner

paulirish commented Feb 24, 2017 via email

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.

3 participants