Logging configurability

[jamesbraza]

• Adds the ability to override logging.basicConfig parameters
• Adds type hints to the signature
• Fixes minor wordings in docstring

[janiversen]

I can see an argument for changing the log layout, but we need to secure that the default_format is not overwritten if level=DEBUG. We need a specific format of the log when people open new issues. I have a small script I use on the log (here and e.g. in homeassistant) which help me extract the information I need, with an unknown format I have to that manually.

[jamesbraza]

Thanks for the review! 👍

we need to secure that the default_format

The default_format lives as a local inside a classmethod, so it can't be changed.

We need a specific format of the log when people open new issues.

Fwiw, it already says this in the docstring of pymodbus_apply_logging_config. Also, the issue template doesn't pass any overrides, so I think when people open an issue they will not pass an override. I think we're covered here.

Would you like me to add a comment on this to the issue template too?

[janiversen]

I am not talking about changing docstrings. It is possible to change the layout of the log, this should not be permitted for level=DEBUG.

I suppose you want it configurable to match what you use in the application, this is a fair argument for log levels != DEBUG. It correspond to not calling pymodbus_apply_logging_config(). The main reason for level = DEBUG is to report an issue and for that the format must be fixed, in order for you not to change the program it needs to be done in pymodbus_apply_logging_config().

[jamesbraza]

Okay I hear what you're saying. The thing is, I actually use debug logging for a whole instrument (pymodbus is one of many protocols), so I don't want to deny logging.DEBUG level configuration. In other words, I actually override with the logging.DEBUG level.

[janiversen]

This still allows to overwrite DEBUG. You are actually a good example, if you overwrite DEBUG, you are not able to submit issues, without somehow changing the overwrite.

In your case it would be a lot simpler, not to call our function, but do

  pymodbus.Log.LOG_LEVEL = <what you want>

A small change in the Log class could actually avoid that, by moving that line into build_msg.

have LOG_LEVEL = None
and do:

   if not cls.LOG_LEVEL:
       cls.LOG_LOVEL = cls…….

IF you do not call our function your basic_config will rule.

[jamesbraza]

   if not cls.LOG_LEVEL:
       cls.LOG_LOVEL = cls…….

I don't quite understand this part. What does the None sentinel level mean?

Does this go into build_msg or apply_logging_config, and where within that method? Why is it mutating the Log.LOG_LEVEL if cls.LOG_LEVEL = None?

[janiversen]

if you do not call apply_logging_config, cls.LOG_LEVEL still need to be set.

cls.LOG_LEVEL starts of being None, to secure it is set to effective log level, first time build_msg is called.

[jamesbraza]

I think cls.LOG_LEVEL will fail because None comparisons in checks like this logging.INFO >= cls.LOG_LEVEL will fail as a TypeError before we even get to build_msg.

[janiversen]

no it will not.

[jamesbraza]

>>> logging.INFO >= None
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '>=' not supported between instances of 'int' and 'NoneType'

I don't understand what you mean, I am sorry.

[jamesbraza]

Looks like you merged an upstream PR that basically nullifies the core of this PR.

@janiversen I am ready to abandon this, up to you.

[janiversen]

your PR so it’s your decision not mine. I merged a PR with some pending cleanup, but it does not have support for overwriting the configuration.

[jamesbraza]

Closing since upstream no longer uses logging.basicConfig, which this PR centers around