feat(Logs): allow configuration of logging levels #80
Conversation
richardkeit
force-pushed
the
log_levels
branch
from
8a85a40 to
96e7b77
Compare
There was a problem hiding this comment.
Good catch - I think given this is a library intended for import we shouldn't be setting logging levels other than our own
But I'd probably prefer to remove the boto/nose etc log level setting as that's really beyond the scope of what we should be doing as you correctly point out.
I think INFO is a reasonable default as well, we don't have any DEBUG logging but just in case.
staxapp/config.py
Outdated
| logging.getLogger("botocore").setLevel(logging.WARNING) | ||
| logging.getLogger("nose").setLevel(logging.WARNING) | ||
| logging.getLogger("urllib3").setLevel(logging.WARNING) | ||
| logging.getLogger().setLevel(os.getenv("LOG_LEVEL", logging.DEBUG)) |
There was a problem hiding this comment.
| logging.getLogger().setLevel(os.getenv("LOG_LEVEL", logging.DEBUG)) | |
| logging.getLogger().setLevel(os.getenv("LOGLEVEL", logging.INFO)) |
There was a problem hiding this comment.
Does the existing convention in this file not lend itself more to LOG_LEVEL? (eg STAX_REGION, API_VERSION)
There was a problem hiding this comment.
That's fair as well - I'll happily accept either (the requested change to INFO aside)
The suggestion for LOGLEVEL was to fit what I could see libraries / projects using in the absence of a convention (I had a bit of a look around but nothing exhaustive) but it was not a strongly held position, I probably confused the issue by including it on the suggestion 🙂
There was a problem hiding this comment.
Thanks @Anton0. We internally had some debate a few years ago about this same thing - LOG_LEVEL vs LOGLEVEL.
RE: the changing of boto3/botocore, I think it's reasonable to keep the WARNING. It was the "ALL" loggers to default which I thought was a bit too verbose.
|
@richardkeit are you able to rebase now that #81 has been merged and we can get this one in as well. |
|
@richardkeit just suggested swapping the values so default is INFO and the example is moving it to DEBUG. So everyone has a better default (than we do currently). |
Hey @samdammers - typically I try keep changes backwards compatible, but if you're happy with change it, so am I |
|
👌 |
Codecov Report
@@ Coverage Diff @@
## master #80 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 8
Lines 434 434
=========================================
Hits 434 434
|
Feature - Logging Levels
Current behaviour
Currently, this library unapologetically sets log levels across all loggers without configuration.
Results in all modules (outside of this library) logging in DEBUG, eg:
I would love to set some more sensible defaults, but have left the defaults as the original authors intended.
New functionaility will allow users customize to their preference