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
fixes: limit no authorization error for sentry #9816
fixes: limit no authorization error for sentry #9816
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9816 +/- ##
==========================================
- Coverage 66.16% 64.46% -1.71%
==========================================
Files 585 534 -51
Lines 30427 29283 -1144
Branches 3152 2810 -342
==========================================
- Hits 20133 18877 -1256
- Misses 10113 10228 +115
+ Partials 181 178 -3
Continue to review full report at Codecov.
|
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.
one question, it seems like this shouldn't be necessary to solve the issue you're having. however, if my proposed solution doesn't work, some more details about the exact apis that are triggering sentry alerts would be awesome
superset/views/base.py
Outdated
@@ -141,6 +141,9 @@ def api(f): | |||
def wraps(self, *args, **kwargs): | |||
try: | |||
return f(self, *args, **kwargs) | |||
except SupersetSecurityException as ex: |
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.
Shouldn't this be managed by also wrapping the function in handle_api_exception
as opposed to adding it here?
@etr2460 I misunderstood the issue. Changing to |
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.
cool, lgtm. Since this is a 4xx status code, I think logging a warning vs. an exception is probably better anyway.
That said, it might be worth looking into filtering these out within your Sentry integration. At some point we'll probably run into a case where we want to keep logging a certain way in open source, but your particular use case would want to ignore them
Agreed @etr2460 - we're taking this on a case-by-case basis. The PRs we've opened are for instances where we think the log level in Superset is wrong for what it's capturing, but we don't want to force changes on the community where it doesn't make sense for others. My belief is that we'll find more alignment than disagreement when improving log levels. |
* rescue no authorization error * update no authorization exception to warning
SUMMARY
We user sentry to help us keep tracking errors and fix crashes.
flask_jwt_extended/view_decorators.py
in_decode_jwt_from_request
atline 312
hasraise NoAuthorizationError(err_msg)
. Sentry reportsNoAuthorizationError
expectedly but this error is not an actual crash. We want filter error out to reduce noise. It will log a 401 instead of throwing error to sentry.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION