-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add the ability to opt into logging on a per-endpoint basis #103
Conversation
Also, expose conditionals that will by default opt all endpoints into being logged. Note one change, responses can now be logged without an actual request being logged.
request_logging/middleware.py
Outdated
if because is not None: | ||
return self._skip_logging_request(request, because) | ||
should_log_route = self._should_log_route(request) | ||
if not should_log_route.log_route: |
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.
I realize this mirrors the old code, but why the nested if
s instead of one and
? Also, it seems like we should dump out in either case so it should be an or
(and maybe we provide an empty string if there's no skip_reason
).
request_logging/middleware.py
Outdated
if should_log_response: | ||
# Either the response is opted-in to logging by default or we've | ||
# conditionally selected the response to be logged. Regardless, log it! | ||
if 400 <= response.status_code < 500: |
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.
Please break this out into its own function,
By default, do not log a response if a request is skipped from being logged as well.
@tclancy apologies for this taking so long to get back to, I've been in the middle of a move and unable to get to this. I think I've addressed your feedback though (and hoping that tests all pass of course) |
def _should_log_route(self, request): | ||
func = self._get_api_func(request) | ||
if func in OPT_INTO_LOGGING_FUNCS: | ||
return ShouldLogRoute(log_route=True, skip_reason=None, silent=False) | ||
|
||
if func in NO_LOGGING_FUNCS: | ||
no_logging = NO_LOGGING_FUNCS[func] | ||
return ShouldLogRoute(log_route=False, skip_reason=no_logging.message, silent=no_logging.silent) | ||
|
||
return ShouldLogRoute(log_route=self.logging_opt_in_defaults[REQUEST](request), skip_reason=None, silent=False) |
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.
I couldn't really come up with a better way to handle the silent case than propagating a flag forward. It definitely feels a little leaky but it works with current behavior
Closing this because it's super old and I don't have the time to update this 😢 |
Also, expose conditionals that will by default opt all endpoints into
being logged.
Note one change, responses can now be logged without an actual request
being logged.
This may be a little more opinionated than needed but I personally prefer not using an attribute on a view method as a flag on whether or not to log the method. For newer versions of Python dictionaries are fairly compact so I think storing methods that we don't want logged / want to opt into logging is a reasonable thing to do (and I think it makes the code a lot cleaner)