Fix - Bypass unnecessary logs on logging out. #11479
Conversation
|
|
||
| @staticmethod | ||
| def _validate_cookie_params(session_id, user_id): | ||
| def _validate_cookie_params(session_id, user_id, from_logout=None): |
There was a problem hiding this comment.
should this default to False?
Qubad786
changed the title
|
jenkins run quality |
|
lgtm 👍 ; after tests pass |
There was a problem hiding this comment.
Can we consider an alternative implementation that doesn't require passing a usage-specific is_from_logout boolean parameter around? I would like to keep this module from not having a baggage of booleans passed around (per Clean Code book etiquette).
One idea: create a context manager for controlling the logging in this middleware. And then, when is_from_logout is true, set the logging context manager logging level to Error: logging.basicConfig(level='logging.ERROR')
There was a problem hiding this comment.
Also, are you disabling the following log message as well? I didn't see the code for that.
middleware.py:371 - SafeCookieData user at request '5' does not match user in session: 'None'
We would need to change it to a Warning level instead of an Error level - since it's an expected case for logouts.
There was a problem hiding this comment.
@nasthagiri Its being skipped here. Setting the logging level is a great idea, I will be doing that! I have to change the specified log to warning level as well.
There was a problem hiding this comment.
Ah. Thanks. I now see there were 2 logs within the same if statement. Thanks.
Qubad786
force-pushed
the
mrehan/plat-998-warnings-on-logout
branch
2 times, most recently
from
365dbfe to
ae3f7f2
Compare
|
@nasthagiri may you please take a look. Thanks! |
|
|
||
| finally: | ||
| if logger.getEffectiveLevel() == ERROR: | ||
| logger.setLevel(INFO) |
There was a problem hiding this comment.
hmm.. is there a way to determine what the original logging level was so it gets reset back to the original value. Here, we are assuming that it was configured to INFO. But that may not always be the case.
There was a problem hiding this comment.
Actually, I have confirmed while debugging, this was set to INFO when got from getLogger(__name__) by default.
There was a problem hiding this comment.
If that's not feasible we can store the original logging level after getting logger through getLogger(__name__) to restore back later?
There was a problem hiding this comment.
I believe you should be able to retrieve the original logging level by calling getLogger(__name__).getEffectiveLevel.
|
Thanks @Qubad786. Looking better. A few more comments. |
| the request is from logout. | ||
| """ | ||
| if _is_from_logout(request): | ||
| logger.setLevel(ERROR) |
There was a problem hiding this comment.
sorry, why do we want to log this as an error? Don't we not want to log this at all?
There was a problem hiding this comment.
The idea was to increase the threshold of our logging level from WARNING to ERROR. This way, only actual errors get logged in the logout case, and not expected warnings.
Qubad786
force-pushed
the
mrehan/plat-998-warnings-on-logout
branch
5 times, most recently
from
2554509 to
79e42f1
Compare
|
@adampalay , @nasthagiri please have a look. Thanks! |
| has 'is_from_log_out' attribute set to true. | ||
| """ | ||
| response = self.client.get(reverse('logout')) | ||
| self.assertEqual(getattr(response.wsgi_request, 'is_from_logout', False), True) # pylint: disable=no-member |
There was a problem hiding this comment.
You can use self.assertTrue too
There was a problem hiding this comment.
Agree. assertTrue will give a better error message when it fails.
|
👍 from me |
|
👍 awesomeness. thanks! |
Qubad786
force-pushed
the
mrehan/plat-998-warnings-on-logout
branch
from
79e42f1 to
faf3a64
Compare
|
jenkins run bokchoy |
Fix - Bypass unnecessary logs on logging out.
…at the user/session change is expected
This is intended to silence a rare false positive that seems to happen when someone logs in on a browser that already has an active session for another user. We believe there should be no further positives once this case is handled. - login and logout views annotate the response to indicate the session user should be changing between the request and response phases - safe-sessions middleware skips the verify-user check when this annotation is present Also: - Adds a test around existing behavior for unexpected user-changes - Remove logging control based on `is_from_log_out`. This reverts most of af9e26f/PR #11479 for two reasons: - The safe-sessions `_verify_user` code has since changed to check for `request.user.id == None` - A commit later in the PR changes the login and logout pages to signal that the user/session change is expected
4 participants
PLAT-998
Background:
There were unnecessary warning and error logs when user legitimately logs out from LMS which were causing overall tracking logs noisy.
Fix:
I have fixed this by setting an attribute
is_from_logouttorequestobject in logout view and checking to see whether the response is coming from logout in safe session middleware.@adampalay, @mushtaqak Please have a look.
Thanks