Adding additional testing for exception handling #2433
Conversation
|
There was a problem hiding this comment.
Nice work @A-Wheeto!
One thing to consider, although not a blocker at all, is that we're tying ourselves in to Sentry as an exception tracker by adding explicit calls to the Sentry gem throughout the job.
If we decided to migrate to another exception tracker in the future (e.g. Honeybadger) then we'd need to find and replace each instance of the call to the Sentry gem.
This problem is often solved by wrapping the calls to the external gem in a local wrapper, e.g. in the this case you could create an ExceptionTracker class with a capture_exception function which then calls Sentry.
With a local abstraction in place, we'd only need to update the ExceptionTracker class if we ever moved to a new provider or if Sentry made a breaking change to their gem's API.
This pattern is typically referred to as Dependency Inversion Principle.
Not necessarily suggesting that we need to implement this now, just a thought for the future.
|
Thanks @matoakley — really good point, and definitely something worth considering. I'll add a tech debt ticket to the backlog so we can revisit this down the line. Since there’s a chance the website could be handed off to another owner in the future (who might use a different exception tracker), it makes sense to maintain that layer of abstraction. |
Status
Review progress:
What's changed?
Steps to perform after deploying to production
If the production environment requires any extra work after this PR has been deployed detail it here. This could be running a Rake task, migrating a DB table, or upgrading a Gem. That kind of thing.