misc(error-reporting): tweak sentry levels and ignore list#3890
misc(error-reporting): tweak sentry levels and ignore list#3890
Conversation
patrickhulce
requested review from
brendankenny and
paulirish
as code owners
lighthouse-core/lib/sentry.js
Outdated
| getContext: noop, | ||
| }; | ||
|
|
||
| const IGNORED_ERRORS = [ |
There was a problem hiding this comment.
afaict these two aren't even being reported to sentry. why start excluding them?
There was a problem hiding this comment.
I marked the unable to load the page ones as ignore in sentry, is there a reason you want to track them?
There was a problem hiding this comment.
ah unable to load is ignored.
but tracingalreadystarted? is that even reported? getting a repro via CLI would be useful as it seems the only repros are with devtools/ extension
There was a problem hiding this comment.
alright, fair enough
just seemed like information we wouldn't actually learn anything from as we have no reason to believe it has anything to do with the URL being audited
lighthouse-core/lib/sentry.js
Outdated
| // Ignore errors matching our shortlist | ||
| if (args[0] && IGNORED_ERRORS.some(regex => regex.test(args[0].message))) return empty; | ||
| // Only report 25% of production errors | ||
| if (!isDevelopment && Math.random() > 0.25) return empty; |
There was a problem hiding this comment.
we have a fairly long tail of errors reported. and i'm seeing quite a few with < 3 reports that look worthwhile. this makes me hesitant to throw so many away.
There was a problem hiding this comment.
could we sample just the 'info' level events?
There was a problem hiding this comment.
could we sample just the 'info' level events?
yes good idea, but FWIW nothing is actually info yet :)
we have a fairly long tail of errors reported. and i'm seeing quite a few with < 3 reports that look worthwhile. this makes me hesitant to throw so many away.
true, I might argue that given our current outstanding issues if something is <0.1% right now we likely shouldn't be investing time to fix it
seems like we need to pick at least 1 of the following:
- ignore more known issues (like falling out of memory cache)
- sample events
- upgrade our plan
There was a problem hiding this comment.
offline we discussed sampling a safelist of known chatty exceptions
lighthouse-core/lib/sentry.js
Outdated
| if (args[0] && args[0].expected) return Promise.resolve(); | ||
| const empty = Promise.resolve(); | ||
| // Ignore expected errors | ||
| if (args[0] && args[0].expected) return empty; |
There was a problem hiding this comment.
feels like it's about time we name the first argument (and maybe second) and spread the remaining.
patrickhulce
force-pushed
the
sentry_tweaks
branch
from
6dc39e4 to
ac8a41b
Compare
lighthouse-core/lib/sentry.js
Outdated
| // Only report 25% of production errors | ||
| if (!isDevelopment && Math.random() > 0.25) return empty; | ||
| if (err.expected) return empty; | ||
| // Sample known errors that occurr at a high frequency |
ghost
mentioned this pull request
3 participants
now that we've had sentry in WPT and the volume shot up this makes some tweaks to make it a little more manageable