Possible logging simplification

[knoepfel]

The motivation for this is that in every module that wants to use framework-supported logging, that module must provide the following boilerplate in the module constructor

self.logger = self.logger.bind(class_module=__name__.split(".")[-1], )

This PR removes this boilerplate by adding the following in only the framework's Module constructor:

self.logger = self.logger.bind(class_name=type(self).__name__, ...)

In many cases, the name of the Python module and the name of the class are the same, so this would not change those cases. This works because the self argument is the type of the most derived class that invokes super().__init__(...) in its own module constructor.

Question: Is this simplification desired despite the slight change in behavior?

[codecov]

Codecov Report

Merging #515 (2fbf2af) into master (d7f4401) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

❗ Current head 2fbf2af differs from pull request most recent head 39f7430. Consider uploading reports for the commit 39f7430 to get more accurate results

@@            Coverage Diff             @@
##           master     #515      +/-   ##
==========================================
- Coverage   94.12%   94.07%   -0.05%     
==========================================
  Files          44       44              
  Lines        2742     2736       -6     
  Branches      392      392              
==========================================
- Hits         2581     2574       -7     
  Misses        122      122              
- Partials       39       40       +1     

Flag	Coverage Δ
python-3.6	93.50% <100.00%> (-0.02%)	⬇️
python-3.9	93.60% <100.00%> (-0.06%)	⬇️
python-pypy-3.7	93.58% <100.00%> (-0.09%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ecisionengine/framework/logicengine/LogicEngine.py	96.55% <ø> (-0.12%)	⬇️
src/decisionengine/framework/modules/Publisher.py	100.00% <ø> (ø)
src/decisionengine/framework/modules/Source.py	100.00% <ø> (ø)
...rc/decisionengine/framework/modules/SourceProxy.py	65.21% <ø> (-0.50%)	⬇️
src/decisionengine/framework/modules/Transform.py	100.00% <ø> (ø)
src/decisionengine/framework/modules/Module.py	100.00% <100.00%> (ø)
src/decisionengine/framework/dataspace/maintain.py	98.11% <0.00%> (-0.95%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d7f4401...39f7430. Read the comment docs.

[goodenou]

I think the change you suggest is fine, and a great solution to the problem of having to add a line in every module. I will let Marco comment on it.

[knoepfel]

Thanks, @goodenou.

[mambelli]

I like the idea to use self and reduce the boilerplate.
I let Lisa pick if the class is preferable. It may be since the logger is an attribute of the class.

Anyway, if the module name is preferable, it is possible to get that one as well:

self.logger = self.logger.bind(class_module=type(self).__module__.split(".")[-1], )

[knoepfel]

Thanks, @mambelli, for your comments. @goodenou, do you prefer the class name or the module name in the bind statement? Just let me know and I'll adjust the code.

[goodenou]

Class name seems a bit better to me.

[knoepfel]

Thanks, @goodenou--the current PR uses class_name. I will open a companion PR in decisionengine_modules.

[goodenou]

@knoepfel Will you also please remove the following lines from the LogicEngine.py module:

from decisionengine.framework.modules.logging_configDict import CHANNELLOGGERNAME
...
self.logger = structlog.getLogger(CHANNELLOGGERNAME)
self.logger = self.logger.bind(class_module=name.split(".")[-1], channel=self.channel_name)

It inherits from the Module class and doesn't need these definitions.

[pep8speaks]

Hello @knoepfel! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-09-21 15:06:58 UTC

[knoepfel]

Good catch, Lisa. Fixed.

[BrunoCoimbra]

The changes look good. If @goodenou and @mambelli agree, this PR is ready to merge.