Opened 3 years ago

Closed 3 years ago

#18993 closed New feature (fixed)

Default django logging to StreamHandler (when DEBUG=True)

Reported by: claudep Owned by: nobody
Component: Core (Other) Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently, logging from Django code (except for django.request and django.db.backends) is going by default to the bin (NullHandler). I think this is discouraging usage of logging in Django, hence I'd like to replace the NullHandler by a StreamHandler redirecting to stdout when DEBUG is True. Working on a patch...

Attachments (2)

18993-1.diff (3.8 KB) - added by claudep 3 years ago.
Output messages from django logger when DEBUG is True
18993-2.diff (7.6 KB) - added by claudep 3 years ago.
Second approach based on global_settings.LOGGING

Download all attachments as: .zip

Change History (14)

Changed 3 years ago by claudep

Output messages from django logger when DEBUG is True

comment:1 Changed 3 years ago by claudep

  • Has patch set

comment:2 Changed 3 years ago by ptone

this would dovetail nicely with #18985

comment:3 Changed 3 years ago by ptone

Looking at this - it seems we should be leveraging the settings.LOGGING dictConfig more.

The current module level code basically ensures that our 'django' logger exists in some minimum form.

What we are talking about for both this, and the deprecation warnings, is some degree of implicit config requirement if something isn't explicitly specified.

Meaning that even if we put the handler in the project template's settings.py dictConfig - deleting it does not remove the behavior, if you don't want it, you have to explicitly set it to a null handler.

Generally this kind of "I didn't ask for this, why are you giving it to me" approach isn't desirable.

I actually think the case for this behavior is stronger for deprecation. That is, if you want to silence warnings, you have to do it explicitly. However for the 'django' named handler, I think it is a harder sell. I'd be more inclined to set the default dictConfig for new projects to use the streamhandler (with the DEBUG check) by default, and if the handler is removed from the project's settings.py file, just ensure that a null handler exists for the logger as is currently done.

comment:4 Changed 3 years ago by claudep

I understand your point of view. The problem I see with your approach is for legacy projects with an existing LOGGING setting. If we begin to use more logging inside Django, they will wonder why they don't get the nice warnings they should... until they realize they have not the good loggers configured.

Changed 3 years ago by claudep

Second approach based on global_settings.LOGGING

comment:5 Changed 3 years ago by claudep

This second patch is another possible approach to configure a default handler by default for the 'django' logger.

comment:6 Changed 3 years ago by ptone

  • Needs documentation set

I still think that the logging config injection here:

https://code.djangoproject.com/attachment/ticket/18993/18993-2.diff#L9

is a bad idea. While it does provide the more visible logging for migrated projects - any warnings that a developer really should know about, should be proper warnings.warn, not logging.warning. I think that if a user deletes the relevant boilerplate logging config bits from their settings.py, the logging behavior should not just magically reappear in their console - only going away if they explicitly set the handler to nullhandler.

comment:7 Changed 3 years ago by claudep

After some thinking, I think there is a small flaw in the current Django logging config system. The LOGGING config in global_settings.py is overriden by any custom settings LOGGING. When we update LOGGING in global_settings, any upgraded project cannot take advantage of newer features in the LOGGING config.

My proposal would be:

  • Add DEFAULT_LOGGING in global_settings.py (with current LOGGING as content). This is *not* supposed to be overriden by project's settings.py (we can even imagine put it somewhere else, django.utils.log comes to mind). By default, global_settings.py LOGGING would be empty. We can still let the current content in project template LOGGING.
  • When configuring LOGGING (django.conf.__init__.py), first feed DEFAULT_LOGGING to the config function, then, if LOGGING is not empty, feed LOGGING to the config system.

Here are the possible scenarios:

  • If the user has set disable_existing_loggers to True in his project's LOGGING, nothing has changed for him. He's still in complete control of logging in his project.
  • If the user has no LOGGING in his settings, the situation is unchanged, Django default logging is configured.
  • If the user has set disable_existing_loggers to False in LOGGING (which is currently the default in the project template), then Django default logging is configured, and the user custom LOGGING is *added* to the Django default configuration.

This latest point is where we really gain from the current situation. We don't have to fiddle any more with the logging setup code.

I may be missing something in my reasoning. Comments welcome!

comment:8 Changed 3 years ago by ptone

This looks like a solid approach, and is cleaner than my idea on IRC of just using .update() on the global settings with the user settings version of the config (which would be another way of merging the two).

I think this happens as early as we are likely to get it to happen in Django, so the following point is minor. Perhaps a note in the docs that disable_existing_loggers is absolute, so that if its used to disable DJango's default logging, it will also disable any already loaded loggers from other libraries.

FWIW I do agree the default logging config should live outside of the global settings

comment:9 Changed 3 years ago by claudep

  • Needs documentation unset

The latest 2 commits of this Github branch have a proposed implementation:
https://github.com/claudep/django/commits/18993

comment:10 Changed 3 years ago by ptone

  • Triage Stage changed from Unreviewed to Ready for checkin

This looks good!

comment:11 Changed 3 years ago by Claude Paroz <claude@…>

In a014ddfef2f606471f25c756d97b3b50fcbd9e91:

Combined Django DEFAULT_LOGGING with user LOGGING config

Refs #18993.

comment:12 Changed 3 years ago by Claude Paroz <claude@…>

  • Resolution set to fixed
  • Status changed from new to closed

In f0f327bbfe1caae6d11fbe20a3b5b96eed1704cf:

Fixed #18993 -- 'django' logger logs to console when DEBUG=True

Thanks Preston Holmes for the review.

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