-
Notifications
You must be signed in to change notification settings - Fork 11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #76 #78
base: master
Are you sure you want to change the base?
Fixes #76 #78
Conversation
a7434d5
to
5f3496f
Compare
Thanks! I’m hoping to give this my full attention sometime over the weekend. |
BTW, I agree with you about feature flags; that started off as something truly global but over time has started to include things that should be per-logger. Part of the problem is that our loggers are created by the logging library (which appears to only support a global configuration). But it’s been ages since I looked at it so if you know of a more-specific configuration mechanism then I’m happy for us to use that. |
I suggest the following solution:
But I still have the following problems:
Or:
What say you, @tintoy ? |
Yeah, I think part of the problem is that python's logging module is essentially global
This was what
I'm hoping we can provide a more intuitive API to do this (just a function like
No argument here 😂 If we had to, I guess we could introduce a logger-name-or-regex approach to individual logger instance configuration, but again, it might start to feel a bit like reinventing the logging module 🤪
Yes - this feels much cleaner to me, too. I'm not too fussed about removing functionality that doesn't make sense as long as we sketch out a migration path for existing consumers in the docs ("migrating from v0.x" could include a workaround for the fact that |
I like the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor suggestions in the PR comments, but either way this looks good to me 🙂
c4f4a91
to
67e96cd
Compare
7a2b99d
to
8334e3e
Compare
@tintoy I've decided to practically completely refactor the project and move all of the configuration into the dictionary/YAML. I'm currently testing it on my staging environment for my production (so far everything works), and it seems that you can now configure it with a single command. I'm re-requesting a review. |
Cheers - will take a look at this later today 🙂 |
Will take a closer look on my computer but, at first glance (hooray for Code Spaces - even on an iPhone 😂), this looks great! |
Had a look on a full-size monitor and, yep, this looks great! |
Since this introduces some breaking changes, I'd invite @tintoy to express what should be left.
Right now everything can be configured using logging.config.dictConfig() including CLEF, override_root_logger and use_structured_logging
Also, this adds an option for ConsoleStructuredLogHandler to output to stderr. It's a config option.
I've tested it on my staging environment.
Also, I wonder, because right now by configuring one handler you can change the feature flags globally (for other loggers), shouldn't we move that to private properties of SeqLogHandlers? Because this behavior is not immediately obvious and somewhat astonishing.
Also, current implementation allows you to configure whether to use CLEF on the level of a ConsoleStructuredLogHandler, which is a code smell (and just plainly doesn't make sense, as a ConsoleStructuredLogHandler will never have to deal with CLEF), which amounts to, how one physicist put it as "spooky action at a distance".
I suggest we slowly shift towards per-handler configuration instead of global feature flags.