-
Notifications
You must be signed in to change notification settings - Fork 5
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Use custom exceptions for recording errors. #193
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,10 @@ | |
confluent_kafka = None | ||
|
||
|
||
class EventProductionException(Exception): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should these live in a public module? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think it needs to, since this is custom to the producing code, and we already had the pattern of keeping consumer exceptions in the |
||
""" An exception we can check for when errors occur in event production code. """ | ||
|
||
|
||
def record_producing_error(error, context): | ||
""" | ||
Record an error in producing an event to both the monitoring system and the regular logs | ||
|
@@ -49,8 +53,8 @@ def record_producing_error(error, context): | |
try: | ||
# record_exception() is a wrapper around a New Relic method that can only be called within an except block, | ||
# so first re-raise the error | ||
raise Exception(error) | ||
except BaseException: | ||
raise EventProductionException(error) | ||
except EventProductionException: | ||
record_exception() | ||
logger.exception(f"Error delivering message to Kafka event bus. {error=!s} {context!r}") | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So will this end up recording the
EventConsumptionException
, or will it record the type oferror
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Well, I guess if we lose the type of
error
, that's already happening, so no complaint here.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we were losing the type of error previously. This allows us to at least see a
EventConsumptionException
recorded as theerror.class
in New Relic.