PB-511: Django request context in logs - #minor #61

benschs · 2024-08-26T15:10:04Z

Add django request context to logs by filter configuration and middleware. Middleware adds the filter with the current request to the log handlers. Any logs will have the request information also added to the log record.

Test case for the option to always add the request attributes in the django filter.

ltshb · 2024-08-27T09:53:22Z

README.md

+
+### Usage & Configuration
+
+Add `logging_utilities.request_middleware.AddRequestToLogMiddleware` to the `settings.MIDDLEWARE` list.


Maybe be a bit more precise and add a django config example.

ltshb · 2024-08-27T09:53:48Z

README.md

@@ -507,6 +510,34 @@ filters:

 **NOTE**: `JsonDjangoRequest` only support the special key `'()'` factory in the configuration file (it doesn't work with the normal `'class'` key).

+## Django request log records
+
+To add context information from the current request to log records. For this the filter `DjangoAppendRequestFilter` must be added as well as the middleware `AddRequestToLogMiddleware`.


maybe to each log records to be more precise

ltshb · 2024-08-27T09:54:59Z

README.md

+
+| Parameter    | Type | Default | Description                                    |
+|--------------|------|---------|------------------------------------------------|
+| `attributes` | list | None    | All request attributes that match any of the dotted keys of the list will be added to the jsonifiable object. When `None` then no attributes are added (default behavior). |


Mismatch between example and docu, is it request_attributes or attributes ?

What if the attribute is not jsonifiable ?

I change between request_attributes and attributes during development. It should now be attributes consistently.

ltshb · 2024-08-27T09:56:44Z

logging_utilities/filters/django_append_request.py

+from django.core.handlers.wsgi import WSGIRequest
+
+
+def r_getattr(obj, attr, *args):


for what for is the prefix r_ ?

I would prefer be more verbose here request_getattr

ltshb · 2024-08-27T09:58:42Z

logging_utilities/filters/django_append_request.py

+        """Initialize the filter
+
+        Args:
+            request_attributes: (WSGIRequest | None)


should be request here instead of request_attributes

ltshb · 2024-08-27T10:02:07Z

logging_utilities/filters/django_append_request.py

+        request = self.request
+        for attr in self.attributes:
+            val = r_getattr(request, attr, "-")
+            if self.always_add or val != "-":
+                setattr(record, "request." + attr, val)


Should we not do this only if we have a request context ? meaning checking that self.request is not none

If there is no request, we only add the value "-" if always_add is explicitly set, so this is fine for me as it is.
I added a test case that explicitly sets None as request to demonstrate the behavior.

ltshb · 2024-08-27T10:04:20Z

logging_utilities/request_middleware.py

@@ -0,0 +1,115 @@
+import logging


I would put this in a new module django_middlewares/request_middleware.py

ltshb · 2024-08-27T12:38:16Z

logging_utilities/request_middleware.py

+    def __init__(self, get_response: Callable[[WSGIRequest], Any], root: str = ""):
+        self.root = root
+        self.get_response = get_response


Might be a good idea to call root rootLogger to make the code easier to read/understand

ltshb · 2024-08-27T12:40:59Z

logging_utilities/request_middleware.py

+        Only include handlers that have at least one filter of type *filter_cls*.
+        """
+        result = {}
+        for logger in self._find_handlers():


Here should it not be handler instead of logger?

ltshb · 2024-08-27T12:48:36Z

tests/test_request_middleware.py

@@ -0,0 +1,62 @@
+import json


Maybe rename to test_django_request_middleware.py

Rename files and variables for better readability and understanding.

boecklic

Nice! Do you have an idea about possible performance impact of this solution? Since it's doing the processing for every request.

boecklic · 2024-08-27T21:29:03Z

logging_utilities/filters/django_append_request.py

+        self.always_add = always_add
+
+    def filter(self, record: LogRecord) -> bool:
+        request = self.request


in __init__ the WSGIRequest is marked as optional and can be None if I understand correctly, so wouln't you need to check for None here and only continue if not None?

boecklic · 2024-08-27T21:43:53Z

README.md

@@ -507,6 +509,44 @@ filters:

 **NOTE**: `JsonDjangoRequest` only support the special key `'()'` factory in the configuration file (it doesn't work with the normal `'class'` key).

+## Django request log records
+
+To add context information from the current request to each log record. For this the filter `DjangoAppendRequestFilter` must be added as well as the middleware `AddRequestToLogMiddleware`.


maybe you could be a little more verbose about the basic idea behind your solution with middleware and filter. With some not very deep understanding of how logging filters work this is not immediately obvious.

boecklic · 2024-08-27T21:44:00Z

logging_utilities/django_middlewares/request_middleware.py

+        if not response:
+            response = self.get_response(w_request)


I'm not sure I understand this part: the process_request method already does a return self.get_response(request), so if that return doesn't evaluate to sth true, why should calling it again result in a different outcome?

boecklic · 2024-08-27T21:51:12Z

logging_utilities/django_middlewares/request_middleware.py

+        return response
+
+    def _find_loggers(self) -> dict[str, logging.Logger]:
+        """Return loggers part of root.


a little more information about what you do here, i.e. what root_logger is about would be helpful. This is not directly obvious also from django middleware doc I think

ltshb

Could you verify if the filter approach is working in a multithread environment ? As discussed yesterday the logging loggers instance and filters are globals and it might be an issue with mutlithread environement.

benschs · 2024-08-28T14:26:35Z

Could you verify if the filter approach is working in a multithread environment ? As discussed yesterday the logging loggers instance and filters are globals and it might be an issue with mutlithread environement.

I could verify that this approach does not work in a multithreaded environment.
I will close this PR and open a new one with another approach of using thread variables.

PB-511: Django request context in logs

1241167

Add django request context to logs by filter configuration and middleware. Middleware adds the filter with the current request to the log handlers. Any logs will have the request information also added to the log record.

benschs requested review from boecklic and ltshb August 26, 2024 15:10

github-actions bot added the feature label Aug 26, 2024

github-actions bot changed the title ~~PB-511: Django request context in logs~~ PB-511: Django request context in logs - #minor Aug 26, 2024

PB-511: Add test case for django filter

f5c44cb

Test case for the option to always add the request attributes in the django filter.

ltshb reviewed Aug 27, 2024

View reviewed changes

PB-511: Refactor django request context

5e7a358

Rename files and variables for better readability and understanding.

benschs requested a review from ltshb August 27, 2024 14:02

boecklic approved these changes Aug 27, 2024

View reviewed changes

ltshb requested changes Aug 28, 2024

View reviewed changes

benschs closed this Aug 28, 2024

benschs deleted the feat-PB-511-django-request-log branch August 29, 2024 09:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PB-511: Django request context in logs - #minor #61

PB-511: Django request context in logs - #minor #61

benschs commented Aug 26, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

benschs Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

benschs Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

ltshb Aug 27, 2024

boecklic left a comment

boecklic Aug 27, 2024

boecklic Aug 27, 2024

boecklic Aug 27, 2024

boecklic Aug 27, 2024

ltshb left a comment

benschs commented Aug 28, 2024


		### Usage & Configuration

		Add `logging_utilities.request_middleware.AddRequestToLogMiddleware` to the `settings.MIDDLEWARE` list.

		from django.core.handlers.wsgi import WSGIRequest


		def r_getattr(obj, attr, *args):

PB-511: Django request context in logs - #minor #61

PB-511: Django request context in logs - #minor #61

Conversation

benschs commented Aug 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boecklic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ltshb left a comment

Choose a reason for hiding this comment

benschs commented Aug 28, 2024