-
Notifications
You must be signed in to change notification settings - Fork 182
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
Don't format exceptions before logging #269
base: master
Are you sure you want to change the base?
Conversation
@ulgens thanks a lot for creating this PR, I ran into the same problem. |
d74150f
to
5ea3a88
Compare
@lucas-sonnabend The issue with test wasn't related to code, it was on Travis side. Tests are fixed on a second test run, but now coverall fails 🤕 |
LGTM 🚀 Don't worry about the code coverage, nothing has changed but coveralls calculation isn't always exact leading to slighty changes on its coverage report. |
@syrusakbary |
5ea3a88
to
56ca230
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has been a long standing issue. Thank you for the fix
56ca230
to
c93bc47
Compare
Would love to see this merged in, as right now we're forced to use Django middleware to bubble the actual exceptions up to Sentry, leaving us with duplicate Sentry issues (one with the big truncated string as shown in the OP, another with the actually useful information like the second one shown in the OP). We've considered hiding all exceptions that come through For anyone else encountering this in Django-land, our current workaround uses graphene-sentry to allow us to bubble exceptions up. Wrapping the default GraphQL view is pretty straightforward, but does lead to the aforementioned problem of duplicate Sentry issues if you don't block the |
@jkimbo 👋 |
Is there any way we can get this merged and release? I'm happy to help if it is needed 😃 |
Fixes #253
traceback.format_exception
is added toreport_error
with graphql-python/graphql-core#154 two years ago, it solves another problem but i couldn't understand why did anyone need to stringify all exceptions on such central place. It removes all details of an exception and downgrades is to simple string. That change makes crucial impact on error logging.Before the change:
All exceptions have same type and same title. No traceback. There are some autogenerated breadcrumbs thanks to Sentry, but it's impossible to find why and where this error is happening. Message is truncated. All details are lost.
After the change:
Exceptions have their own types. Message is short, clear and gives information. Traceback is on place. Breadcrumbs are useful.