gh-92592 : allow logging filters to return a LogRecord #92591

adriangb · 2022-05-10T02:46:14Z

https://discuss.python.org/t/expand-logging-filter-api-to-allow-returning-a-logrecord

vsajip

Thanks for this PR.

vsajip · 2022-05-10T13:32:41Z

Doc/howto/logging-cookbook.rst

@@ -713,6 +713,41 @@ which, when run, produces something like:
    2010-09-06 22:38:15,301 d.e.f DEBUG    IP: 123.231.231.123 User: fred     A message at DEBUG level with 2 parameters
    2010-09-06 22:38:15,301 d.e.f INFO     IP: 123.231.231.123 User: fred     A message at INFO level with 2 parameters

+Imparting contextual in Handlers


Should be contextual information rather than just contextual. Also, no need to capitalise handlers here, as we're not talking about the class.

vsajip · 2022-05-10T13:32:41Z

Doc/howto/logging-cookbook.rst

+Imparting contextual in Handlers
+---------------------------------
+
+Each :class:`~Handler` has it's own chain of Filters.


it's should be its - the former is only used as a contraction of it is. Also, no need to capitalise filters here.

vsajip · 2022-05-10T13:32:41Z

Doc/howto/logging-cookbook.rst

+
+Each :class:`~Handler` has it's own chain of Filters.
+If you want to add contextual information to a :class:`LogRecord` without leaking
+this contextual information to other handlers, you can use a filter that returns


You could shorten this contextual information to just it.

Doc/howto/logging-cookbook.rst

vsajip · 2022-05-10T13:32:41Z

Doc/library/logging.rst

@@ -663,8 +663,9 @@ empty string, all events are passed.
   .. method:: filter(record)

      Is the specified record to be logged? Returns zero for no, nonzero for


After the change, this should be `a 'falsy' value for no, and a 'truthy' value for yes.

vsajip · 2022-05-10T13:32:41Z

Doc/library/logging.rst

@@ -686,6 +687,12 @@ which has a ``filter`` method with the same semantics.
   parameter. The returned value should conform to that returned by
   :meth:`~Filter.filter`.

+.. versionchanged:: 3.12
+   You can now return a :class:`LogRecord` instance from filters to replace
+   the log record without modifying it in place. This allows filters installed


installed on -> attached to

vsajip · 2022-05-10T13:32:41Z

Lib/logging/__init__.py

-        this and the record is then dropped. Returns a zero value if a record
-        is to be dropped, else non-zero.
+        this by returning a falsy value  and the record is then dropped and
+        this method returns a falsy value.


Isn't this now repetition which isn't needed?

I think it's worth differentiating between the return values of filters and the return value of the Filterer.filter function. The main difference is that filters may return a truthy value that is not a log record but Filterer.filter will now always return a log record (or falsy). I reworded it a bit to make these two different paragraphs.

vsajip · 2022-05-10T13:32:41Z

Lib/logging/__init__.py

-        is to be dropped, else non-zero.
+        this by returning a falsy value  and the record is then dropped and
+        this method returns a falsy value.
+        Filters can return a log record, which case that log record


Change sentences to If a filter attached to a handler returns a log record instance, then that instance is used in place of the original log record in any further processing of the event by that handler. If a filter returns any other truthy value, the original log record is used in any further processing of the event by that handler.

Thank you that does sound better

vsajip · 2022-05-10T13:32:41Z

Lib/test/test_logging.py

@@ -467,6 +467,65 @@ def log_at_all_levels(self, logger):
        for lvl in LEVEL_RANGE:
            logger.log(lvl, self.next_message())

+    def test_handler_filter_replaces_record(self):
+        def replace_message(record: logging.LogRecord):
+            return logging.LogRecord(


Use copy.copy() here and elsewhere instead of this.

vsajip · 2022-05-10T13:32:41Z

Misc/NEWS.d/next/Library/2022-05-09-22-27-11.gh-issue-92591.V7RCk2.rst

@@ -0,0 +1,3 @@
+Let :mod:`logging` Filters to return a :class:`logging.LogRecord` instance


No need to capitalise filters and handlers here, and also either use Allow filters to return ... or Let filters return ..., as Let ... to ... would be grammatically incorrect. Also, strictly, it would be more accurate to say ... so that filters attached to handlers can enrich ...

bedevere-bot · 2022-05-10T13:32:45Z

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

…into logging-filters

adriangb · 2022-05-10T14:22:00Z

Thank you for the quick an in-depth review @vsajip, that was great feedback.

I think using copy.copy resolves one of the two things I didn't like about this (the very verbose copying).

How do you feel about the interaction with filters on logging.Logger?
Since the handlers and loggers share the same implementation we have to do both.
As far as I can tell this change has no benefits for those filters, but also has no downsides.
I added a test that confirms this change "works" for logger filters, but it is pretty contrived and not representative of real world usage IMO.

vsajip · 2022-05-10T14:40:06Z

Since the handlers and loggers share the same implementation we have to do both.

Well, that shared Filterer implementation could take one of two paths - if instance(self, Handler) -> the new path, else the old (simpler) path.

adriangb · 2022-05-10T14:50:17Z

I'm not as concerned about the implementation (it's really not any more complex) as I am about the explainability. Doing something like that sounds like a leaky implementation detail. For example, you would not be able to use the same filter on a handler and a logger and we would have to explain why.

If there are no cons to letting this also work for logger filters, I think we should just do that, even if it doesn't enable any new functionality.

vsajip · 2022-05-10T15:16:51Z

No cons from my point of view. I was just thinking out loud 😄

gpshead · 2022-05-10T18:12:28Z

The Azure pipelines failure suggests whitespace issues in a couple files. Try a make patchcheck

adriangb · 2022-05-10T19:03:15Z

Thank you @gpshead, I was wondering why that was failing

vsajip · 2022-05-10T20:38:50Z

Doc/library/logging.rst

+.. versionchanged:: 3.12
+   You can now return a :class:`LogRecord` instance from filters to replace
+   the log record without modifying it in place. This allows filters attached to
+   on a :class:`Handler` to modify the log record before it is emitted without


attached to a handler, not attached to on a handler.

thank you, corrected

vsajip · 2022-05-10T20:38:50Z

Doc/library/logging.rst

+      Is the specified record to be logged? Returns falsy for no, truthy for
+      yes. Filters can either modify log records in-place or return a completely
+      different record instance which will replace the original
+      log record without modifying it.


I think the without modifying it should be replaced by in any future processing of the event.

vsajip · 2022-05-10T20:38:50Z

Doc/library/logging.rst

@@ -686,6 +687,12 @@ which has a ``filter`` method with the same semantics.
   parameter. The returned value should conform to that returned by
   :meth:`~Filter.filter`.

+.. versionchanged:: 3.12
+   You can now return a :class:`LogRecord` instance from filters to replace
+   the log record without modifying it in place. This allows filters attached to


without -> , rather than

vsajip · 2022-05-10T20:38:50Z

Misc/NEWS.d/next/Library/2022-05-09-22-27-11.gh-issue-92591.V7RCk2.rst

@@ -0,0 +1,3 @@
+Let :mod:`logging` filters to return a :class:`logging.LogRecord` instance


This still says Let ... to which is incorrect. Did you forget to make this change, which I suggested earlier?

yes, it looks like I missed that part of the comment, thank you for bringing it back up and for your patience

allow logging filters to return a LogRecord

3aeeba6

bedevere-bot added the awaiting review label May 10, 2022

adriangb added 5 commits May 10, 2022

edit docstring

1e4d27f

remove unecessary __bool__ impl

b526a35

edit docs

f1ea183

Add docs and blurb

19561d3

Merge branch 'main' into logging-filters

14e704d

adriangb changed the title ~~allow logging filters to return a LogRecord~~ gh-92592 : allow logging filters to return a LogRecord May 10, 2022

adriangb added 5 commits May 10, 2022

fix rst

4d34540

touch up wording

a69e76a

add test for filters on loggers

67453f6

push straggler change

b1e1084

Merge branch 'main' into logging-filters

f72c594

vsajip requested changes May 10, 2022

View changes

bedevere-bot removed the awaiting review label May 10, 2022

bedevere-bot added the awaiting changes label May 10, 2022

adriangb added 3 commits May 10, 2022

pr feedback

225329f

Merge branch 'logging-filters' of https://github.com/adriangb/cpython …

735d78a

…into logging-filters

Merge branch 'main' into logging-filters

5e54c77

adriangb marked this pull request as ready for review May 10, 2022

gpshead self-requested a review May 10, 2022

fix whitespace

f52e404

vsajip requested changes May 10, 2022

View changes

fix docs as per pr feedback

46e2f7f

python / cpython Public

gh-92592 : allow logging filters to return a LogRecord #92591

gh-92592 : allow logging filters to return a LogRecord #92591

adriangb commented May 10, 2022 •

edited

vsajip left a comment

vsajip May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

adriangb May 10, 2022

vsajip May 10, 2022

adriangb May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

bedevere-bot commented May 10, 2022

adriangb commented May 10, 2022

vsajip commented May 10, 2022

adriangb commented May 10, 2022 •

edited

vsajip commented May 10, 2022

gpshead commented May 10, 2022

adriangb commented May 10, 2022

vsajip May 10, 2022

adriangb May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

vsajip May 10, 2022

adriangb May 10, 2022 •

edited

		@@ -663,8 +663,9 @@ empty string, all events are passed.
		.. method:: filter(record)

		Is the specified record to be logged? Returns zero for no, nonzero for

		@@ -0,0 +1,3 @@
		Let :mod:`logging` Filters to return a :class:`logging.LogRecord` instance

python / cpython Public

gh-92592 : allow logging filters to return a LogRecord #92591

Are you sure you want to change the base?

gh-92592 : allow logging filters to return a LogRecord #92591

Conversation

adriangb commented May 10, 2022 • edited

vsajip 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

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

bedevere-bot commented May 10, 2022

adriangb commented May 10, 2022

vsajip commented May 10, 2022

adriangb commented May 10, 2022 • edited

vsajip commented May 10, 2022

gpshead commented May 10, 2022

adriangb commented May 10, 2022

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

adriangb May 10, 2022 • edited

Choose a reason for hiding this comment

adriangb commented May 10, 2022 •

edited

adriangb commented May 10, 2022 •

edited

adriangb May 10, 2022 •

edited