Skip to content

Conversation

@alvinchow86
Copy link

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alvinchow86 This logger will fail is logging hasn't been initialized previously, so it will work for you but not for someone who doesn't initialize the logger elsewhere.

After the creation of sift_logger, check if sift_logger has any handlers, and if not, run the logging.basicConfig command.

Should look like this

sift_logger = logging.getLogger('sift_client')
if not sift_logger.handlers:
  logging.basicConfig()

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok I'll take a look. only thing is I'm running a Django app where I set a LOGGING dictionary in settings so I'm not explicitly calling logging.basicConfig/dictConfig/etc

@yschatzberg yschatzberg self-assigned this Jan 29, 2015
@yschatzberg
Copy link
Contributor

@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?

@TallGirlVanessa
Copy link

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:

  • It only adds a handler to the module's logger, not the root logger
  • It only adds the handler when the module's called, rather than when the module's imported, so that importing a module doesn't have surprising side effects

(That global statement's kind of annoying though. I'd refactor this into an object.)

@sww
Copy link

sww commented Dec 4, 2015

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.

@philfreo
Copy link
Contributor

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?

@TallGirlVanessa
Copy link

@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.

@sripadsriram
Copy link

cc @fredsadaghiani

@fredsadaghiani
Copy link
Contributor

Thanks, a better solution might be to just use warnings.warn(... rather than install a logger. Thoughts?

@JohnMcSpedon
Copy link

@fredsadaghiani's fix was implemented in #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

@philfreo
Copy link
Contributor

Thank you!

@TallGirlVanessa
Copy link

Great, thanks a bunch!

@fredsadaghiani
Copy link
Contributor

Closing. See #31 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants