-
-
Notifications
You must be signed in to change notification settings - Fork 62
ref(error-tracking): format clickhouse errors better #7271
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
Conversation
🔍 Existing Issues For ReviewYour pull request is modifying functions with the following pre-existing issues: 📄 File: snuba/web/db_query.py
Did you find this useful? React with a 👍 or 👎 |
@@ -503,9 +502,6 @@ def _raw_query( | |||
elif isinstance(cause, ExecutionTimeoutError): | |||
status = QueryStatus.TIMEOUT | |||
|
|||
if request_status.slo == SLO.AGAINST: |
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.
We raise a QueryException
later and I think we just end up double reporting to Sentry - if there is a reason to keep this line lemme know
8d01662
to
45f418b
Compare
That's good to remove the stack trace from the message itself. Maybe we can still attach it to the error using https://develop.sentry.dev/sdk/data-model/event-payloads/exception/ ? Also, I would separate the error code into an attribute and not as part of the message itself. |
a long time ago i wrote this sdk integration to parse clickhouse stacktraces and stitch them as stackframes into the event: https://docs.sentry.io/platforms/python/integrations/gnu_backtrace/ it's activated here: Line 83 in bb6bf85
It worked for a while but I don't know if it works now. |
@untitaker @phacops okie so i spent some time updating the That being said, I still think the changes in this PR are reasonable since we don't need the clickhouse stack trace reported multiple times |
Cleans up how we record clickhouse errors in Sentry - we still will get the clickhouse stack trace, but we don't need to dump the whole stack trace in the error message itself
Before
After