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

Inject Lighthouse metrics back into trace for viewing #1446

Merged
merged 4 commits into from
Feb 3, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lighthouse-cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ function runLighthouse(url: string,
// delete artifacts from result so reports won't include artifacts.
const artifacts = results.artifacts;
results.artifacts = undefined;

if (flags.saveArtifacts) {
assetSaver.saveArtifacts(artifacts);
}
Expand Down
5 changes: 5 additions & 0 deletions lighthouse-core/lib/asset-saver.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const fs = require('fs');
const log = require('../../lighthouse-core/lib/log.js');
const stringifySafe = require('json-stringify-safe');
const URL = require('./url-shim');
const Metrics = require('./traces/metrics-evts');

/**
* Generate a filenamePrefix of hostname_YYYY-MM-DD_HH-MM-SS
Expand Down Expand Up @@ -119,6 +120,10 @@ function prepareAssets(artifacts, results) {
const traceData = Object.assign({}, trace);
const html = screenshotDump(screenshots, results);

if (results && results.audits) {
const evts = new Metrics(traceData.traceEvents, results.audits).generateFakeEvents();
traceData.traceEvents.push(...evts);
Copy link
Member

Choose a reason for hiding this comment

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

It seems bad to alter the original trace. Not sure when we did const traceData = Object.assign({}, trace);, but really we should be doing something like traceData.traceEvents = Array.from(traceData.traceEvents) to get a new array. Is that crazy?

Copy link
Member Author

Choose a reason for hiding this comment

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

On one hand, yeah, it feels bad to alter the trace. It'd be nice to keep the objects separate.

On the other:

  • I don't think we have a need to keep around the original trace.
  • We've sorted the trace in-place already (a few times).
  • The memory consumption is pretty huge for these traces, so there's that advantage to augmenting the solo trace.

Copy link
Member

Choose a reason for hiding this comment

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

We've sorted the trace in-place already (a few times).

very good point :)

}
assets.push({
traceData,
html
Expand Down
204 changes: 204 additions & 0 deletions lighthouse-core/lib/traces/metrics-evts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,204 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

rename to indicate progressive web metrics? pwmetrics-events.js? Or is the intention to keep expanding?

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed. sg

* @license
* Copyright 2016 Google Inc. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

2017

*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
'use strict';

const log = require('../../../lighthouse-core/lib/log.js');

class Metrics {
Copy link
Contributor

Choose a reason for hiding this comment

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

throw in a @fileoverview


constructor(traceEvents, auditResults) {
this._traceEvents = traceEvents;
this._auditResults = auditResults;
this._fakeEvents = [];
}

/**
* Returns simplified representation of all metrics
* @return {!Array<{getTs: Function, id: string, name: string}>} metrics to consider
*/
static get metricsDefinitions() {
return [
{
name: 'Navigation Start',
id: 'navstart',
getTs: auditResults => {
const fmpExt = auditResults['first-meaningful-paint'].extendedInfo;
return fmpExt.value.timestamps.navStart;
}
},
{
name: 'First Contentful Paint',
id: 'ttfcp',
getTs: auditResults => {
const fmpExt = auditResults['first-meaningful-paint'].extendedInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

Everything coming from auditResults['first-meaningful-paint'] looks like a mistake. Do we stash several things in auditResults['first-meaningful-paint'].extendedInfo? There are a few others like that, but reading this code is non-obvious.

Copy link
Member Author

Choose a reason for hiding this comment

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

Essentially we have 3 actual audits in lighthouse that collect these metrics. And yeah we stash several key metrics within the extInfo of each of them.
So, yeah, AFAIK all this is correct and intended... any ideas on how it could look less odd?

Copy link
Contributor

Choose a reason for hiding this comment

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

was expecting all the first-* stuff to be together, but I guess that makes sense if they're coming from different audits.

return fmpExt.value.timestamps.fCP;
}
},
{
name: 'First Meaningful Paint',
id: 'ttfmp',
getTs: auditResults => {
const fmpExt = auditResults['first-meaningful-paint'].extendedInfo;
return fmpExt.value.timestamps.fMP;
}
},
{
name: 'Perceptual Speed Index',
id: 'psi',
getTs: auditResults => {
const siExt = auditResults['speed-index-metric'].extendedInfo;
return siExt.value.timestamps.perceptualSpeedIndex;
}
},
{
name: 'First Visual Change',
id: 'fv',
getTs: auditResults => {
const siExt = auditResults['speed-index-metric'].extendedInfo;
return siExt.value.timestamps.firstVisualChange;
}
},
{
name: 'Visually Complete 85%',
id: 'vc85',
getTs: auditResults => {
const siExt = auditResults['time-to-interactive'].extendedInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

auditResults['speed-index-metric']?

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, check that all of these are right. Now I'm really confused :)

Copy link
Member Author

Choose a reason for hiding this comment

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

actually vc85 does actually get delivered on the TTI choo-choo train. :)
that does mean that siExt => ttiExt, so thanks for catching that!

return siExt.value.timestamps.visuallyReady;
}
},
{
name: 'Visually Complete 100%',
id: 'vc',
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest : id: 'vc100' to be like the other

Copy link
Member Author

Choose a reason for hiding this comment

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

sounds good.
I had chosen 'vc' origianlly because WebPageTest uses vc for the same thing. OR SO I THOUGHT!. Looking again... i can't see "vc" anywhere, so yeah i'm happy to tweak the id.

getTs: auditResults => {
const siExt = auditResults['speed-index-metric'].extendedInfo;
return siExt.value.timestamps.visuallyComplete;
}
},
{
name: 'Time to Interactive',
id: 'tti',
getTs: auditResults => {
const ttiExt = auditResults['time-to-interactive'].extendedInfo;
return ttiExt.value.timestamps.timeToInteractive;
}
}
];
}

/**
* Returns simplified representation of all metrics' timestamps from monotonic clock
* @param {!Array<!AuditResult>} auditResults
* @return {!Array<{ts: number, id: string, name: string}>} metrics to consider
*/
gatherMetrics(auditResults) {
Copy link
Member

Choose a reason for hiding this comment

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

make static? (or can use this._auditResults)

const metricDfns = Metrics.metricsDefinitions;
const resolvedMetrics = [];
metricDfns.forEach(metric => {
// try/catch in case auditResults is missing an audit result
Copy link
Contributor

Choose a reason for hiding this comment

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

an -> in

try {
resolvedMetrics.push({
id: metric.id,
name: metric.name,
ts: metric.getTs(auditResults)
});
} catch (e) {
log.error('metrics-evts', `${metric.name} timestamp not found: ${e.message}`);
}
});
return resolvedMetrics;
}

/**
* Getter for our navigationStart trace event
*/
get navigationStartEvt() {
Copy link
Member

Choose a reason for hiding this comment

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

make just getNavigationStartEvt() or something instead of a getter? Kind of confusing below when this property appears.

Could also be an internal function that the constructor calls, populating the this.navigationStartEvt property so the behavior doesn't change from this

if (!this._navigationStartEvt) {
const filteredEvents = this._traceEvents.filter(e => {
return e.name === 'TracingStartedInPage' || e.cat === 'blink.user_timing';
});

const tracingStartedEvt = filteredEvents.filter(e => e.name === 'TracingStartedInPage')[0];
const navigationStartEvt = filteredEvents.filter(e => {
return e.name === 'navigationStart' &&
e.pid === tracingStartedEvt.pid && e.tid === tracingStartedEvt.tid;
})[0];
this._navigationStartEvt = navigationStartEvt;
}
return this._navigationStartEvt;
}


/**
* Constructs performance.measure trace events, which have start/end events as follows:
Copy link
Contributor

Choose a reason for hiding this comment

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

is "ph: b", beginning? If so, maybe say being/end events instead of start/end.

Copy link
Member Author

Choose a reason for hiding this comment

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

actually "b" stands for "nestable start" :D
The trace event phases are kind of insane.

* { "pid": 89922,"tid":1295,"ts":77176783452,"ph":"b","cat":"blink.user_timing","name":"innermeasure","args":{},"tts":1257886,"id":"0xe66c67"}
* { "pid": 89922,"tid":1295,"ts":77176882592,"ph":"e","cat":"blink.user_timing","name":"innermeasure","args":{},"tts":1257898,"id":"0xe66c67"}
* @param {!Object} metric
Copy link
Member

Choose a reason for hiding this comment

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

can do more specific @param {{ts: number, id: string, name: string}} metric from gatherMetrics above

* @param {number} navStartTs
* @return {Array} Pair of trace events (start/end)
Copy link
Member

Choose a reason for hiding this comment

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

{!Array}

*/
synthesizeEventPair(metric, navStartTs) {
if (!metric.ts || metric.id === 'navstart') {
Copy link
Member

Choose a reason for hiding this comment

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

one alternative: could filter these out in the metrics.forEach loop in generateFakeEvents. Either way, should maybe log.error on metric with no ts?

Copy link
Member Author

Choose a reason for hiding this comment

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

sg for both.

return [];
}

// We'll masquerade our fake events to look mostly like navigationStart
const eventBase = {
pid: this.navigationStartEvt.pid,
tid: this.navigationStartEvt.tid,
cat: 'blink.user_timing',
name: metric.name,
args: {},
// randomized id is same for the pair
id: `0x${((Math.random() * 1000000) | 0).toString(16)}`
};
const fakeMeasureStartEvent = Object.assign({}, eventBase, {
ts: navStartTs,
ph: 'b'
});
const fakeMeasureEndEvent = Object.assign({}, eventBase, {
ts: metric.ts,
ph: 'e',
});
return [fakeMeasureStartEvent, fakeMeasureEndEvent];
}

/**
* @returns {Array} User timing raw trace event pairs
Copy link
Member

Choose a reason for hiding this comment

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

@returns {!Array}

*/
generateFakeEvents() {
Copy link
Member

Choose a reason for hiding this comment

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

not sure if you have more plans for this class, but these could all pretty easily be static. Caching this.navigationStartEvt is probably the only real win to having an instance right now, but since you verify that it's equal to the found metric anyway, you could always pass the event into synthesizeEventPair instead of just the timestamp

const metrics = this.gatherMetrics(this._auditResults);
if (metrics.length === 0) {
log.error('metrics-events', 'Metrics collection had errors, not synthetizing trace events');
return [];
}

// confirm our navStart's correctly match
const navStartTs = metrics.find(e => e.id === 'navstart').ts;
if (this.navigationStartEvt.ts !== navStartTs) {
log.error('metrics-evts', 'Reference navigationStart doesn\'t match fMP\'s navStart');
Copy link
Member

Choose a reason for hiding this comment

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

great candidate for online error reporting :)

return [];
}

metrics.forEach(metric => {
log.log('metrics-evts', `Sythesizing trace events for ${metric.name}`);
Copy link
Member

Choose a reason for hiding this comment

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

log.verbose? these are going to flash by incredibly quickly

this._fakeEvents.push(...this.synthesizeEventPair(metric, navStartTs));
Copy link
Member

Choose a reason for hiding this comment

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

any purpose to keeping this as a member variable? If you call generateFakeEvents again you'll get double events

});
return this._fakeEvents;
}
}

module.exports = Metrics;
Loading