DM-17651: Allow lsst.log to forward to Python logging #37
Conversation
There was a problem hiding this comment.
Looks OK. I'd probably add new method to log module to change that class variable instead of manipulating it directly. Log class is implementation detail and should not be used directly. Also having that method would probably help if we decide to implement log4cxx forwarding too.
|
I don't know what the split is between Python and C++ logging in normal Task output. Having some output may be better than none, but it doesn't seem like a comprehensive-enough solution to replace #33 completely. |
timj
force-pushed
the
tickets/DM-17651
branch
from
52178a3
to
0be378f
Compare
timj
force-pushed
the
tickets/DM-17651
branch
from
829c057
to
d49bf57
Compare
python/lsst/log/log/logContinued.py
Outdated
| @@ -250,6 +328,36 @@ def emit(self, record): | |||
| logger = Log.getLogger(record.name) | |||
| # Use standard formatting class to format message part of the record | |||
| message = self.formatter.format(record) | |||
There was a problem hiding this comment.
This message is not used when UsePythonLogging is set, maybe move it below if to save few CPU cycles.
| # If the logger has a parent and propagation is allowed | ||
| # do nothing. | ||
| if pylgr.parent and pylgr.parent.hasHandlers() and pylgr.propagate: | ||
| return |
There was a problem hiding this comment.
I think that in general it is a mistake to have root logger configured without any handlers. So you should probably only handle one special case when this handler is the only handler for the root logger?
There was a problem hiding this comment.
You mean always return immediately unless this is the root logger and no handlers exist?
There was a problem hiding this comment.
Yeah, I guess this is what it means - standard way to use LogHandler is to setup root logger to only have a LogHandler handler. Attaching LogHandler to non-root loggers is still possible but then people are supposed to know what they are doing and if they disable propagation then we are not expected to send messages to other handlers.
There was a problem hiding this comment.
I'll leave it as is for the moment so that the message might appear. The logic is a bit tricky because if LogHandler is attached to a child logger and also the root logger you have to work through the logic multiple times and try not to duplicate yourself. Always returning without action might be the best approach (and is what happens most of the time).
If Python forwarding is enabled the lsst.log.LogHandler should do nothing itself and rely on other logging handlers to process the message. If no other handlers are present attempts to use a StreamHandler.
Python logging has deprecating logging.warn and preferrs logging.warning. This change adds lsst.log.warning to enable both forms to reduce cognitive dissonance between the two logging packages.
timj
force-pushed
the
tickets/DM-17651
branch
from
6f2f353
to
09ab2a6
Compare
This only deals with the python messages. C++ log messages will do what they always do. This is implemented as a big class property on
lsst.log.Log. The idea is that a test can turn this on insetUp, check that a log message was created, and then turn it off intearDown.