1645423 - Sentry should ignore errors caused by local changes

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement, P3)

Description

[Mitchell Hentges [:mhentges] 🦀]

This is a tricky problem, because we haven't bundled Sentry with released, static software.
The issue is that if I'm working on a ./mach subcommand, and I'm iterating on solving a problem, I will probably run into and solve errors as I go. Unfortunately, Sentry will report these errors, and it's tricky to tell from the Sentry dashboard which errors are "real" and which are "local development" temporary errors. This is unwanted noise.

In regular released, static software bundled with Sentry, this isn't an issue because users (usually) aren't modifying the code being tracked by Sentry.

I feel that we have two vague options here:

1. Don't report Sentry issues that are caused by local mach changes. I'm not sure how this would be done - have a blacklist of mach-y files, don't submit if the diff from .::central contains any of these files? Hmm.
2. Report the Sentry issues, but attach additional information, like the changed files from .::central, maybe? Unsure.

Comment 1

[Ricky Stewart]

When I've dealt with similar issues in the past, we would just ignore or not report errors from certain people. e.g. if I (by which I mean me, Ricky) am running mach and it fails, I've either 1) encountered an issue in code I'm iterating on, in which case I'll fix it immediately or 2) encountered a bug that is new to me in which case I have enough context to just report, and likely fix, the bug. So reporting my mach failures to Sentry is almost never useful for anyone, it's just noise. We can start with a sensible group of blacklisted people, including you and me initially, but maybe expanding to others as we notice that certain people are over-represented in Sentry reports. Can we do something like that robustly?

Comment 2

[Sylvestre Ledru [:Sylvestre]]

Yeah, you probably received error from me today working on mozlint (working on avoiding the word blacklist ;)

You would like to create a skip list in-tree ?

Comment 3

[Ricky Stewart]

Yeah, I would like to skip logging for certain people entirely, in a hard-coded, in-tree list. That can be as simple as if getpass.getuser() in (MY_USER_NAME, MHENTGES_USER_NAME, ...), or something more robust that won't have false positives in case somebody else in the world happens to have the exact same Unix username as me. (Maybe we can use the moz-phab infra, if the user has moz-phab installed and configured, we can look up the corresponding Phabricator username and make sure it isn't rstewart/etc., which would be more reliable? I don't know how moz-phab works or whether this would be possible.)

My thinking is the following:

a) Suggestion (1) in comment 0 is going to be hard to build and maintain, to the point where I'm not sure it can be meaningfully implemented.

b) Suggestion (2) in comment 0 can work, but robustly diffing against central isn't trivial when you consider all the many ways people can configure their local DVCS'es. Furthermore this would still require somebody to look through all the changed_files and categorize them mentally. This is just as likely to increase noise rather than decrease noise IMO.

c) Instead, the no-log-list DOES actually decrease noise without significantly cutting down on signal that a Sentry maintainer would want to see.

Comment 4

[Mitchell Hentges [:mhentges] 🦀]

I'll create a separate ticket for the simple fix of having a blocklist (here).
In the future, depending on how much work it is, I'd really like to be able to avoid submitting errors entirely if a file imported by mach is changed between the current state and the closest central revision.

I'd like to reach a point where we don't receive any false negatives in our Sentry reports.

Comment 5

[Andrew Halberstadt [:ahal]]

It would be nice to have a per-command blocklist, as the maintainers of one command are usually not the same as the maintainers for another. If we implement this ability, I think it would be nice for the blocklist registration to occur near where the commands are defined rather than a central location. But no need to let perfect be the enemy of good.

Going back to the original idea of this bug..

What if we avoid submitting to Sentry if any of the modified files are mentioned in the traceback?

(In reply to Ricky Stewart from comment #3)

b) Suggestion (2) in comment 0 can work, but robustly diffing against central isn't trivial when you consider all the many ways people can configure their local DVCS'es.

We already have a utility function that implements this pretty robustly for both hg and git:
https://searchfox.org/mozilla-central/source/python/mozversioncontrol/mozversioncontrol/__init__.py#192

I'm not claiming it's perfect, but it's used in several production level places already (where the cost of a mistake would be far greater than it would here).

Comment 6

[Mitchell Hentges [:mhentges] 🦀]

In general, I think that we would rather that Sentry have false negatives (it doesn't send an error) than false positives (it sends errors that don't actually exist, such as due to local modifications). I think that this stance is necessary in the same way that it is for automated tests: a flaky test is worse than no test at all.

What if we avoid submitting to Sentry if any of the modified files are mentioned in the traceback?

I think that this is a decent solution, but errors may be caused by a file that isn't in the traceback. If this is because a developer modified that file locally, then the reported error would be a false positive.

On the flip side, I'm assuming that it's possible to deterministically tell which files are part of mach. If I'm mistaken here, perhaps traceback-checking will be more effective.

We already have a utility function that implements this pretty robustly for both hg and git:

This is great, thanks!

Comment 7

[Ricky Stewart]

Working on this.

Comment 8

[Ricky Stewart]

Attachment: Bug 1645423 - Sentry ignores errors caused by local changes

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/65482538a61f
Sentry ignores errors caused by local changes r=mhentges

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/65482538a61f