Closed Bug 1505233 Opened 4 years ago Closed 4 years ago

cleanup error handling in processor rules


(Socorro :: Processor, task, P2)


(Not tracked)



(Reporter: willkg, Assigned: willkg)



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
Blocks: 1505231
Priority: -- → P2
Commits pushed to master at
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.
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

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.
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.
Merge pull request #4696 from willkg/1505233-rules

fix bug 1505233: clean up Rule error handling
Closed: 4 years ago
Resolution: --- → FIXED
This comment in the PR is worth having in the bug, too, for archival purposes:

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