Add syslog support#84
Conversation
* Adding syslog setup utility method so that you can easily use `logzero` to log directly to the `syslog` facility of your choice. Still more to come for testing, adding documentation, and perhaps streamlining the implementation a bit.
logzero/__init__.py
Outdated
| logger.addHandler(rotating_filehandler) | ||
|
|
||
|
|
||
| def remove_internal_loggers(logger_to_update, disableStderrLogger=True): |
There was a problem hiding this comment.
perhaps add a default value like logger_to_update=logger? Like this it would raise an exception if no argument is passed for that.
There was a problem hiding this comment.
How about we just make it private? I don't think this method should ever be used outside of logzero internals. I think the real, proper design would be to do what I described in Discord and make logzero loggers a class where this method would be a private class method, instead.
| if isinstance(handler, RotatingFileHandler): | ||
| logger_to_update.removeHandler(handler) | ||
| elif isinstance(handler, logging.StreamHandler) and disableStderrLogger: | ||
| logger_to_update.removeHandler(handler) |
There was a problem hiding this comment.
should this also remove the syslog handler?
|
very nice, thanks! 2 small comments/questions. logzero has grown from a tiny lib to a bit more. perhaps it's time to rethink the internal and custom logger setup and brainstorm on what could be good in a v2 ^^ |
* A couple of improvements based on feedback from @metachris in code review.
* Added some documentation around logging to `syslog`. * Added a primitive test of logging to syslog. No good way to capture syslog across all platforms, so only confirming that logging doesn't go to stdout or stderr currently.
|
All of my changes are done here now, @metachris! |
| logger_to_update.removeHandler(handler) | ||
|
|
||
|
|
||
| def syslog(logger_to_update=logger, facility=SysLogHandler.LOG_USER, disableStderrLogger=True): |
There was a problem hiding this comment.
I'd rather have disableStderrLogger default to False, as with the logfile setup. What do you think?
There was a problem hiding this comment.
I think you can argue that this method shouldn't have a disableStderrLogger argument at all and should just remove every internal logger, as the name implies. It's purely an internal method, anyway. I left the param just for consistency, but I do think it makes sense that the default behavior is to remove every internal logger, as the name implies.
To be honest, I don't really care for the way that disableStderrLogger is passed all around (and I'm the one that added it 😂 ). It's a reflection of the fact that we need a Logger class that carries these properties internally. I almost went down the path of doing that redesign yesterday but opted for the quicker implementation for now.
Given that this is purely an internal, private method, I'm inclined to leave it pending the OO-redesign, but if you'd rather we invert it, I can do that, too. 👍
There was a problem hiding this comment.
Alright, good for me :)
But I think removing all other loggers would be a bit hard, sometimes people want file and syslog together? idk... but logfile(..) just adds the file logger, gues syslog(..) should behave the same?
There was a problem hiding this comment.
This method is really only used when you're trying to setup a logger to work a specific way (e.g. with a log file or with syslog). I suppose someone may have a use case to do file and syslog together, but the current "flat" design doesn't readily support that. The problem is that there are usually default loggers added, but when I call the syslog method, I probably do not want those defaults (hence removing existing loggers first, just like the logfile method does).
If we refactored to an OO-design, then you will just define what loggers it should have via constructor args, and this method shouldn't generally be needed (except in the case where users are constantly changing around the default logger, but I don't know how common of a use case that is).
I think that trying to handle those types of multi-way logging scenarios with the current non-OO-design is going to make the code more complex, so my recommendation is we wait until the OO-design is in place and then we can support any arbitrary number of built-in loggers much more easily.
Thoughts?
There was a problem hiding this comment.
yeah, if we find the time we should move to a new design. hope we find the time, but there are so many priorities that i have the feeling it will be a while ^^
|
Thanks! Just a tiny thought, perhaps we leave it, but would like to hear your feedback on this. |
|
new pypy build is on the way (v1.5.0) |
logzeroto log directly to thesyslogfacility of your choice.Still more to come for testing, adding documentation, and perhaps streamlining the implementation a bit.