Opened 6 weeks ago

Closed 6 weeks ago

Last modified 6 weeks ago

#30553 closed Cleanup/optimization (fixed)

Misleading logging documentation about disable_existing_loggers default value.

Reported by: darthdragon Owned by: Swatantra
Component: Documentation Version: master
Severity: Normal Keywords: disable_existing_loggers logging
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: yes UI/UX: no

Description

In the documentation it's stated that:

"If the disable_existing_loggers key in the LOGGING dictConfig is set to True (which is the default) ..."

But we can see in https://github.com/django/django/blob/master/django/utils/log.py (master branch commit 10b44e4 at the time of writting) that it is set to False instead:

...
DEFAULT_LOGGING = {
    'version': 1,
    'disable_existing_loggers': False,
    'filters': {
...

Moving "(which is the default)" in the paragraph should be enough:

"If the disable_existing_loggers key in the LOGGING dictConfig is set to True then all loggers from the default configuration will be disabled. Disabled loggers are not the same as removed; the logger will still exist, but will silently discard anything logged to it, not even propagating entries to a parent logger. Thus you should be very careful using 'disable_existing_loggers': True; it’s probably not what you want. Instead, you can set disable_existing_loggers to False (which is the default) and redefine some or all of the default loggers; or you can set LOGGING_CONFIG to None and handle logging config yourself."

Change History (11)

comment:1 Changed 6 weeks ago by felixxm

Easy pickings: set
Summary: Misleading logging documentation about disable_existing_loggers default valueMisleading logging documentation about disable_existing_loggers default value.
Triage Stage: UnreviewedAccepted
Type: UncategorizedCleanup/optimization
Version: 2.2master

Thanks for the report, it seems that this note is incorrect since its introduction in 095643e69145d6899313c518fdd39919c9a89908 because we changed that in 72c65fea41a6a01f24e134e7627417d94746291a.

comment:2 Changed 6 weeks ago by Carlton Gibson

I think what's meant here is the default for dictConfig if disable_existing_loggers isn't provided.

If absent, this parameter defaults to True.

https://docs.python.org/3.7/library/logging.config.html#dictionary-schema-details

I guess the point is that it's important to provide disable_existing_loggers: False because often the default behaviour is not what you want. (Why aren't my loggers working? comes up a lot because of this.)

For me, it'd be worth a rephrase to clarify this. (We see a lot of confusions.)

comment:3 Changed 6 weeks ago by Carlton Gibson

Yeah, it seems this kind of thought was exactly what led to #20981 in the first place.

comment:4 Changed 6 weeks ago by felixxm

You're right I missed that in dictionary-schema-details.

comment:5 Changed 6 weeks ago by darthdragon

I think these two sentences are what may start the headache:

"By default, the LOGGING setting is merged with Django’s default logging configuration using the following scheme.

If the disable_existing_loggers key in the LOGGING dictConfig is set to True (which is the default) then all loggers from the default configuration will be disabled. ...

Because even after reading it 10 times, if you have never looked to python logging.config (and I didn't until today, shame on me), you may not understand that also any loggers that may have been defined before are also disabled.

And maybe clarifying in "Django’s default logging configuration", that the default django loggers are added with disable_existing_loggers=False.

comment:6 Changed 6 weeks ago by Swatantra

Owner: changed from nobody to Swatantra
Status: newassigned

comment:7 Changed 6 weeks ago by felixxm

Has patch: set

comment:8 Changed 6 weeks ago by Carlton Gibson

Patch needs improvement: set

As per comment on PR, I think the solution here should just clarify that the "default" in question if that of dictConfig, rather than referring to the value used by Django's DEFAULT_LOGGING during startup.

comment:9 Changed 6 weeks ago by Carlton Gibson

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:10 Changed 6 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

Resolution: fixed
Status: assignedclosed

In 03cd3d1:

Fixed #30553 -- Clarified the default value of disable_existing_loggers.

comment:11 Changed 6 weeks ago by Mariusz Felisiak <felisiak.mariusz@…>

In 6dca3361:

[2.2.x] Fixed #30553 -- Clarified the default value of disable_existing_loggers.

Backport of 03cd3d137e2c29484b020b9768a4741f1096be97 from master

Note: See TracTickets for help on using tickets.
Back to Top