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.Formatter enhancement - Checking on style and fmt fields #79025

Closed
BNMetrics mannequin opened this issue Sep 29, 2018 · 7 comments
Closed

logging.Formatter enhancement - Checking on style and fmt fields #79025

BNMetrics mannequin opened this issue Sep 29, 2018 · 7 comments
Labels
3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement

Comments

@BNMetrics
Copy link
Mannequin

BNMetrics mannequin commented Sep 29, 2018

BPO 34844
Nosy @gvanrossum, @vsajip, @BNMetrics
PRs
  • bpo-34844: logging.Formatter enhancement - Ensure styles and fmt matches in logging.Formatter  #9703
  • 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 2018-10-16.06:44:50.300>
    created_at = <Date 2018-09-29.20:17:10.806>
    labels = ['3.8', 'type-feature', 'library']
    title = 'logging.Formatter enhancement - Checking on style and fmt fields'
    updated_at = <Date 2018-10-16.06:44:50.299>
    user = 'https://github.com/BNMetrics'

    bugs.python.org fields:

    activity = <Date 2018-10-16.06:44:50.299>
    actor = 'vinay.sajip'
    assignee = 'none'
    closed = True
    closed_date = <Date 2018-10-16.06:44:50.300>
    closer = 'vinay.sajip'
    components = ['Library (Lib)']
    creation = <Date 2018-09-29.20:17:10.806>
    creator = 'BNMetrics'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 34844
    keywords = ['patch']
    message_count = 7.0
    messages = ['326690', '326691', '326763', '326829', '327108', '327772', '327775']
    nosy_count = 3.0
    nosy_names = ['gvanrossum', 'vinay.sajip', 'BNMetrics']
    pr_nums = ['9703']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue34844'
    versions = ['Python 3.8']

    @BNMetrics
    Copy link
    Mannequin Author

    BNMetrics mannequin commented Sep 29, 2018

    Issue: Currently logging.Formatter does not check if the format passed in is valid style or if the field is valid when creating the logging.Formatter object. It would be nice to have such check in the constructor of the logging.Formatter.

    Here are 2 scenarios:

    Scenario 1: Passing in invalid fmt when creating the logging.Formatter.
    Example:
    import logging
    logger = logging.getLogger('my_logger')
    handler = logging.StreamHandler()

        fmt = logging.Formatter("blah-blah", style="{")
        handler.setFormatter(fmt)
    
        logger.addHandler(handler)
        logger.setLevel(logging.DEBUG)
    
        logger.info('This is my logging message.')

    The above example would output the fmt string("blah-blah") whenever a logging operation is performed, such as logging.info, logging.warning.
    And this goes the same for mismatching style and fmt, like so:
    fmt = logging.Formatter("{asctime}-{message}", style="%")

    Scenario 2: Passing in invalid fields to logging.Formatter

        import logging
        logger = logging.getLogger('my_logger')
        handler = logging.StreamHandler()
    
        fmt = logging.Formatter("%(FuncName)s-%(message)s")
        handler.setFormatter(fmt)
    
        logger.addHandler(handler)
        logger.setLevel(logging.DEBUG)
    
        logger.info('This is my logging message.'
    )
    As you can see from the above example, the "%(FuncName)s" field is misspelled with a capital "F", which should have been a lowercase "f". In this scenario, we would get an interesting stacktrace:

    --- Logging error ---

    Traceback (most recent call last):
      File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/logging/__init__.py", line 992, in emit
        msg = self.format(record)
      File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/logging/__init__.py", line 838, in format
        return fmt.format(record)
      File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/logging/__init__.py", line 578, in format
        s = self.formatMessage(record)
      File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/logging/__init__.py", line 547, in formatMessage
        return self._style.format(record)
      File "/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/logging/__init__.py", line 391, in format
        return self._fmt % record.__dict__
    KeyError: 'FuncName'
    Call stack:
      File "<stdin>", line 1, in <module>
    Message: 'This is my logging message.'
    Arguments: ()

    Personally, I think the "KeyError" here can be misleading and confusing to some.

    Proposal:

    I would like to make a PR with the following changes:

    • Checking fmt to match the style in the constructor of logging.Formatting

    • When calling log message functions such as "logger.info()", an "extra" key word arg can be passed for custom format fields,
      for example:
      fmt = logging.Formatter("{cpuUsage} - {message}", style="{") # In this case, cpuUsage would be custom
      logger.info("my logging message", extra={"cpuUsage": "my_cpu_usage"})

      • I would like to have custom fields passed in as an additional (optional) argument into the constructor for logging.Formatter
      • Another option is to have an additional member method for adding additional fields
      • I think we could essentially have both options
      • With this, we can remove the "extra" argument in Logger.makeRecord()

    Draw Backs:

    • This change might essentially break some existing code where someone might use the "extra" argument in log message functions, as we would do the check on logging.Formatter level

    Please let me know your thoughts, and I thought it would be nice to get some new ideas and areas I haven't thought about before I make this PR.

    Best regards,
    Luna Chen (BNMetrics)

    @BNMetrics BNMetrics mannequin added stdlib Python modules in the Lib dir type-feature A feature request or enhancement labels Sep 29, 2018
    @BNMetrics BNMetrics mannequin changed the title logging.Formatter enhancement - Checking on style and logging.Formatter enhancement - Checking on style and fmt fields Sep 29, 2018
    @gvanrossum
    Copy link
    Member

    Moving to Python 3.8 because this is a feature proposal. Adding Vinay to nosy list because this is about logging.

    @gvanrossum gvanrossum added the 3.8 only security fixes label Sep 29, 2018
    @vsajip
    Copy link
    Member

    vsajip commented Oct 1, 2018

    Checking fmt to match the style in the constructor of logging.Formatter

    This seems a reasonable change to want to make. You would need to parse the format string for fields using the appropriate style. This should probably be via a validate() method in each of the XXXStyle classes, which is passed the format string and raises an exception if invalid.

    I would like to have custom fields passed in as an additional (optional) argument into the constructor for logging.Formatter

    If this is just a list of custom field names, it could be inferred from the passed format string, which will now be being parsed for fields for the checking described above. So there should be no need to pass an additional argument.

    With this, we can remove the "extra" argument in Logger.makeRecord()

    We can't do this, because of the need to maintain backwards compatibility. Note also that custom fields can be introduced into a LogRecord in other ways, e.g. using Filters.

    the "KeyError" here can be misleading and confusing

    It seems reasonable to make a change to re-raise such a KeyError using a more informative error message, perhaps as a ValueError.

    @BNMetrics
    Copy link
    Mannequin Author

    BNMetrics mannequin commented Oct 1, 2018

    Thank you Vinay!
    I think you have some good points! :)

    I'm going to make the following changes

    • re-raise the keyError into ValueError in XXXStyle.format
    • add validate() methods to XXXstyle class, and call them in logging.Formatter

    Best regards,
    Luna Chen

    @vsajip
    Copy link
    Member

    vsajip commented Oct 5, 2018

    I see a PR has been added, I'll start to review it after the CLA has been signed.

    @vsajip
    Copy link
    Member

    vsajip commented Oct 15, 2018

    New changeset 18fb1fb by Vinay Sajip (BNMetrics) in branch 'master':
    bpo-34844: logging.Formatter enhancement - Ensure style and format string matches in logging.Formatter (GH-9703)
    18fb1fb

    @gvanrossum
    Copy link
    Member

    W00t! Congrats Luna and thanks for your contribution. Thanks Vinay for the prompt reviews!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.8 only security fixes stdlib Python modules in the Lib dir type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    2 participants