Skip to content

Conversation

@robertclancy
Copy link
Contributor

ActionDispatch's ShowExceptions swallows exceptions and stores the cause
in a rack env variable "action_dispatch.exception". Use this to report
on any exceptions swallowed by ShowExceptions.

ActionDispatch's ShowExceptions swallows exceptions and stores the cause
in a rack env variable "action_dispatch.exception". Use this to report
on any exceptions swallowed by ShowExceptions.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you just change this line to say 'should capture rails errors when ActionDispatch::ShowExceptions is enabled'?

- PR feedback
@seanlinsley
Copy link
Contributor

This should be a setting. For example, we set rescue_from ActiveRecord::RecordNotFound globally in our app. We don't want those showing up in Sentry.

@robertclancy
Copy link
Contributor Author

Any exceptions rescued in a controller using rescue_from do not end up in the "action_dispatch.exception" env variable.

As far as I can tell, the only place that env variable is set is in the ShowExceptions middleware, which catches exceptions not rescued in a controller.

@nateberkopec
Copy link
Contributor

rescue_from doesn't use ShowExceptions. This LGTM.

nateberkopec added a commit that referenced this pull request Sep 13, 2015
…h_errors

Capture exceptions swallowed by rails
@nateberkopec nateberkopec merged commit 5b39a2c into getsentry:master Sep 13, 2015
@greysteil
Copy link
Contributor

This is causing some issues for us - our app specifies an exceptions_app which handles things like parameter parsing errors. We don't want those rescued exceptions to be reported to sentry - our app has already done the right thing and returned a 400 (for example).

My hunch is that the best way forward on this one is to make this a setting. Alternatively we could delete the action_dispatch.exception env variable once the exception has been handled by our exceptions app, but that feels unintuitive. @nateberkopec / @seanlinsley - any thoughts?

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.

4 participants