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

logging.config.dictConfig does not work with callable filters #86072

Closed
raybb mannequin opened this issue Oct 2, 2020 · 11 comments
Closed

logging.config.dictConfig does not work with callable filters #86072

raybb mannequin opened this issue Oct 2, 2020 · 11 comments
Labels
3.11 only security fixes type-feature A feature request or enhancement

Comments

@raybb
Copy link
Mannequin

raybb mannequin commented Oct 2, 2020

BPO 41906
Nosy @vsajip, @mariocj89, @miss-islington, @l0rb, @PetrPy
PRs
  • bpo-41906: Accept built filters in dictConfig #30756
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2022-01-24.12:42:07.148>
    created_at = <Date 2020-10-02.00:43:26.721>
    labels = ['type-feature', '3.11']
    title = 'logging.config.dictConfig does not work with callable filters'
    updated_at = <Date 2022-01-24.12:42:07.145>
    user = 'https://bugs.python.org/raybb'

    bugs.python.org fields:

    activity = <Date 2022-01-24.12:42:07.145>
    actor = 'vinay.sajip'
    assignee = 'none'
    closed = True
    closed_date = <Date 2022-01-24.12:42:07.148>
    closer = 'vinay.sajip'
    components = []
    creation = <Date 2020-10-02.00:43:26.721>
    creator = 'raybb'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 41906
    keywords = ['patch']
    message_count = 11.0
    messages = ['377791', '377800', '377813', '377831', '379211', '389549', '410979', '410993', '411013', '411019', '411470']
    nosy_count = 7.0
    nosy_names = ['vinay.sajip', 'mariocj89', 'lkollar', 'miss-islington', 'lorb', 'raybb', 'PetrPy111']
    pr_nums = ['30756']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue41906'
    versions = ['Python 3.11']

    @raybb
    Copy link
    Mannequin Author

    raybb mannequin commented Oct 2, 2020

    According to the docs here (https://docs.python.org/3/library/logging.html):

    "You don’t need to create specialized Filter classes, or use other classes with a filter method: you can use a function (or other callable) as a filter. The filtering logic will check to see if the filter object has a filter attribute: if it does, it’s assumed to be a Filter and its filter() method is called. Otherwise, it’s assumed to be a callable and called with the record as the single parameter."

    If I use this code:

    def noErrorLogs(param):
        return 1 if param.levelno < 40 else 0
    
    logconfig_dict = {
        'filters': {
            'myfilter': {
                '()': noErrorLogs,
            }
        },
        "handlers": {
            "console": {
                "class": "logging.StreamHandler",
                "level": "DEBUG",
                "stream": "ext://sys.stdout",
                "filters": ["myfilter"]
            }
        },
        "root": {"level": "DEBUG", "handlers": ["console"]},
        "version": 1,
    }
    dictConfig(logconfig_dict)

    I get the error "Unable to configure filter 'myfilter'" because "noErrorLogs() missing 1 required positional argument: 'param'"

    However, If I use this code:

    def noErrorLogs(param):
        return 1 if param.levelno < 40 else 0
    
    logconfig_dict = {
        "handlers": {
            "console": {
                "class": "logging.StreamHandler",
                "level": "DEBUG",
                "stream": "ext://sys.stdout",
            }
        },
        "root": {"level": "DEBUG", "handlers": ["console"]},
        "version": 1,
    }
    
    logging.basicConfig()
    dictConfig(logconfig_dict)
    l = logging.getLogger()
    l.handlers[0].addFilter(noErrorLogs)

    Then the filter works correctly.

    The bug I am reporting is that when using logging.config.dictConfig you cannot pass in a callable that acts a filter. You can only pass in a class with a filter method or a function that returns a callable filter.

    If this is the expected behavior perhaps the docs should make it clear that callables cannot be used with dictConfig.

    @raybb raybb mannequin added type-bug An unexpected behavior, bug, or error labels Oct 2, 2020
    @vsajip
    Copy link
    Member

    vsajip commented Oct 2, 2020

    The value of the '()' key should be a factory - something that returns a filter (either something that has a filter method or a callable). That returned callable, or the filter method, will be called with a LogRecord and its return value used to determine whether the event should be processed. An example of using factories is here:

    https://docs.python.org/3/library/logging.config.html#user-defined-objects

    In your first example, you're passing the filter where you need to pass the factory. If you change it to e.g. return a lambda that does the filtering, it should work as expected.

    At the moment you can't directly pass a filter callable in the config dictionary - only a factory that returns a filter. I'll look at this as a possible enhancement.

    @vsajip vsajip added 3.10 only security fixes type-feature A feature request or enhancement and removed type-bug An unexpected behavior, bug, or error labels Oct 2, 2020
    @raybb
    Copy link
    Mannequin Author

    raybb mannequin commented Oct 2, 2020

    Thank you for the clarification.

    I think I was most confused by the docs on this page (which I should have included in my initial post): https://docs.python.org/3/howto/logging.html

    It says:

    "In Python 3.2, a new means of configuring logging has been introduced, using dictionaries to hold configuration information. This provides a superset of the functionality of the config-file-based approach outlined above, and is the recommended configuration method for new applications and deployments."

    Since it is the recommended configuration method I had naively assumed that it would be compatible with the feature of just using callables instead which was mentioned here https://docs.python.org/3/library/logging.html.

    I think it would be a nice enhancement long term to be able too support callables in dictconfigs.

    In the short term do you think it is reasonable to update the first page mentioned in this comment to clarify that even though dictConfig is recommended it is not a feature parity with the object oriented api?

    It would also be nice to clarify this on the second page to say that callables don't work if using dictConfig.

    I understand it does say a factory is required on the page you linked. I think anyone who reads the docs as well as they should will find it. But I think adding information about this too other parts of the docs will make it much easier for newer folks to catch the difference early on.

    Thanks for your work on the logging parts of the api in python. I've read many of your docs and they are very helpful.

    @vsajip
    Copy link
    Member

    vsajip commented Oct 2, 2020

    You make reasonable points. I won't close this issue, and get to those updates when I can/as time allows. Meanwhile, if you or someone else proposes specific changes by way of a pull request, I'll look at that too. And thanks for your kind words about the logging docs.

    @l0rb
    Copy link
    Mannequin

    l0rb mannequin commented Oct 21, 2020

    I looked into implementing this and it's not entirely clear how it could work. The main issue I encountered is how to distinguish between a callable that is a factory that takes one argument and a callable that is a filter.

    One possible approach I see is to rely on the absence of any other keys in the configuration sub-directory. Not entirely happy with that (and have to think how to treat factory with optional parameter vs filter with optional parameter).
    So this would be assumed to be a filter, if inspection reveals that fltr takes an argument:
    'myfilter': {
    '()': fltr,
    }
    while this would be assumed to be a factory:
    'myfilter': {
    '()': fctry,
    'bar': 'baz',
    }

    A different approach that I think would be not optimal is to just call the callable with a dummy LogRecord passed ind and see if it returns a boolean or callable (or throws).

    @mariocj89
    Copy link
    Mannequin

    mariocj89 mannequin commented Mar 26, 2021

    Vinay would you consider a patch for logging where dictConfig allows taking objects directly in addition to the reference id?

    That would allow the following:

    
    def noErrorLogs(param):
        return 1 if param.levelno < 40 else 0
    
    logconfig_dict = {
        "handlers": {
            "console": {
                "class": "logging.StreamHandler",
                "level": "DEBUG",
                "stream": "ext://sys.stdout",
                "filters": [noErrorLogs]
            }
        },
        "root": {"level": "DEBUG", "handlers": ["console"]},
        "version": 1,
    }
    dictConfig(logconfig_dict)
    

    or alternatively passing them on declaration:

    logconfig_dict = {
        'filters': {
            'myfilter': noErrorLogs,
        },
        "handlers": {
            "console": {
                "class": "logging.StreamHandler",
                "level": "DEBUG",
                "stream": "ext://sys.stdout",
                "filters": ["myfilter"]
            }
        },
        "root": {"level": "DEBUG", "handlers": ["console"]},
        "version": 1,
    }
    dictConfig(logconfig_dict)
    

    I'm happy to put a patch together if that looks good to you.

    @PetrPy
    Copy link
    Mannequin

    PetrPy mannequin commented Jan 19, 2022

    I would definitely vote for implementing this enhancement. I have just ran into the very same issue and my search ended here. Using dictConfig e.g. with lambdas seems very natural to me and I understood the docs incorrectly exactly as had been reported.

    @vsajip
    Copy link
    Member

    vsajip commented Jan 20, 2022

    Vinay would you consider a patch for logging where dictConfig allows taking objects directly in addition to the reference id?

    Sorry I didn't respond to this; it dropped off my radar. Certainly, I would consider such a patch.

    @mariocj89
    Copy link
    Mannequin

    mariocj89 mannequin commented Jan 20, 2022

    Great! I'll put something together then. If you have any preference about the implementation or any pointer on the way you think should be done, please let me know.

    @vsajip
    Copy link
    Member

    vsajip commented Jan 20, 2022

    If you have any preference about the implementation or any pointer

    Nothing particular, just try to fit in with the existing code conventions even where they don't follow modern idioms (e.g. a lot of the code in this package predates PEP-8 and new styles of string formatting).

    @miss-islington
    Copy link
    Contributor

    New changeset d7c6863 by Mario Corchero in branch 'main':
    bpo-41906: Accept built filters in dictConfig (GH-30756)
    d7c6863

    @vsajip vsajip added 3.11 only security fixes and removed 3.10 only security fixes labels Jan 24, 2022
    @vsajip vsajip closed this as completed Jan 24, 2022
    @vsajip vsajip added 3.11 only security fixes and removed 3.10 only security fixes labels Jan 24, 2022
    @vsajip vsajip closed this as completed Jan 24, 2022
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.11 only security fixes type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants