Skip to content
This repository was archived by the owner on Jan 12, 2024. It is now read-only.

Add Opbeat.capture_rack_exception #27

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

teodor-pripoae
Copy link

Currently I'm handling all exceptions at application level in order to properly format them and return proper response codes (400 vs 500).

https://github.com/intridea/grape#exception-handling

Since the Opbeat::Rack middleware is put before my Grape middleware and since the Grape middleware is catching the errors and formatting them, the errors will never arrive in Opbeat::Rack middleware.

This PR adds a method named capture_rack_exception, which allows to also pass the rack environment.

I also changed how Opbeat::Rack sends errors, passing them through this method, also allowing for async behaviour.

This method was extracted from our codebase, we are using this method in production as a monkey patch for a few months.

@@ -28,16 +28,14 @@ def call(env)
rescue Error => e
raise # Don't capture Opbeat errors
rescue Exception => e
evt = Event.from_rack_exception(e, env)
Opbeat.send(evt)
Opbeat.capture_rack_exception(e, env)
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the PR :) Can you explain why you don't send the event anymore?

Copy link
Author

Choose a reason for hiding this comment

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

Becouse it is sent in the capture_rack_exception method. Also I think that original method didn't support async and also didn't support ignoring user defined errors. (note: it was also not checking if evt is nil)

Copy link
Author

Choose a reason for hiding this comment

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

Sorry If you got confused about removing the expect(Opbeat).to receive(:send).with(@event) lines from tests. I put them back since the original issue that was getting the specs to fail was from elsewhere.

The async tests were letting the configuration have async callback, and since the middleware now supports async behaviour, the specs needed change. I fixed the async specs to cleanup after finish, so now that changes are not needed anymore.

Copy link
Author

Choose a reason for hiding this comment

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

Also, maybe I didn't explain properly. This PR allows users to call the capture_rack_exception from their code (similar to capture_exception), allowing them to pass rack environment.

@teodor-pripoae
Copy link
Author

Hi,

Did you have a chance to review this ?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants