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

WIP: Fix Rails multi-exception reports #422

Merged

Conversation

nateberkopec
Copy link
Contributor

No description provided.

@nateberkopec
Copy link
Contributor Author

ping @mattrobenolt

This fixes the issue for the app you provided in #388, and, as far as I can tell, doesn't break any additional use cases. Can you take a look?

@nateberkopec
Copy link
Contributor Author

Can also confirm this works correctly with rescue_from - errors which are not re-raised by rescue_from are not reported.

@greysteil
Copy link
Contributor

👍 ❤️

@mattrobenolt
Copy link
Contributor

I tested this locally, and it seems to record the wrong one now.

It only logged one event, but it only logged the event wiht the url of /500, not the original url. So seems like we're filtering out the wrong one.

@mattrobenolt
Copy link
Contributor

For posterity, I ran against my docker container:

$ docker run --rm --name=sentry-test -e RAILS_ENV=production -e SENTRY_DSN=https://xxx:xxx@app.getsentry.com/3825 -p 3000:3000 -v $PWD:/usr/src/app -v /Users/matt/code/getsentry/raven-ruby:/usr/local/bundle/gems/sentry-raven-0.15.2 -it sentry-test

And mounted in this PR.

Without mounting this in, it was recording the error twice. Now only records once, but the wrong one.

nateberkopec added a commit that referenced this pull request Jan 6, 2016
@nateberkopec nateberkopec merged commit 2a23c63 into getsentry:master Jan 6, 2016
@nateberkopec
Copy link
Contributor Author

For posterity: I worked this out with @mattrobenolt and made sure the right exception was recorded.

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

Successfully merging this pull request may close these issues.

3 participants