CV2-4952: ClassyCat Error Handling and Metrics #104
Conversation
…s with correct error codes. not tested yet.
# Conflicts: # lib/queue/processor.py
ashkankzme
requested review from
DGaffney,
computermacgyver and
skyemeedan
as code owners
| @@ -64,7 +68,10 @@ def get_response(self, message: schemas.Message) -> schemas.GenericItem: | |||
| result = self.process(message) | |||
| Cache.set_cached_result(message.body.content_hash, result) | |||
| except Exception as e: | |||
| return self.handle_fingerprinting_error(e) | |||
| if isinstance(e, PrestoBaseException): | |||
There was a problem hiding this comment.
since we are (I think) nicely bubbling up errors to this level, should logger.exception() here so that it will still get logged to Sentry from the presto side?
There was a problem hiding this comment.
I think this exists on line 58, no?
There was a problem hiding this comment.
oh, I didn't realize that is what capture_custom_messages does. In Presto we are explicitly capturing message so it will go to Sentry only and not normal python logs? If so maybe that capture message argument should be 'error' instead of 'info'
There was a problem hiding this comment.
done! (changed info to error)
overall, I also think it's better to log errors when they happen. makes debugging easier, and we also won't lose the line number on which the error logging happens. for all of classycat errors, I made sure we log the errors where they happen.
test/lib/model/test_classycat.py
Outdated
|
|
||
| self.assertEqual(result.responseMessage, "Error classifying items: list index out of range") | ||
| try: | ||
| result = self.classycat_model.process(classify_message) |
There was a problem hiding this comment.
side comment: there is an assertRaises pattern in the unitTest framework that can be helpful for these
There was a problem hiding this comment.
great idea, updated
# Conflicts: # lib/model/classycat_classify.py
ashkankzme
changed the title
Description
Error handling for ClassyCat using new error handling / exception mechanisms in Presto
ClassyCat modules now raise unhandled "PrestoBaseException" objects that include an http error code to properly respond to consumer requests.
Reference: CV2-4952
How has this been tested?
Has been tested locally and in unit tests
Are there any external dependencies?
No
Have you considered secure coding practices when writing this code?
We return stack traces included in error messages, which raises security concerns.