PB-511: Django request context in logs - #minor #62
Conversation
github-actions
bot
changed the title
benschs
force-pushed
the
feat-PB-511-django-request-log
branch
from
4da35d2 to
500c438
Compare
There was a problem hiding this comment.
Nicer approach, and simpler
Some improvements thoughts to make it more generic
- move the thread local into
logging_utilities/thread_context.py
# thread_context.py
from threading import local
class ThreadContext(local):
pass
thread_context = ThreadContext()
- Call the middleware
add_context.py
from thread_context import thread_context
def __call__(self, request):
thread_context.request = request
# do the processing
thread_context.request = None
- Call the filter
add_thread_context.pyNo django dependencies
from thread_context import thread_context
class AddThreadContext(logging.Filter):
# contexts := list of context key mapping between thread_context key and logger key, or use a list of dict {logger : "logger key", context: "context key"}
def __init__(self, contexts = []):
self.contexts = contexts
def filter(self, record):
for context in contexts:
if thread_context[context.context_key] != None:
setattr(record, context.logger_key, thread_context[context.context_key])
return True
This way we would have a new very generic logging filter that could be reused by any system that require any thread context. And we would have a django filter that could in future add other thread context values, not only request.
| def clear_thread_variable(): | ||
| REQUEST_VARS.__dict__.clear() |
There was a problem hiding this comment.
Why having a clear if we have a delete function. I would only keep delete. See also my other comment
| def __call__(self, request): | ||
| set_variable('request', request) | ||
| response = self.get_response(request) | ||
| clear_thread_variable() | ||
| return response | ||
|
|
||
| def process_exception(self, request, exception): | ||
| clear_thread_variable() |
There was a problem hiding this comment.
Here in clean up I would only remove the added key and not clear the whole local variable. This is basically the same but make code clearer and we could theoretically use the local variable for something else as well.
| def process_exception(self, request, exception): | ||
| clear_thread_variable() |
There was a problem hiding this comment.
IN process exception we need to make sure that the exception has been logged before by another system with the request infos. I'm not sure anymore how it worked on django, but did you verify that in case of an exception the exception is logged before you clear this variable ?
benschs
force-pushed
the
feat-PB-511-django-request-log
branch
from
500c438 to
97fcc36
Compare
|
@ltshb Thanks for all the input. That all makes sense to me and I have rewritten the parts. |
Middleware and filter that can be combined to add django request fields to all logs within the scope of the request. Middleware adds request object to a thread local variable. Filter adds the request from thread to log record. Existing json filter can be used to decide which request fields should be added to the log.
benschs
force-pushed
the
feat-PB-511-django-request-log
branch
from
97fcc36 to
ee99aea
Compare
Extent documentation for adding django requests to log records. Add descriptions of the django middleware and the filter to add thread variables to log records. Add a use case example in README to add django request context to all log records, by combining multiple components of this library.
Middleware and filter that can be combined to add django request fields to all logs within the scope of the request.
Middleware adds request object to a thread local variable. Filter adds the request from thread to log record. Existing json filter can be used to decide which request fields should be added to the log.