Skip to content

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

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

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .pylintrc
Original file line number Diff line number Diff line change
Expand Up @@ -276,6 +276,7 @@ function-naming-style=snake_case
good-names=i,
j,
k,
f,
ex,
fd,
Run,
Expand Down
31 changes: 31 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,9 @@ All features can be fully configured from the configuration file.
- [Usage](#usage)
- [Django Request Filter Constructor](#django-request-filter-constructor)
- [Django Request Config Example](#django-request-config-example)
- [Django request log records](#django-request-log-records)
- [Usage \& Configuration](#usage--configuration)
- [Configure the filter `DjangoAppendRequestFilter`](#configure-the-filter-djangoappendrequestfilter)
- [Filter out LogRecord attributes based on their types](#filter-out-logrecord-attributes-based-on-their-types)
- [Attribute Type Filter Constructor](#attribute-type-filter-constructor)
- [Attribute Type Filter Config Example](#attribute-type-filter-config-example)
Expand Down Expand Up @@ -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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe to each log records to be more precise


### Usage & Configuration

Add `logging_utilities.request_middleware.AddRequestToLogMiddleware` to the `settings.MIDDLEWARE` list.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


#### Configure the filter `DjangoAppendRequestFilter`

```yaml
filters:
django_request_meta:
(): logging_utilities.filters.django_append_request.DjangoAppendRequestFilter
request_attributes:
- path
- method
- META.QUERY_STRING
always_add: True
```

| 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). |
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the attribute is not jsonifiable ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

| `always_add` | bool | False | By default empty attributes are omitted. If set, empty attributes will be added with value `-` |



## Filter out LogRecord attributes based on their types

If different libraries or different parts of your code log different object types under the same
Expand Down
49 changes: 49 additions & 0 deletions logging_utilities/filters/django_append_request.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import functools
from logging import LogRecord
from typing import Optional

from django.core.handlers.wsgi import WSGIRequest


def r_getattr(obj, attr, *args):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for what for is the prefix r_ ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer be more verbose here request_getattr


def _getattr(obj, attr):
if isinstance(obj, dict):
return obj.get(attr)
return getattr(obj, attr, *args)

return functools.reduce(_getattr, [obj] + attr.split('.'))


class DjangoAppendRequestFilter():
"""Logging Django request attributes

This filter adds Django request context attributes to the log record.
"""

def __init__(
self, request: Optional[WSGIRequest] = None, request_attributes=None, always_add=False
):
"""Initialize the filter

Args:
request_attributes: (WSGIRequest | None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be request here instead of request_attributes

Request from which to read the attributes from.
attributes: (list | None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here you are not consistent between docu and code, docu says attributes and code says request_attributes, I prefer the docu 😉

Request attributes that should be added to log entries
always_add: bool
Always add attributes even if they are missing. Missing attributes with have the
value "-".
"""
self.request = request
self.attributes = request_attributes if request_attributes else list()
self.always_add = always_add

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


return True
115 changes: 115 additions & 0 deletions logging_utilities/request_middleware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import logging
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

from typing import Any
from typing import Callable
from typing import Optional
from typing import Type
from typing import TypeVar

from django.core.handlers.wsgi import WSGIRequest

from logging_utilities.filters.django_append_request import \
DjangoAppendRequestFilter

# Create a generic variable that can be 'WrappedRequest', or any subclass.
T = TypeVar('T', bound='WrappedRequest')


class WrappedRequest(WSGIRequest):
"""WrappedRequest adds the 'logging_filter' field to a standard request to track it.
"""

@classmethod
def from_parent(
cls: Type[T], parent: WSGIRequest, logging_filter: Optional[DjangoAppendRequestFilter]
) -> T:
return cls(parent.environ, logging_filter)

def __init__(self, environ: Any, logging_filter: Optional[DjangoAppendRequestFilter]) -> None:
super().__init__(environ)
self.logging_filter = logging_filter


class AddRequestToLogMiddleware():
"""Middleware that adds a logging filter *DjangoAppendRequestFilter* to the request.
"""

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


def __call__(self, request: WSGIRequest) -> Any:
w_request = WrappedRequest.from_parent(request, None)
response = self.process_request(w_request)
if not response:
response = self.get_response(w_request)
response = self.process_response(w_request, response)

return response

def _find_loggers(self) -> dict[str, logging.Logger]:
"""Return loggers part of root.
"""
result: dict[str, logging.Logger] = {}
prefix = self.root + "."
for name, log in logging.Logger.manager.loggerDict.items():
if not isinstance(log, logging.Logger) or not name.startswith(prefix):
continue # not under self.root
result[name] = log
# also add root logger
result[self.root] = logging.getLogger(self.root)
return result

def _find_handlers(self) -> list[logging.Handler]:
"""List handlers of all loggers
"""
handlers = set()
for logger in self._find_loggers().values():
for handler in logger.handlers:
handlers.add(handler)
return list(handlers)

def _find_handlers_with_filter(self, filter_cls: type) -> dict[logging.Handler, list[Any]]:
"""Dict of handlers mapped to their filters.
Only include handlers that have at least one filter of type *filter_cls*.
"""
result = {}
for logger in self._find_handlers():
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here should it not be handler instead of logger?

attrs = []
for f in logger.filters:
if isinstance(f, filter_cls):
attrs.extend(f.attributes)
if attrs:
result[logger] = attrs
return result

def _add_filter(self, f: DjangoAppendRequestFilter) -> None:
"""Add the filter to relevant handlers.
Relevant handlers are once that already include a filter of the same type.
This is how we "overrite" the filter with the current request.
"""
filter_cls = type(f)
for handler, attrs in self._find_handlers_with_filter(filter_cls).items():
f.attributes = attrs
handler.addFilter(f)

def process_request(self, request: WrappedRequest) -> Any:
"""Add a filter that includes the current request. Add the filter to the request to be
removed again later.
"""
request.logging_filter = DjangoAppendRequestFilter(request)
self._add_filter(request.logging_filter)
return self.get_response(request)

def _remove_filter(self, f: DjangoAppendRequestFilter) -> None:
"""Remove the filter from any handlers that may have it.
"""
filter_cls = type(f)
for handler in self._find_handlers_with_filter(filter_cls):
handler.removeFilter(f)

def process_response(self, request: WrappedRequest, response: Any) -> Any:
"""Remove the filter if set.
"""
if request.logging_filter:
self._remove_filter(request.logging_filter)
return response
3 changes: 2 additions & 1 deletion tests/test_django_attribute.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,8 @@
else:
dictionary = OrderedDict

settings.configure()
if not settings.configured:
settings.configure()

logger = logging.getLogger(__name__)

Expand Down
96 changes: 96 additions & 0 deletions tests/test_django_request.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import json
import logging
import sys
import unittest
from collections import OrderedDict

from django.conf import settings
from django.test import RequestFactory

from logging_utilities.filters.django_append_request import \
DjangoAppendRequestFilter
from logging_utilities.formatters.json_formatter import JsonFormatter

# From python3.7, dict is ordered
if sys.version_info.major >= 3 and sys.version_info.minor >= 7:
dictionary = dict
else:
dictionary = OrderedDict

if not settings.configured:
settings.configure()

logger = logging.getLogger(__name__)


class DjangoAppendRequestFilterTest(unittest.TestCase):

def setUp(self) -> None:
self.factory = RequestFactory()

@classmethod
def _configure_django_filter(cls, _logger, django_filter):
_logger.setLevel(logging.DEBUG)
for handler in _logger.handlers:
handler.setFormatter(JsonFormatter(add_always_extra=True))
handler.addFilter(django_filter)

def test_django_request_log(self):
request = self.factory.get("/some_path?test=some_value")
with self.assertLogs('test_formatter', level=logging.DEBUG) as ctx:
test_logger = logging.getLogger("test_formatter")
self._configure_django_filter(
test_logger,
DjangoAppendRequestFilter(
request, request_attributes=["path", "method", "META.QUERY_STRING"]
)
)

test_logger.debug("first message")
test_logger.info("second message")

message1 = json.loads(ctx.output[0], object_pairs_hook=dictionary)
self.assertDictEqual(
message1,
dictionary([("levelname", "DEBUG"), ("name", "test_formatter"),
("message", "first message"), ("request.method", "GET"),
("request.path", "/some_path"),
("request.META.QUERY_STRING", "test=some_value")])
)
message2 = json.loads(ctx.output[1], object_pairs_hook=dictionary)
self.assertDictEqual(
message2,
dictionary([("levelname", "INFO"), ("name", "test_formatter"),
("message", "second message"), ("request.method", "GET"),
("request.path", "/some_path"),
("request.META.QUERY_STRING", "test=some_value")])
)

def test_django_request_log_always_add(self):
request = self.factory.get("/some_path?test=some_value")
with self.assertLogs('test_formatter', level=logging.DEBUG) as ctx:
test_logger = logging.getLogger("test_formatter")
self._configure_django_filter(
test_logger,
DjangoAppendRequestFilter(
request, request_attributes=["does", "not", "exist"], always_add=True
)
)

test_logger.debug("first message")
test_logger.info("second message")

message1 = json.loads(ctx.output[0], object_pairs_hook=dictionary)
self.assertDictEqual(
message1,
dictionary([("levelname", "DEBUG"), ("name", "test_formatter"),
("message", "first message"), ("request.does", "-"), ("request.not", "-"),
("request.exist", "-")])
)
message2 = json.loads(ctx.output[1], object_pairs_hook=dictionary)
self.assertDictEqual(
message2,
dictionary([("levelname", "INFO"), ("name", "test_formatter"),
("message", "second message"), ("request.does", "-"), ("request.not", "-"),
("request.exist", "-")])
)
62 changes: 62 additions & 0 deletions tests/test_request_middleware.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
import json
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe rename to test_django_request_middleware.py

import logging
import sys
import unittest
from collections import OrderedDict

from django.conf import settings
from django.test import RequestFactory

from logging_utilities.filters.django_append_request import \
DjangoAppendRequestFilter
from logging_utilities.formatters.json_formatter import JsonFormatter
from logging_utilities.request_middleware import AddRequestToLogMiddleware

# From python3.7, dict is ordered
if sys.version_info.major >= 3 and sys.version_info.minor >= 7:
dictionary = dict
else:
dictionary = OrderedDict

if not settings.configured:
settings.configure()

logger = logging.getLogger(__name__)


class AddRequestToLogMiddlewareTest(unittest.TestCase):

def setUp(self) -> None:
self.factory = RequestFactory()

@classmethod
def _configure_django_filter(cls, _logger):
for handler in _logger.handlers:
handler.setFormatter(JsonFormatter(add_always_extra=True))
django_filter = DjangoAppendRequestFilter(
request_attributes=["method", "path", "META.QUERY_STRING", "headers"]
)
handler.addFilter(django_filter)

def test_request_log(self):

def test_handler(request):
logger.info("test message")
return "some response"

with self.assertLogs(logger, level=logging.DEBUG) as ctx:
self._configure_django_filter(logger)
my_header = {"HTTP_CUSTOM_KEY": "VALUE"}
request = self.factory.get("/some_path?test=some_value", **my_header)
middleware = AddRequestToLogMiddleware(test_handler, root="tests")
middleware(request)

message1 = json.loads(ctx.output[0], object_pairs_hook=dictionary)
print(message1)
self.assertDictEqual(
message1,
dictionary([("levelname", "INFO"), ("name", __name__), ("message", "test message"),
("request.method", "GET"), ("request.path", "/some_path"),
("request.META.QUERY_STRING", "test=some_value"),
('request.headers', "{'Cookie': '', 'Custom-Key': 'VALUE'}")])
)