classification
Title: logging.Formatter enhancement - Checking on style and fmt fields
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.8
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: BNMetrics, gvanrossum, vinay.sajip
Priority: normal Keywords: patch

Created on 2018-09-29 20:17 by BNMetrics, last changed 2018-10-16 06:44 by vinay.sajip. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 9703 merged BNMetrics, 2018-10-04 19:35
Messages (7)
msg326690 - (view) Author: Luna Chen (BNMetrics) * Date: 2018-09-29 20:17
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)
msg326691 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-09-29 22:02
Moving to Python 3.8 because this is a feature proposal. Adding Vinay to nosy list because this is about logging.
msg326763 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2018-10-01 05:37
> 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.
msg326829 - (view) Author: Luna Chen (BNMetrics) * Date: 2018-10-01 20:58
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
msg327108 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2018-10-05 07:15
I see a PR has been added, I'll start to review it after the CLA has been signed.
msg327772 - (view) Author: Vinay Sajip (vinay.sajip) * (Python committer) Date: 2018-10-15 18:41
New changeset 18fb1fb943b7dbd7f8a76017ee2a67ef13effb85 by Vinay Sajip (BNMetrics) in branch 'master':
bpo-34844: logging.Formatter enhancement - Ensure style and format string matches in logging.Formatter  (GH-9703)
https://github.com/python/cpython/commit/18fb1fb943b7dbd7f8a76017ee2a67ef13effb85
msg327775 - (view) Author: Guido van Rossum (gvanrossum) * (Python committer) Date: 2018-10-15 19:17
W00t! Congrats Luna and thanks for your contribution. Thanks Vinay for the prompt reviews!
History
Date User Action Args
2018-10-16 06:44:50vinay.sajipsetstatus: open -> closed
resolution: fixed
stage: patch review -> resolved
2018-10-15 19:17:30gvanrossumsetmessages: + msg327775
2018-10-15 18:41:42vinay.sajipsetmessages: + msg327772
2018-10-05 07:15:38vinay.sajipsetmessages: + msg327108
2018-10-04 19:35:12BNMetricssetkeywords: + patch
stage: patch review
pull_requests: + pull_request9088
2018-10-01 20:58:40BNMetricssetmessages: + msg326829
2018-10-01 05:37:52vinay.sajipsetmessages: + msg326763
2018-09-29 22:02:27gvanrossumsetnosy: + vinay.sajip

messages: + msg326691
versions: + Python 3.8, - Python 3.6
2018-09-29 20:17:33BNMetricssettitle: logging.Formatter enhancement - Checking on style and -> logging.Formatter enhancement - Checking on style and fmt fields
2018-09-29 20:17:10BNMetricscreate