globality-corp / flake8-logging-format

Flake8 extension to validate (lack of) logging format strings
Apache License 2.0
134 stars 21 forks source link

Suggestion: Include motivation in README.md. #15

Open Julian-O opened 5 years ago

Julian-O commented 5 years ago

The README.md currently says:

One way to ensure consistency and rigor in logging is to always use extra to pass non-constant data and, therefore, to never use format strings, concatenation, or other similar techniques to construct a log string.

However, if doesn't explain why this rigor is important. It is easy to dismiss this: "A foolish consistency is the hobgoblin of little minds."

I would add a few paragraphs explaining why this practice should be adopted.

e.g.

It is important to avoid using format strings for two reasons:

  1. Performance: If a log message is turned off, it is unnecessary to perform the string interpolation and/or the string concatenation and the implicit calls to __str__() functions on the parameters. Keeping the parameter separate means that the logging system can skip such operations.
    1. Maintainability: The logging module is designed to provide a separation of concerns: the treatment of log messages from a module can be modified outside of the module. In particular, logging.Handler and logging.Filter classes can be customised to identify particular log messages and take particular actions, such as filter them, escalate them, replace them with a clearer log message or translate them to a different language. These manipulations are easier if access is given to the raw log message before the string interpolation is performed.
fpuga commented 5 years ago

It seems that the original motivation was explained in #5 but i agree that it will be nice have it in the README.

Also, i don't understand the example in the README. Afaik extra keyword is not used for variable data but for the logging.Formatter.

>>> import logging

>>> logging.error("Hello {world}", extra={"world":"Earth", "color": "blue"})                                                                                                                                                   
ERROR:root:Hello {world}

>>> import logging 
>>> logging.basicConfig(format="%(color)s - %(message)s") 
>>> logging.error("Hello {world}", extra={"world":"Earth", "color": "blue"})                                                                                                                                   
blue - Hello {world}
afallou commented 5 years ago

@fpuga that is correct. We use two custom formatters. One we wrote for console output, that will format the log message using those extra arguments. The other from pythonjsonlogger will add the extra dict to the logged JSON; we use it for the logs we send to our logging aggregation system (we use Loggly).

I agree the README could be made clearer there.

afallou commented 5 years ago

@Julian-O a paragraph on motivation was added to the README in https://github.com/globality-corp/flake8-logging-format/pull/19

fpuga commented 5 years ago

Thanks @afallou, it's more clear now.

FanchenBao commented 4 years ago

Since it is clear now that extra dict is only used for formatter, please also update the example. It could cause confusion.

Dreamsorcerer commented 4 years ago

Has caused confusion. Just spent half an hour trying to figure out why the variables weren't being output.