Open Vacant0mens opened 1 year ago
That sounds like a good solution, thanks for taking the initiative!
I'm happy for you to make this change if you're comfortable doing so 🙂
Out of curiosity, was there a reason the original design returned the StructuredLogHandler
class instead of the StructuredLogger
class, using the logging.getLogger()
call?
Can't remember anymore I'm afraid 😂
I'm guessing it was a lack of familiarity with the API semantics for custom logging components, but it was a while ago now. I'm certainly not wedded to that implementation detail 🙂
I was looking over the documentation for the logging module. looks like basicConfig()
, dictConfig()
, and fileConfig()
don't actually return anything when called. They just set up the logging config in the background and expect the user to call one of the logging functions to start using it.
i.e.:
>>> import logging
>>> logging.basicConfig(level=logging.INFO) # <- no output from this call
>>> logging.info("this is a log")
INFO:root:this is a log
>>>
I'd be okay refactoring the log_to_seq()
and log_to_console()
functions to set up the classes like I said (or maybe use dictConfig()
for easier management in the future?) and just let them get configured in the background in the same way.
Also, this python-logstash module looks pretty solid. I bet we could use that as an example for this enhancement, as well as that other one for adding synchronous-vs-asynchronous logging in Issue #44
Thoughts on the two topics?
Sure, that sounds like it could work - just as long as whatever solution we choose can still handle configuration of the root logger 🙂
(happy for you to have a go at it; maybe create a PR if you're ok with that?)
The main thing about log_to_seq
is that it encapsulates "necessary" configuration for seq (formatter, former style, log handlers, etc). Originally when I wrote it, it was because Seq we originally from the .NET world and I wasn't sure how familiar consumers would be with Python logging (and associated configuration) so it was enough to get them going as a kind of "quick start".
Yeah, and as a basic quick-start thing, it works as it is for the majority of basic use cases, but using basicConfig()
in the background like that isn't good for more advanced use cases (like if you have other Logger or Handler classes to set up aside from the ones imported from seqlog) because it says in the logging documentation that if you call basicConfig()
after the root logger already has at least one handler, that function does nothing. ... which I just realized means that if you call log_to_seq()
with override_root_logger=True
, it doesn't currently set up any other loggers besides the StructuredRootLogger
.
I'm just trying to think of the best way to keep the log_to_seq()
and log_to_console()
functions easy to use, but also make them more scalable.
Disclaimer
I'm not an expert with the seqlog module. I try to do my best research before posting issues on GitHub, but I'm only human. If any of the following is inaccurate, feel free to correct me.
Also, I am willing to do the coding work for this. I just wanted to (1) have a discussion about it first (if needed), (2) track my idea and its progress, and (3) make sure I wasn't misunderstanding the way that the seqlog module was put together.
Description
_(This is in response to issues #40 and #41 regarding the Usage documentation page, as well as the
seqlog.log_to_seq()
module-level function)_For clarity and flexibility, a
name
argument should be added tolog_to_seq()
(with a default value). It can't currently be provided by adding it as a kwarg, becauselogging.basicConfig()
doesn't accept aname
argument. If it's not set somewhere it results in the logger'sname
property being set to'__main__'
, which isn't easily searchable. (However, It is possible to calllogging.getLogger(name=__name__)
to get the currently-configured Logger instance if the current logger wasn't given a name, but this is not very user-friendly.)The stdlib logging documentation states:
While this may be true for a lot of use cases for the seqlog module, the
log_to_seq()
function should instantiate (and return) theStructuredLogger
class rather than creating a default instance of it and returningSeqLogHandler
. (in my opinion, returning the logger would make more sense to the users.) That way it's not assumed thatStructuredLogger
is the only logger or logger class that any given user will be using.Suggested Actions
log_to_seq()
function to accept aname
argument and give it a default value (something likeDefaultSeqLogger
would be fine, orDefaultSeqRootLogger
ifoverride_root_logger=True
).log_to_seq()
function, instantiate theStructuredLogger
class (withname
as given or its default value), assign the class a new instance ofSeqLogHandler
withloggerClass.addHandler()
(using the same arguments to instantiate it as are currently there).StructuredLogger
class and its handler/s.