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

core(lib): add error reporting #2420

merged 26 commits into from
Oct 27, 2017

Conversation

patrickhulce
Copy link
Collaborator

@patrickhulce patrickhulce commented Jun 1, 2017

fixes #2364

@brendankenny
Copy link
Member

an overview of your design would be helpful (and be useful as docs for CONTRIBUTING). e.g. difference between expected errors and non-fatal ones, when to capture message vs exception, when things will be automatically collected and when it should be explicitly called (like the stack trace comment)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

some hopefully helpful comments.

An overview would definitely be helpful cause it is hard to tell what's on purpose and what's currently just sketched out (e.g. handling all our platforms) :)

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

if (typeof flags.disableErrorReporting === 'undefined') {
Copy link
Member

Choose a reason for hiding this comment

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

should be enableErrorReporting?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

that makes opt-out (which is the only usable flag) become --no-enable-error-reporting which feels odd, admittedly it definitely feels odd to check disableErrorReporting in runner but I didn't think we had any other case where we renamed the CLI flag from CLI to core

const results = await lighthouse(url, flags, config);
const results = await lighthouse(url, flags, config, {
environment: isDev() ? 'development' : 'production',
serverName: 'redacted',
Copy link
Member

Choose a reason for hiding this comment

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

?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

they send the serverName by default which is potentially private information we definitely we don't need to collect, this doesn't actually fix it but was trying it out

@@ -122,10 +123,17 @@ class LoadFastEnough4Pwa extends Audit {
}

if (!areLatenciesAll3G) {
// eslint-disable-next-line max-len
Copy link
Member

Choose a reason for hiding this comment

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

move not needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya i'll revert

// eslint-disable-next-line max-len
const debugString = `First Interactive was found at ${timeToFirstInteractive.toLocaleString()}, however, the network request latencies were not sufficiently realistic, so the performance measurements cannot be trusted.`;

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.

what is the level of detail we want to log for this sort of thing? I guess magnitude of the issue is something, and then can cross reference with the site being tested?

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 saving the diff to 150ms sgtm

@@ -287,7 +288,7 @@ class Driver {
}

// If both the data and the url are empty strings, the page had no manifest.
return reject('No web app manifest found.');
return reject(ExpectedError(new Error('No web app manifest found.')));
Copy link
Member

Choose a reason for hiding this comment

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

this seems more like impetus to add a third return possibility (e.g. null)

Copy link
Member

Choose a reason for hiding this comment

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

(and this whole method will hopefully get fixed in #2349 (comment))

return promise.catch(err => {
if (err.fatal) {
throw err;
} else {
Sentry.captureException(err, {tags: {gatherer: gathererName}, level: 'warning'});
Copy link
Member

Choose a reason for hiding this comment

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

if this is just for non-fatal errors, may make more sense in collectArtifacts (or even runner)

Copy link
Collaborator Author

@patrickhulce patrickhulce Jun 6, 2017

Choose a reason for hiding this comment

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

why there? the method calling out that just explicitly catches errors and looks at them was the first place that came to my mind or where this logic might live if I was looking for it, so open to much better explanations than that haha :)

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's just for pulling fatal errors out of the stream. Non-fatal errors are handled on down the line (and due to a continuing lack of fatal errors, we should start to consider getting rid of it altogether :)

collectArtifacts is explicitly for collating the results of gatherers, including any errors, and has a slot ready made for the sentry call

@@ -77,7 +79,7 @@ class StartUrl extends Gatherer {

afterPass(options, tracingData) {
if (!this.startUrl) {
return Promise.reject(new Error('No start_url found inside the manifest'));
return Promise.reject(ExpectedError(new Error('No start_url found inside the manifest')));
Copy link
Member

Choose a reason for hiding this comment

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

if we go with ExpectedError, it should definitely be new ExpectedError or createExpectedError and should settle on whether to pass a string or an exception 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.

passing a string with createExpectedError has the problem of sentry showing you the line where the error was created and not the more useful line here, and I thought you didn't like things extending from Error for some reason but I can't remember why? haha

const log = require('./log');

// Fix the polyfill. See https://github.com/GoogleChrome/lighthouse/issues/73
self.setImmediate = function (callback) {
Copy link
Member

Choose a reason for hiding this comment

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

prefer rest args, but what did happen to the fix for #73?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its still just waiting for us to bump devtools :)

};

const noop = () => undefined;
const sentryDelegate = module.exports = {
Copy link
Member

Choose a reason for hiding this comment

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

this delegate is very hard to follow but you probably already know that :)

@@ -33,6 +34,8 @@ class Runner {

const config = opts.config;

Sentry.init(opts.flags.disableErrorReporting === false, opts.environmentData || {});
Copy link
Member

Choose a reason for hiding this comment

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

definitely should be opts.flags.enableErrorReporting === true

Copy link
Member

Choose a reason for hiding this comment

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

is having || {} going to be confusing in the data? runner may want to try to infer more to differentiate different environments (but you might already be planning that)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

mentioned above too, but why "definitely"?

@brendankenny
Copy link
Member

and, just to be clear, the structure of things seems to be what we've been discussing for a while/what I was imaginging, so it's looking good to me :)

The biggest unfortunate thing is the expected/regular/fatal exception split, but we've also been anticipating having to go down that path for a while as well. Maybe we can revisit resolving on a sentinel value instead of rejecting on an "expected" exception, but avoid going back to the -1 world by still having them automatically managed by gather-runner/runner.

@@ -151,8 +152,8 @@ function saveResults(results: Results, artifacts: Object, flags: Flags) {
return innerPromise.then((_: Results) => Printer.write(results, outputType, outputPath));
}, Promise.resolve(results));
} else {
const outputPath =
flags.outputPath || `${resolvedPath}.report.${typeToExtension(flags.output)}`;
const outputPath = flags.outputPath ||
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this was actually totally breaking vscode-insiders syntax highlighting :) filed an issue about it

Copy link
Member

@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.

can you add us to the sentry project so we can see the results?


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. :)


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, ',
Copy link
Member

Choose a reason for hiding this comment

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

this message is pretty huge. (See screenshot) can we break it up?
fwiw the message string can be colorized before going into prompt()

Lighthouse requests permission to anonymously report back runtime exceptions
This can include data such as the test URL, its subresources, your OS, Chrome version, and Lighthouse version.
May Lighthouse report this data to aid in improving the tool?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

sure thing

@patrickhulce
Copy link
Collaborator Author

can you add us to the sentry project so we can see the results?

@paulirish yeah the free plan only allows one member though, time to apply for OSS account or whip out the gcard?

@paulirish
Copy link
Member

paulirish commented Jun 19, 2017 via email

networkThrottling: !opts.flags.disableNetworkThrottling,
cpuThrottling: !opts.flags.disableCpuThrottling,
};
Sentry.mergeContext({extra: Object.assign({}, environmentData.extra, sentryContext)});
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 moving this context setting stuff into lib/sentry ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ya done

@patrickhulce
Copy link
Collaborator Author

A not insignificant portion of this PR is now changing the behavior of the StartUrl gatherer to pass the debugString instead of an error, so might be worth breaking that out into a separate PR at this point

@brendankenny
Copy link
Member

so might be worth breaking that out into a separate PR at this point

I'd say 👍

@@ -56,6 +56,7 @@ export function getFlags() {
],
'Configuration:')
.describe({
'enable-error-reporting': 'Enables error reporting (on by default)',
Copy link
Member

Choose a reason for hiding this comment

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

maybe we can come up with a different flag name, as it isn't really the case that it's on by default (instead it prompts by default), and a first glance may alarm some

Copy link
Collaborator Author

@patrickhulce patrickhulce Jun 20, 2017

Choose a reason for hiding this comment

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

hence why I had it as disable-error-reporting but someone wanted it flipped :P

CONTRIBUTING.md Outdated
}
```

#### If you throw an error in an audit to communicate audit failure instead of using the score property, *use createExpectedError*.
Copy link
Member

Choose a reason for hiding this comment

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

If you throw an error in an audit to communicate audit failure instead of using the score property, use createExpectedError

after the changes to manifest fetching/start_url, are expected errors still needed? It sounds more like we should just discourage throwing errors to indicate an expected negative result

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes I've removed createExpectedError entirely now, will update

@patrickhulce
Copy link
Collaborator Author

patrickhulce commented Jun 20, 2017

opened and rebased against #2549

@paulirish
Copy link
Member

Blocked on #2549

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

some ideas on not saying opt-out since there's just opting in or not opting in. Take or leave these :)

* 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

* Your operating system

## How do I opt-in/opt-out?
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


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


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.

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

Copy link
Member

@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.

great doc.

* Your operating system

## How do I opt-in/opt-out?
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.

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

@paulirish
Copy link
Member

@patrickhulce change lgtm

heads up: this discussion is unresolved https://github.com/GoogleChrome/lighthouse/pull/2420/files#r145858868

@paulirish
Copy link
Member

@brendankenny lets do thisssssssssss. @patrickhulce now understood what you were going for in that init() thread. :)

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

two last things, but otherwise LGTM!

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

},
});

return await runLighthouse(url, cliFlags, config);
Copy link
Member

Choose a reason for hiding this comment

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

nit: since the function is async anyway, can drop the await 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.

done

@patrickhulce patrickhulce merged commit de57850 into master Oct 27, 2017
@patrickhulce patrickhulce deleted the add_sentry branch October 27, 2017 00:01
@paulirish
Copy link
Member

epic PR. super excited about this. \o/

christhompson pushed a commit to christhompson/lighthouse that referenced this pull request Nov 28, 2017
* refactor(StartUrl): switch from error to debugString object

* feat: add error reporting

* cave to peer pressure

* lint

* merge fixes

* reduce yarn churn

* limit impact to run.ts

* clang format

* avoid duplicate errors

* lint

* disable error reporting on test

* feedback

* remove enableErrorReporting default

* update readme

* fix yarn.lock

* add error reporting doc

* doc feedback

* move sentry call

* more cleanup

* remove unused environmentData
@GoogleChrome GoogleChrome deleted a comment from stevinus73 Feb 4, 2022
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.

Non-fatal events to track
3 participants