Skip to content
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

Merged
merged 1 commit into from
Aug 29, 2023

Conversation

dianakhuang
Copy link
Contributor

@dianakhuang dianakhuang commented Aug 28, 2023

In the past, we used generic exceptions to record errors to New Relic. This creates custom exceptions for producing and consuming so that we can query New Relic on custom exceptions instead of relying on log messages.

edx/edx-arch-experiments#389

Merge checklist:
Check off if complete or not applicable:

  • Version bumped
  • Changelog record added
  • Documentation updated (not only docstrings)
  • Commits are squashed
  • Noted any: Concerns, dependencies, deadlines, tickets, testing instructions

In the past, we used generic exceptions to record errors
to New Relic. This creates custom exceptions for producing
and consuming so that we can query New Relic on custom
exceptions instead of relying on log messages.

edx/edx-arch-experiments#389
raise Exception(error)
except BaseException:
raise EventConsumptionException(error)
except EventConsumptionException:
self._add_message_monitoring(run_context=run_context, message=maybe_kafka_message, error=error)
record_exception()
Copy link
Contributor

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 of error?

Copy link
Contributor

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

Copy link
Contributor Author

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 the error.class in New Relic.

@@ -38,6 +38,10 @@
confluent_kafka = None


class EventProductionException(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these live in a public module?

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 consumer.py file. I am open to disagreements, though.

@dianakhuang dianakhuang merged commit d867e8e into main Aug 29, 2023
9 checks passed
@dianakhuang dianakhuang deleted the diana/custom-exceptions branch August 29, 2023 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants