-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Avoid reporting bad requests to Sentry #3454
Conversation
Does this include 404s? |
@@ -4,7 +4,7 @@ | |||
from redash import settings, __version__ | |||
|
|||
|
|||
NON_REPORTED_EXCEPTIONS = ['QueryExecutionError'] | |||
NON_REPORTED_EXCEPTIONS = ['BadRequest', 'QueryExecutionError'] |
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.
@arikfr nope, only 400s. If you want 404s as well, smash this:
NON_REPORTED_EXCEPTIONS = ['BadRequest', 'QueryExecutionError'] | |
NON_REPORTED_EXCEPTIONS = ['NotFound', 'BadRequest', 'QueryExecutionError'] |
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.
Only the author of the PR, can accept suggestions... Do you think we should keep reporting 404s there?
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.
The only thing I can think that we should keep 404s for is any manual construction of links in the frontend that might not pop up as a bug during development and testing.
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.
Two things about this:
- In dev/test we don't send Sentry reports anyway.
- This kind of issues should be tracked from the frontend.
So where are we with regards to this PR? Do we still want to keep 400s after all? |
I'm conflicted. On the one hand it feels to me that Sentry should receive only actionable issues. On the otherhand, it proved useful debugging a production issue 🤔 I will open a thread in Sentry's forum to see what they recommend. |
@rauchy , thanks for the PR! We've updated a lot of things now that we're Community-driven so - if you're still interested in getting this merged - would you mind rebasing off master to re-run the CI, as well as updating merge conflicts? We're trying to clean up our PR todo list, so if you're not interested, that's fine - we'll close the PR in about a week if we don't hear back. If you're interested in reopening the PR afterwards, we would also very much welcome that. |
Looking over that message on the Sentry forums, it seems like sending the extra data does have uses:
So, lets not merge this PR. We can revisit this topic later on if this turns out to be a bad move. 😁 |
No description provided.