Skip to content

Commit

Permalink
feat: add error reporting
Browse files Browse the repository at this point in the history
  • Loading branch information
patrickhulce committed Jun 20, 2017
1 parent fbd724e commit 528286d
Show file tree
Hide file tree
Showing 15 changed files with 422 additions and 25 deletions.
36 changes: 36 additions & 0 deletions CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,42 @@ Don't:
If no reference doc exists yet, then you can use the `helpText` as a stopgap for explaining
both why the audit is important and how to fix it.

## Tracking Errors

We track our errors in the wild with Sentry. In general, do not worry about wrapping your audits or gatherers in try/catch blocks and reporting every error that could possibly occur; `lighthouse-core/runner.js` and `lighthouse-core/gather/gather-runner.js` already catch and report any errors that occur while running a gatherer or audit, including errors fatal to the entire run. However, there are some situations when you might want to expliticly handle an error and report it to Sentry or wrap it to avoid reporting. Generally, you can interact with Sentry simply by requiring the `lighthouse-core/lib/sentry.js` file and call its methods. The module exports a delegate that will correctly handle the error reporting based on the user's opt-in preference and will simply no-op if they haven't so you don't need to check.


#### If you have an expected error that is recoverable but want to track how frequently it happens, *use Sentry.captureException*.

```js
const Sentry = require('./lighthouse-core/lib/sentry');

try {
doRiskyThing();
} catch (err) {
Sentry.captureException(err, {
tags: {audit: 'audit-name'},
level: 'warning',
});
doFallbackThing();
}
```

#### If you need to track a code path that doesn't necessarily mean an error occurred, *use Sentry.captureMessage*.

NOTE: If the message you're capturing is dynamic/based on user data or you need a stack trace, then create a fake error instead and use `Sentry.captureException` so that the instances will be grouped together in Sentry.

```js
const Sentry = require('./lighthouse-core/lib/sentry');

if (networkRecords.length === 1) {
Sentry.captureMessage('Site only had 1 network request');
return null;
} else {
// do your thang
}
```

# For Maintainers

## Release guide
Expand Down
15 changes: 14 additions & 1 deletion lighthouse-cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import * as path from 'path';
const perfOnlyConfig = require('../lighthouse-core/config/perf.json');
const performanceXServer = require('./performance-experiment/server');
import * as Printer from './printer';
import {askPermission, isDev} from './sentry-prompt';
import {Results} from './types/types';
const pkg = require('../package.json');

Expand Down Expand Up @@ -164,10 +165,22 @@ export async function runLighthouse(
url: string, flags: Flags, config: Object|null): Promise<{}|void> {
let launchedChrome: LaunchedChrome|undefined;

if (flags.enableErrorReporting) {
const isErrorReportingEnabled = await askPermission();
flags.enableErrorReporting = isErrorReportingEnabled;
}

try {
launchedChrome = await getDebuggableChrome(flags);
flags.port = launchedChrome.port;
const results = await lighthouse(url, flags, config);
const results = await lighthouse(url, flags, config, {
name: 'redacted', // prevent sentry from using hostname
environment: isDev() ? 'development' : 'production',
release: pkg.version,
tags: {
channel: 'cli',
},
});

const artifacts = results.artifacts;
delete results.artifacts;
Expand Down
4 changes: 3 additions & 1 deletion lighthouse-cli/cli-flags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import {GetValidOutputOptions, OutputMode} from './printer';
export interface Flags {
skipAutolaunch: boolean, port: number, selectChrome: boolean, chromeFlags: string, output: any,
outputPath: string, interactive: boolean, saveArtifacts: boolean, saveAssets: boolean,
view: boolean, maxWaitForLoad: number
view: boolean, maxWaitForLoad: number, enableErrorReporting: boolean
}

export function getFlags() {
Expand Down Expand Up @@ -56,6 +56,7 @@ export function getFlags() {
],
'Configuration:')
.describe({
'enable-error-reporting': 'Enables error reporting (on by default)',
'disable-storage-reset':
'Disable clearing the browser cache and other storage APIs before a run',
'disable-device-emulation': 'Disable Nexus 5X emulation',
Expand Down Expand Up @@ -102,6 +103,7 @@ Example: --output-path=./lighthouse-results.html`,

// default values
.default('chrome-flags', '')
.default('enable-error-reporting', true)
.default('disable-cpu-throttling', false)
.default('output', GetValidOutputOptions()[OutputMode.domhtml])
.default('port', 0)
Expand Down
66 changes: 66 additions & 0 deletions lighthouse-cli/sentry-prompt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
import {existsSync} from 'fs';
import {join as joinPath} from 'path';

import {Configstore, inquirer} from './shim-modules';

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

const MAXIMUM_WAIT_TIME = 20 * 1000;

const MESSAGE = [
'Lighthouse would like to report back any errors that might occur while auditing. \n ',
'Information such as the URL you are auditing, its subresources, your operating system, Chrome, ',
'and Lighthouse versions may be recorded. Would you be willing to have Lighthouse automatically ',
'report this information to the team to aid in improving the product?',
].join('');

async function prompt() {
if (!process.stdout.isTTY || process.env.CI) {
// Default non-interactive sessions to false
return false;
}

let timeout: NodeJS.Timer;

const prompt = inquirer.prompt([
{
type: 'confirm',
name: 'isErrorReportingEnabled',
default: false,
message: MESSAGE,
},
]);

const timeoutPromise = new Promise((resolve: (a: boolean) => {}) => {
timeout = setTimeout(() => {
prompt.ui.close();
process.stdout.write('\n');
log.warn('CLI', 'No response to error logging preference, errors will not be reported.');
resolve(false);
}, MAXIMUM_WAIT_TIME);
});

return Promise.race([
prompt.then((result: {isErrorReportingEnabled: boolean}) => {
clearTimeout(timeout);
return result.isErrorReportingEnabled;
}),
timeoutPromise,
]);
}

export async function askPermission() {
const configstore = new Configstore('lighthouse');
let isErrorReportingEnabled = configstore.get('isErrorReportingEnabled');
if (typeof isErrorReportingEnabled === 'boolean') {
return isErrorReportingEnabled;
}

isErrorReportingEnabled = await prompt();
configstore.set('isErrorReportingEnabled', isErrorReportingEnabled);
return isErrorReportingEnabled;
}

export function isDev() {
return existsSync(joinPath(__dirname, '../.git'));
}
25 changes: 24 additions & 1 deletion lighthouse-cli/shim-modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,27 @@ try {
};
}

export {opn, updateNotifier};
let inquirer;
try {
inquirer = require('inquirer');
} catch (e) {
inquirer = {
prompt() {
console.error('module `inquirer` not installed. Not reporting errors.');
return Promise.resolve({isErrorReportingEnabled: false});
}
}
}

let Configstore;
try {
Configstore = require('configstore');
} catch (e) {
Configstore = function Configstore() {
console.error('module `configstore` not installed. Not persisting user choices.');
this.get = () => false;
this.set = () => undefined;
}
}

export {opn, updateNotifier, inquirer, Configstore};
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

const Audit = require('../audit');
const Formatter = require('../../report/formatter');
const Sentry = require('../../lib/sentry');

const KB_IN_BYTES = 1024;

Expand Down Expand Up @@ -108,6 +109,14 @@ class UnusedBytes extends Audit {
const v1TableHeadings = Audit.makeV1TableHeadings(result.headings);
const v2TableDetails = Audit.makeV2TableDetails(result.headings, results);

if (debugString) {
// Use captureException to preserve the stack and take advantage of Sentry grouping
Sentry.captureException(new Error(debugString), {
tags: {audit: this.meta.name},
level: 'warning',
});
}

return {
debugString,
displayValue,
Expand Down
15 changes: 15 additions & 0 deletions lighthouse-core/audits/load-fast-enough-for-pwa.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const Audit = require('./audit');
const URL = require('../lib/url-shim');
const Emulation = require('../lib/emulation');
const Formatter = require('../report/formatter');
const Sentry = require('../lib/sentry');
const Util = require('../report/v2/renderer/util.js');

// Maximum TTFI to be considered "fast" for PWA baseline checklist
Expand Down Expand Up @@ -111,6 +112,20 @@ class LoadFastEnough4Pwa extends Audit {
}

if (!areLatenciesAll3G) {
const sentryContext = Sentry.getContext();
const hadThrottlingEnabled = sentryContext && sentryContext.extra &&
sentryContext.extra.networkThrottling;

if (hadThrottlingEnabled) {
const violatingLatency = firstRequestLatencies
.find(item => Number(item.latency) < latency3gMin);
Sentry.captureMessage('Network request latencies were not realistic', {
tags: {audit: this.meta.name},
extra: {violatingLatency},
level: 'warning',
});
}

return {
rawValue: true,
// eslint-disable-next-line max-len
Expand Down
3 changes: 3 additions & 0 deletions lighthouse-core/gather/computed/trace-of-tab.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

const ComputedArtifact = require('./computed-artifact');
const log = require('../../lib/log');
const Sentry = require('../../lib/sentry');

class TraceOfTab extends ComputedArtifact {
get name() {
Expand Down Expand Up @@ -69,6 +70,8 @@ class TraceOfTab extends ComputedArtifact {
// In this case, we'll use the last firstMeaningfulPaintCandidate we can find.
// However, if no candidates were found (a bogus trace, likely), we fail.
if (!firstMeaningfulPaint) {
Sentry.captureMessage('No firstMeaningfulPaint found, using fallback');

const fmpCand = 'firstMeaningfulPaintCandidate';
log.verbose('trace-of-tab', `No firstMeaningfulPaint found, falling back to last ${fmpCand}`);
const lastCandidate = frameEvents.filter(e => e.name === fmpCand).pop();
Expand Down
12 changes: 8 additions & 4 deletions lighthouse-core/gather/gather-runner.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ const log = require('../lib/log.js');
const Audit = require('../audits/audit');
const URL = require('../lib/url-shim');
const NetworkRecorder = require('../lib/network-recorder.js');
const Sentry = require('../lib/sentry');

/**
* @typedef {!Object<string, !Array<!Promise<*>>>}
Expand Down Expand Up @@ -121,12 +122,15 @@ class GatherRunner {
* Test any error output from the promise, absorbing non-fatal errors and
* throwing on fatal ones so that run is stopped.
* @param {!Promise<*>} promise
* @param {string=} gathererName
* @return {!Promise<*>}
*/
static recoverOrThrow(promise) {
static recoverOrThrow(promise, gathererName) {
return promise.catch(err => {
if (err.fatal) {
throw err;
} else {
Sentry.captureException(err, {tags: {gatherer: gathererName}, level: 'warning'});
}
});
}
Expand Down Expand Up @@ -173,7 +177,7 @@ class GatherRunner {
return chain.then(_ => {
const artifactPromise = Promise.resolve().then(_ => gatherer.beforePass(options));
gathererResults[gatherer.name] = [artifactPromise];
return GatherRunner.recoverOrThrow(artifactPromise);
return GatherRunner.recoverOrThrow(artifactPromise, gatherer.name);
});
}, pass);
}
Expand Down Expand Up @@ -212,7 +216,7 @@ class GatherRunner {
return chain.then(_ => {
const artifactPromise = Promise.resolve().then(_ => gatherer.pass(options));
gathererResults[gatherer.name].push(artifactPromise);
return GatherRunner.recoverOrThrow(artifactPromise);
return GatherRunner.recoverOrThrow(artifactPromise, gatherer.name);
});
}, pass);
}
Expand Down Expand Up @@ -269,7 +273,7 @@ class GatherRunner {
log.log('status', status);
const artifactPromise = Promise.resolve().then(_ => gatherer.afterPass(options, passData));
gathererResults[gatherer.name].push(artifactPromise);
return GatherRunner.recoverOrThrow(artifactPromise);
return GatherRunner.recoverOrThrow(artifactPromise, gatherer.name);
}).then(_ => {
log.verbose('statusEnd', status);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@

const Gatherer = require('../gatherer');
const URL = require('../../../lib/url-shim');
const Sentry = require('../../../lib/sentry');

/* global document, Image, atob */

Expand Down Expand Up @@ -126,7 +127,14 @@ class OptimizedImages extends Gatherer {
return imageRecords.reduce((promise, record) => {
return promise.then(results => {
return this.calculateImageStats(driver, record)
.catch(err => ({failed: true, err}))
.catch(err => {
Sentry.captureException(err, {
tags: {gatherer: 'optimized-images'},
extra: {imageUrl: URL.elideDataURI(record.url)},
level: 'warning',
});
return {failed: true, err};
})
.then(stats => {
if (!stats) {
return results;
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-core/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ const Config = require('./config/config');
*
*/

module.exports = function(url, flags = {}, configJSON) {
module.exports = function(url, flags = {}, configJSON, environmentData) {
const startTime = Date.now();
return Promise.resolve().then(_ => {
if (!url) {
Expand All @@ -42,7 +42,7 @@ module.exports = function(url, flags = {}, configJSON) {
const connection = new ChromeProtocol(flags.port);

// kick off a lighthouse run
return Runner.run(connection, {url, flags, config})
return Runner.run(connection, {url, flags, config, environmentData})
.then(lighthouseResults => {
// Annotate with time to run lighthouse.
const endTime = Date.now();
Expand Down
Loading

0 comments on commit 528286d

Please sign in to comment.