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(lib): add error reporting #2420

Merged
merged 26 commits into from
Oct 27, 2017
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
fbd724e
refactor(StartUrl): switch from error to debugString object
patrickhulce Jun 20, 2017
528286d
feat: add error reporting
patrickhulce Jun 20, 2017
062fee6
cave to peer pressure
patrickhulce Jun 20, 2017
b7f89f6
lint
patrickhulce Jun 21, 2017
94f6f54
Merge branch 'master' into add_sentry
patrickhulce Jul 12, 2017
5ebc608
merge fixes
patrickhulce Jul 12, 2017
80f2040
reduce yarn churn
patrickhulce Jul 12, 2017
140ac9e
limit impact to run.ts
patrickhulce Jul 12, 2017
eea3f67
clang format
patrickhulce Jul 12, 2017
40c0506
avoid duplicate errors
patrickhulce Jul 12, 2017
2624082
lint
patrickhulce Jul 12, 2017
011a77d
disable error reporting on test
patrickhulce Jul 12, 2017
14a833a
feedback
patrickhulce Jul 20, 2017
7ba93f4
remove enableErrorReporting default
patrickhulce Jul 20, 2017
37718c2
Merge branch 'master' into add_sentry
patrickhulce Aug 16, 2017
2891e64
Merge branch 'master' into add_sentry
patrickhulce Aug 16, 2017
58e186d
update readme
patrickhulce Aug 16, 2017
7d83585
Merge branch 'master' into add_sentry
patrickhulce Oct 17, 2017
38c9630
fix yarn.lock
patrickhulce Oct 17, 2017
49c061c
add error reporting doc
patrickhulce Oct 19, 2017
cf0e640
doc feedback
patrickhulce Oct 20, 2017
637b124
Merge branch 'master' into add_sentry
patrickhulce Oct 20, 2017
d77270a
move sentry call
patrickhulce Oct 21, 2017
6b4f9ea
more cleanup
patrickhulce Oct 21, 2017
1aac5a2
remove unused environmentData
patrickhulce Oct 26, 2017
a4c982d
merge branch 'master' into add_sentry
patrickhulce Oct 26, 2017
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
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
30 changes: 30 additions & 0 deletions docs/error-reporting.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Error Reporting Explained

## What's going on?

The Lighthouse team is constantly trying to improve the reliability of our tools. We've added functionality to anonymously report runtime exceptions, given your consent, using [Sentry](https://sentry.io/welcome/). We will use this information to detect new bugs and avoid regressions.
Copy link
Member

Choose a reason for hiding this comment

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

maybe
"If you give your consent, we've added functionality to anonymously report runtime exceptions using [Sentry](https://sentry.io/welcome/)."

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

heh, but the functionality is there if they give consent or not, the phrase is modifying "report" ;)

rephrased


Only CLI users are currently impacted. Node module consumers are opted out by default without a prompt, and the DevTools and extension integrations have no automatic error reporting functionality.
Copy link
Member

Choose a reason for hiding this comment

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

maybe just include Node module consumers with Devtools and extension users for now since it won't be happening for them?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah was trying to be explicit that the code isn't even there, but that was probably confusing


## What will happen if I opt-in?
Runtime exceptions will be reported to the team along with information on your environment such as the URL you tested, your OS, and Chrome version. See [what data gets reported](#what-data-gets-reported).

## What will happen if I opt-out?
Copy link
Member

Choose a reason for hiding this comment

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

"What will happen if I don't opt-in?"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Runtime exceptions will not be reported to the team. Your ability to use Lighthouse will not be affected in any way.

## What data gets reported?

* The URL you tested
* The runtime settings used (throttling enabled/disabled, emulation, etc)
* The message, stack trace, and associated data of the error
* Your Lighthouse version
* Your Chrome version
* Your operating system

## How do I opt-in/opt-out?
Copy link
Member

Choose a reason for hiding this comment

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

maybe just have "How do I opt-in?" to be clear it's only opt-in?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

The first time you run the CLI you will be prompted with a message asking you if Lighthouse can anonymously report runtime exceptions. You can give a direct response of `yes` or `no` to save your response, and you will not be prompted again. If no response is given within 20 seconds, a `no` response will be assumed, and you will not be prompted again. Non-interactive terminal sessions and invocations with the `CI` environment variable set will automatically be opted out.
Copy link
Member

Choose a reason for hiding this comment

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

"You can give a direct response of yes or no to save your response, and you will not be prompted again." -> "You can give a direct response of yes or no, and you will not be prompted again.", maybe?

Copy link
Member

Choose a reason for hiding this comment

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

"will automatically be opted out." -> "will automatically not be opted in." maybe? (slightly more awkward but clearer there is no state that needs to be opted out of :)

Copy link
Member

Choose a reason for hiding this comment

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

also "y" and "n" work.
and just hitting enter defaults to "no". this should be mentioned as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done


The CLI also has two flags to control error reporting that will override the saved preference. Running Lighthouse with `--enable-error-reporting` will report errors regardless of the saved preference, and running Lighthouse with `--no-enable-error-reporting` will *not* report errors regardless of the saved preferences.

## How do I change my opt-in/opt-out preference?
Copy link
Member

Choose a reason for hiding this comment

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

How do I change my opt-in preference??

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Your response to the prompt will be saved to your home directory `~/.config/configstore/lighthouse.json` and used on future runs. To trigger a re-prompt, simply delete this file and Lighthouse will ask again on the next run. You can also edit this json file directly or run Lighthouse with the `--[no-]enable-error-reporting` flags.
20 changes: 18 additions & 2 deletions lighthouse-cli/bin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
'use strict';

import {existsSync} from 'fs';
import * as path from 'path';

import * as Commands from './commands/commands';
Expand All @@ -18,7 +19,11 @@ const pkg = require('../package.json');

// accept noop modules for these, so the real dependency is optional.
import {updateNotifier} from './shim-modules';
import {askPermission} from './sentry-prompt';

function isDev() {
return existsSync(path.join(__dirname, '../.git'));
}

// Tell user if there's a newer version of LH.
updateNotifier({pkg}).notify();
Expand Down Expand Up @@ -59,6 +64,17 @@ if (cliFlags.output === Printer.OutputMode[Printer.OutputMode.json] && !cliFlags
cliFlags.outputPath = 'stdout';
}

export function run() {
return runLighthouse(url, cliFlags, config);
export async function run() {
if (typeof cliFlags.enableErrorReporting === 'undefined') {
cliFlags.enableErrorReporting = await askPermission();
}

return await runLighthouse(url, cliFlags, config, {
name: 'redacted', // prevent sentry from using hostname
environment: isDev() ? 'development' : 'production',
release: pkg.version,
tags: {
channel: 'cli',
},
});
}
6 changes: 4 additions & 2 deletions 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 {
port: number, chromeFlags: string, output: any, outputPath: string, saveArtifacts: boolean,
saveAssets: boolean, view: boolean, maxWaitForLoad: number, logLevel: string,
hostname: string, blockedUrlPatterns: string[]
hostname: string, blockedUrlPatterns: string[], enableErrorReporting: boolean
}

export function getFlags(manualArgv?: string) {
Expand Down Expand Up @@ -54,10 +54,12 @@ export function getFlags(manualArgv?: string) {
[
'save-assets', 'save-artifacts', 'list-all-audits', 'list-trace-categories',
'additional-trace-categories', 'config-path', 'chrome-flags', 'perf', 'port',
'hostname', 'max-wait-for-load'
'hostname', 'max-wait-for-load', 'enable-error-reporting'
],
'Configuration:')
.describe({
'enable-error-reporting':
'Enables error reporting (prompts once by default, setting this flag will force error reporting to that state).',
Copy link
Member

Choose a reason for hiding this comment

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

should add this to the readme.

We probably also want to add a note to the readme that the CLI will ask you one time if you want to opt in to sending analytics about errors LH runs into. Opting out will opt out permanently, or you can pass in --enable-error-reporting false, or if the CI environment variable is set to true.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

'blocked-url-patterns': 'Block any network requests to the specified URL patterns',
'disable-storage-reset':
'Disable clearing the browser cache and other storage APIs before a run',
Expand Down
4 changes: 2 additions & 2 deletions lighthouse-cli/run.ts
Original file line number Diff line number Diff line change
Expand Up @@ -138,13 +138,13 @@ export function saveResults(results: Results, artifacts: Object, flags: Flags) {
}

export async function runLighthouse(
url: string, flags: Flags, config: Object|null): Promise<{}|void> {
url: string, flags: Flags, config: Object|null, environmentData?: Object): Promise<{}|void> {
Copy link
Member

Choose a reason for hiding this comment

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

not needed anymore?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

let launchedChrome: LaunchedChrome|undefined;

try {
launchedChrome = await getDebuggableChrome(flags);
flags.port = launchedChrome.port;
const results = await lighthouse(url, flags, config);
const results = await lighthouse(url, flags, config, environmentData);

const artifacts = results.artifacts;
delete results.artifacts;
Expand Down
63 changes: 63 additions & 0 deletions lighthouse-cli/sentry-prompt.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
/**
* @license Copyright 2017 Google Inc. All Rights Reserved.
* 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.
*/
import {Configstore, inquirer} from './shim-modules';

const log = require('lighthouse-logger');

const MAXIMUM_WAIT_TIME = 20 * 1000;
// clang-format off
const MESSAGE =
`${log.reset}We're constantly trying to improve Lighthouse and its reliability.\n ` +
`May we anonymously report runtime exceptions to improve the tool over time?\n ` +
`${log.reset}Learn more: https://github.com/GoogleChrome/lighthouse/blob/master/docs/error-reporting.md`;
// clang-format on

async function prompt() {
if (!process.stdout.isTTY || process.env.CI) {
// Default non-interactive sessions to false
Copy link
Member

Choose a reason for hiding this comment

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

wdyt about defaulting CI to true?

Copy link
Collaborator Author

@patrickhulce patrickhulce Oct 19, 2017

Choose a reason for hiding this comment

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

Hmm I'm definitely against in our CI so an opt-out env variable seems good.

It'd be nice to gather errors from folks running Lighthouse in CI, but silently opting them in seems bad. Can always use --enable-error-reporting to skip this part entirely and run with error reporting so I'd vote no to default CI true.

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 do like the idea of enabling this for our smoke tests on master though 👍 followup work

Copy link
Member

Choose a reason for hiding this comment

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

ok sg

return false;
}

let timeout: NodeJS.Timer;

const prompt = inquirer.prompt([
Copy link
Member

Choose a reason for hiding this comment

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

getting an exception in the prompt() method:

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

hmm strange, I can't repro, it happens every time?

Copy link
Member

Choose a reason for hiding this comment

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

yarn install-all fixes. :)

{
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;
}
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};
1 change: 1 addition & 0 deletions lighthouse-cli/test/cli/run-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ describe('CLI run', function() {
const url = 'chrome://version';
const filename = path.join(process.cwd(), 'run.ts.results.json');
const flags = getFlags(`--output=json --output-path=${filename} ${url}`);
flags.enableErrorReporting = false;
Copy link
Member

Choose a reason for hiding this comment

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

why use this here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

otherwise the mock and expected errors from tests get reported to sentry, this test skips bin.ts where we do the prompting/setting to false for CI

return run.runLighthouse(url, flags, fastConfig).then(passedResults => {
assert.ok(fs.existsSync(filename));
const results = JSON.parse(fs.readFileSync(filename, 'utf-8'));
Expand Down
10 changes: 10 additions & 0 deletions lighthouse-core/audits/byte-efficiency/byte-efficiency-audit.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
'use strict';

const Audit = require('../audit');
const Sentry = require('../../lib/sentry');
const Util = require('../../report/v2/renderer/util');

const KB_IN_BYTES = 1024;
Expand Down Expand Up @@ -134,6 +135,15 @@ class UnusedBytes extends Audit {

const tableDetails = Audit.makeTableDetails(result.headings, results);

if (debugString) {
// Track these with Sentry since we don't want to fail the entire audit for a single image failure.
// 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
17 changes: 17 additions & 0 deletions lighthouse-core/audits/load-fast-enough-for-pwa.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
const Audit = require('./audit');
const URL = require('../lib/url-shim');
const Emulation = require('../lib/emulation');
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,22 @@ class LoadFastEnough4Pwa extends Audit {
}

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

if (hadThrottlingEnabled) {
// Track these instances in Sentry since there should be no requests that are fast when
// throttling is enabled, and it's likely a throttling bug we should look into.
const violatingLatency = firstRequestLatencies
.find(item => Number(item.latency) < latency3gMin);
Sentry.captureMessage('Network request latencies were not realistic', {
Copy link
Member

Choose a reason for hiding this comment

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

would you mind adding a comment to each of these special sentry captures why they're worth capturing (vs not or letting the error handler get them)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah

tags: {audit: this.meta.name},
extra: {violatingLatency},
level: 'warning',
});
}

return {
rawValue: true,
// eslint-disable-next-line max-len
Expand Down
4 changes: 4 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('lighthouse-logger');
const Sentry = require('../../lib/sentry');

// Bring in web-inspector for side effect of adding [].stableSort
// See https://github.com/GoogleChrome/lighthouse/pull/2415
Expand Down Expand Up @@ -76,6 +77,9 @@ 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) {
// Track this with Sentry since it's likely a bug we should investigate.
Sentry.captureMessage('No firstMeaningfulPaint found, using fallback');

const fmpCand = 'firstMeaningfulPaintCandidate';
fmpFellBack = true;
log.verbose('trace-of-tab', `No firstMeaningfulPaint found, falling back to last ${fmpCand}`);
Expand Down
12 changes: 11 additions & 1 deletion lighthouse-core/gather/gatherers/dobetterweb/optimized-images.js
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');

const JPEG_QUALITY = 0.92;
const WEBP_QUALITY = 0.85;
Expand Down Expand Up @@ -161,7 +162,16 @@ 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 => {
// Track this with Sentry since these errors aren't surfaced anywhere else, but we don't
// want to tank the entire run due to a single image.
Sentry.captureException(err, {
tags: {gatherer: 'OptimizedImages'},
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(_ => {
// set logging preferences, assume quiet
Expand All @@ -38,7 +38,7 @@ module.exports = function(url, flags = {}, configJSON) {
const connection = new ChromeProtocol(flags.port, flags.hostname);

// 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