graphql-python / graphql-core-legacy

GraphQL base implementation for Python (legacy version – see graphql-core for the current one)
MIT License
374 stars 184 forks source link

Don't format exceptions before logging #269

Open ulgens opened 4 years ago

ulgens commented 4 years ago

Fixes #253

traceback.format_exception is added to report_error with https://github.com/graphql-python/graphql-core/pull/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 commented 4 years ago

@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 commented 4 years ago

@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 :face_with_head_bandage:

KingDarBoja commented 4 years ago

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 commented 4 years ago

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

DylanBohlender commented 4 years ago

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 commented 4 years ago

@jkimbo :wave:

valberg commented 4 years ago

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