Skip to content
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 a logging.Filter implementation #52

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

znerol
Copy link
Member

@znerol znerol commented Nov 28, 2021

This PR adds an AnonipFilter which can be used as a filter in the Python logging framework. The filter can either be hard-coded when initializing the logging framework using logging.basicConfig, or it can be supplied in the filters entry of logging.dictConfig. A working example of the latter approach:

version: 1

formatters:
    log_fmt:
        format: '%(name)s: %(message)s'

filters:
    anonip:
        (): anonip.AnonipFilter
        keys: [msg]
        anonip:
            columns: [1]
            ipv4mask: 16
            ipv6mask: 64

handlers:
    stream:
        class: logging.StreamHandler
        formatter: log_fmt
        filters: [anonip]

root:
    level: INFO
    handlers: [stream]

disable_existing_loggers: False

@znerol
Copy link
Member Author

znerol commented Nov 28, 2021

Note: requires #51

@znerol
Copy link
Member Author

znerol commented Dec 2, 2021

Note: I am capable and willing to maintain this in a separate repo / package if you think that the feature is not in scope for this project. Just let me know...

@open-dynaMIX
Copy link
Member

open-dynaMIX commented Dec 2, 2021

Hey @znerol

Thank you very much for this contribution 🚀! I'm sorry you had to ping me. I like this feature a lot and I think it absolutely is in the scope of this project.

I'm very absorbed by work and life right now, but I see some light at the end of the tunnel by the end of next week. Maybe I already find some time this weekend, but I really can't promise.

@znerol
Copy link
Member Author

znerol commented Dec 2, 2021

I guess I have to update the docs anyway (params changed since the first commit). Also it might be helpful to provide a working example for an existing app. I'm planning to use this in matrix-org/synapse thus will try to come up with a working config for that project.

@znerol znerol force-pushed the feature/master/logging-filter branch from 3e8ebb1 to fe9f794 Compare December 4, 2021 17:50
@znerol
Copy link
Member Author

znerol commented Dec 4, 2021

Testing this with the following log.yml in synapse:

version: 1

# In systemd's journal, loglevel is implicitly stored, so let's omit it
# from the message text.
formatters:
    journal_fmt:
        format: '%(name)s: [%(request)s] %(message)s'

filters:
    context:
        (): synapse.logging.context.LoggingContextFilter
        request: ""
    anonip.access:
        (): anonip.AnonipFilter
        args: [0]

handlers:
    journal:
        class: systemd.journal.JournalHandler
        formatter: journal_fmt
        filters: [context]
        SYSLOG_IDENTIFIER: synapse

loggers:
    synapse.access.http.8008:
        filters: [anonip.access]

root:
    level: INFO
    handlers: [journal]

disable_existing_loggers: False

Regrettably LogRecord.args can be different things (dict or tuple, maybe more). Thus the code to handle that is quite messy (could be cleaned up a bit - but some branching will remain). I am tempted to introduce multiple implementations of AnonipFilter (one for each args type and another one for extras). That might make it a bit difficult for people to initially setup logging, but I expect the implementation to be more robust (and easier to test).

Copy link
Member

@open-dynaMIX open-dynaMIX left a comment

Choose a reason for hiding this comment

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

Awesome work, thanks a lot 🚀

I'd like to play around with it a bit, as soon as the tests pass.

"""
See logging.Filter.filter()
"""
if record.name != "anonip":
Copy link
Member

Choose a reason for hiding this comment

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

It would be nice to make this method more readable. As a start we could exit early with

if record.name == "anonip":
    return True

tests.py Show resolved Hide resolved
@open-dynaMIX
Copy link
Member

Just as a heads up: I just renamed the master branch to main.

@open-dynaMIX
Copy link
Member

open-dynaMIX commented Dec 26, 2021

Hey @znerol

I had some time to play around with it and made a suggestion in #59. There I have squashed your 12 commits into one, rebased onto current main and have added an additional commit.

With my commit things seem to work and tests are passing (coverage is still lacking tho). I did mostly look and fix the args handling, ignoring extra, but I guess the same things will apply to this and it could probably share the same logic.

I have the impression, that the logging module puts the args into a tuple, if you provide just one string. That's why I wrote the test accordingly. But maybe I'm mistaken.

You could now cherry-pick my commit into your branch and squash all commits together and use it as a base to finish this feature (I don't care if you will be the sole author then ;)).

On another note: I'm planning on releasing v1.1.0 in the coming days. Would be nice to land this feature in main before. If you don't have enough time right now, we can also include it in v1.2.0 later.

Happy holidays to you!

@znerol
Copy link
Member Author

znerol commented Dec 27, 2021

There is no need to rush this into 1.1.0.

I still dislike the sheer number of control statements / conditionals in both implementations. I'd like to try breaking the whole thing up into multiple filter classes. One for record.msg, one for record.extra and maybe different ones for record.args if args is expected to be a tuple and record.orgs if a mapping is expected.

The filter needs to be attached to a specific logger instance anyway. According to my tests with synapse attaching it to the root logger results in a multitude of problems. Since the system administrator needs to decide on which logger to attach the filter, choosing the necessary implementation shouldn't be to much additional trouble.

@znerol
Copy link
Member Author

znerol commented Dec 27, 2021

See #60

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants