Closed alvinchow86 closed 8 years ago
@alvinchow86 Thanks for submitting this, before I can merge it we'd need to figure out how to maintain backwards compatibility. Did you have any suggestions on this?
Huh, ran into this yesterday. We started using sift and were surprised to find that our logging configuration got messed up, just by importing a new library, without even calling anything in it. I mean, on one hand I understand the desire to make things "just work". But I'd definitely vote for removing it and calling it out as a breaking change.
If sift does need to add logging handlers, I think it'd be better to do it the way Werkzeug does it (though it's worth noting that Werkzeug's a framework, not a library, so it's not surprising that it adds logging handlers). From https://github.com/mitsuhiko/werkzeug/blob/master/werkzeug/_internal.py#L75-L87 :
def _log(type, message, *args, **kwargs):
"""Log into the internal werkzeug logger."""
global _logger
if _logger is None:
import logging
_logger = logging.getLogger('werkzeug')
# Only set up a default log handler if the
# end-user application didn't set anything up.
if not logging.root.handlers and _logger.level == logging.NOTSET:
_logger.setLevel(logging.INFO)
handler = logging.StreamHandler()
_logger.addHandler(handler)
getattr(_logger, type)(message.rstrip(), *args, **kwargs)
Then it's called like this:
_log('info', ' * Restarting with reloader')
Advantages:
(That global
statement's kind of annoying though. I'd refactor this into an object.)
How about
API_URL = 'https://api.siftscience.com'
sift_logger = logging.getLogger('sift_client')
if not logging.root.handlers and sift_logger.level == logging.NOTSET:
sift_logger.setLevel(logging.INFO)
handler = logging.StreamHandler()
sift_logger.addHandler(handler)
?
It's a more focused approach and would not affect the global logging setup.
We also installed sift
and were unhappy to find out that using the client messes up all our existing logging. @yschatzberg Any chance for a fix here soon to not call basicConfig
in all cases?
@sww That's better, but I'd still argue installing handlers on import is unexpected behavior, especially for a library. Werkzeug's check for a root handler only works because it runs after client code has been initialized.
cc @fredsadaghiani
Thanks, a better solution might be to just use warnings.warn(...
rather than install a logger. Thoughts?
@fredsadaghiani's fix was implemented in https://github.com/SiftScience/sift-python/pull/31
I've pushed a new version of this library to PyPI.
pip install --upgrade sift
will now install sift-python 1.1.2.2, which should no longer affect existing logging
config
Thank you!
Great, thanks a bunch!
Closing. See #31 instead.
this library shouldn't be calling
logging.basicConfig()
, since it's a global configuration and will muck with existing logging setups. I think this call is meant for the top-level application to control