Skip to content
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

Disable logger propagation by default #1993

Merged
merged 1 commit into from
Sep 12, 2016
Merged

Conversation

mitsuhiko
Copy link
Contributor

@mitsuhiko mitsuhiko commented Aug 25, 2016

Main motivation for this is that logging.basicConfig() otherwise logs
all things twice by default since the logger propagates upwards to the
default handler.


This change is Reviewable

@untitaker
Copy link
Contributor

I feel like a single logging handler should be used at the root.

@mitsuhiko
Copy link
Contributor Author

@untitaker you mean remove our own default handler entirely? Generelly I would like to merge this for next release. It tremendously improves what happens if you configure sentry/raven/bugsnag etc.

@untitaker
Copy link
Contributor

Yeah exactly that. What does sentry do wrt logging?

@mitsuhiko
Copy link
Contributor Author

@untitaker sentry is not that important. Anything that configures the logging system will break currently. You can just issue logging.basicConfig() and you get all exceptions twice. It just shows up with those things like sentry more because errors are reported twice there in the UI. (Though sentry is less affected since it dedupes in the raven client. Others do not).

@untitaker
Copy link
Contributor

I feel like you haven't really said what you think of this, or why it is not an alternative:

I feel like a single logging handler should be used at the root.

@mitsuhiko
Copy link
Contributor Author

@untitaker as i said I'm not sure what you mean by that. Do you mean we should not configure our own log handler? Because we can't change what logging.basicConfig() does which does install a default handler.

@untitaker
Copy link
Contributor

I replied with "yeah exactly that"

@untitaker
Copy link
Contributor

Also note that I don't have any opinion on that topic, this is just for
the sake of having evaluated all alternatives <3

On Sun, Sep 11, 2016 at 01:24:54AM -0700, Armin Ronacher wrote:

@untitaker as i said I'm not sure what you mean by that. Do you mean we should not configure our own log handler? Because we can't change what logging.basicConfig() does which does install a default handler.

You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#1993 (comment)

@mitsuhiko
Copy link
Contributor Author

I will merge this.

@mitsuhiko mitsuhiko merged commit e00e2c2 into master Sep 12, 2016
@untitaker
Copy link
Contributor

I would still like to hear some sort of rationale, longer or shorter. Not because I want to educate myself at the expense of your time, but because I think it will serve as a useful reference when later this behavior will possibly be changed again.

@davidism davidism deleted the feature/logger-propagation branch April 20, 2017 19:52
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants