Closed Bug 1505233 Opened 3 years ago Closed 3 years ago
cleanup error handling in processor rules
socorro.lib.transform_rule has a Rule class that processor rules derive from. This class has a ton of error handling. Since this was written, I think the requirements for rules regarding errors have changed. Before, it seems like it was helpful for processing to continue even if rules threw errors. Now, it's more helpful for rules to throw errors and those errors get surfaced so we can fix the code. This bug covers moving Rule to be part of the processor code rather than socorro.lib and simplifying it as much as we can.
Assignee: nobody → willkg
Status: NEW → ASSIGNED
Priority: -- → P2
Commits pushed to master at https://github.com/mozilla-services/socorro https://github.com/mozilla-services/socorro/commit/c782708c4a212644d054f62b32361b18fddf68c7 bug 1505233: move Rule to processor This moves socorro.lib.transform_rules to socorro.processor.rules.base. Rules are only used by the processor, so it's easier if the code is centralized there. https://github.com/mozilla-services/socorro/commit/80900623526052117547f5716bbc2523b794b1a7 fix bug 1505233: gut Rule and simplify it Previously, Rule was implemented so that a rule could stop processing for a crash report. We don't use that feature, so we can nix that and stop checking the return of action and act. Rule had action/_action and predicate/_predicate where action and predicate had a bunch of error handling. Instead of handling errors in Rule, we're going to let them get handled by the thing running the rules. Rules should be defensive and thoughtful in how they execute and not throw errors willy-nilly. Rules used to have versions. I took all that out since it's not used anywhere. After simplifying Rule, there wasn't anything to test, so I removed the Rule tests. There were some other subclasses of Rule, but those weren't used, so I removed those and their tests. https://github.com/mozilla-services/socorro/commit/d0ae0f347797b908584e7c6830d188dbeb795eff fix bug 1505233: push exception handling to process_crash We pulled all the exception handling out of Rule. This puts it in process_crash. It'll surface errors in sentry and also add a processor note so we can reprocess crashes. https://github.com/mozilla-services/socorro/commit/19dd7b51c1fa824588c64ceb4d07683cbaeb52bd Merge pull request #4696 from willkg/1505233-rules fix bug 1505233: clean up Rule error handling
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
This comment in the PR is worth having in the bug, too, for archival purposes: https://github.com/mozilla-services/socorro/pull/4696#discussion_r231596817 > The thing that calls process_crash also has error handling, but > that error handling is tied in with processing the crash and > saving it. I didn't want to further complicate that. Further, if > we handled errors there, processing would halt on the first error > thrown. That'd be great if we had a system where crashes that > couldn't get through processing were put aside in a separate bin. > But we don't have that. Our options are: > > 1. continue processing despite errors and end up with a partially > processed crash > 2. throw it back in the rabbitmq queue > 3. drop it altogether and never process it again > > We can't do option 3 because then we have crashes we've saved to > S3 but don't have indexed and don't know anything about. Option > 2 could lead to the queue filled with bad crashes and the > processor grinding to a halt. Option 1 is unenthusing and could > lead to confusion during analysis. > > I'm going for option 1. The hope is that we'll catch the errors > in sentry and fix the problems and push the fixes out. Since we're > adding notes to the crashes in question, we can reprocess them > once the problem is fixed. > > We'll see how that goes.
You need to log in before you can comment on or make changes to this bug.