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

Don't format exceptions before logging #269

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ulgens
Copy link

@ulgens ulgens commented Mar 8, 2020

Fixes #253

traceback.format_exception is added to report_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:
image
image

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:

image
image

Exceptions have their own types. Message is short, clear and gives information. Traceback is on place. Breadcrumbs are useful.

@lucas-sonnabend
Copy link

@ulgens thanks a lot for creating this PR, I ran into the same problem.
I'm pretty sure to get the tests to pass you have to fix the code in graphql/execution/tests/test_executor_thread.py

@ulgens ulgens force-pushed the fix_exception_logging branch 2 times, most recently from d74150f to 5ea3a88 Compare March 22, 2020 03:11
@ulgens
Copy link
Author

ulgens commented Mar 22, 2020

@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 🤕

@KingDarBoja
Copy link
Contributor

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.

@lucas-sonnabend
Copy link

@syrusakbary
@jhgg
@dittos
as you are the main contributors according to the readme, what are the next steps to get this approved, merged and released?

Copy link
Member

@mvanlonden mvanlonden left a 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

@mvanlonden mvanlonden requested a review from jkimbo May 13, 2020 18:22
@ulgens ulgens force-pushed the fix_exception_logging branch from 56ca230 to c93bc47 Compare May 17, 2020 17:45
@DylanBohlender
Copy link

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 graphql.execution.utils from Sentry, but since there's a probability that this will be fixed (since this PR exists), we'll hang tight on that for now and hope to hear news that this makes its way into core.

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 graphql.execution.utils logger.

@ulgens
Copy link
Author

ulgens commented Jul 13, 2020

@jkimbo 👋

@valberg
Copy link

valberg commented Sep 9, 2020

Is there any way we can get this merged and release? I'm happy to help if it is needed 😃

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.

Improve log errors
6 participants