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

Made logging config examples more accessible. #10908

Merged
merged 1 commit into from
Mar 17, 2020

Conversation

theGOTOguy
Copy link
Contributor

Documentation update which clarifies that your project's name must be included among the loggers in order to actually catch log messages from your internal libraries. They will not by default be caught by Django.

From the existing documentation, it appeared to me that 'django' was the catch-all logger for all log messages thrown by my application. In fact, this is not the case. Any log message originating from my app that isn't directly coming from a Django library is silently dropped. In order to actually include these messages, it is necessary to include an additional entry among the loggers, indicating the app name.

@timgraham
Copy link
Member

I'm not sure. There's some assumption that you read Python's documentation to know the basics of how logging works.

@theGOTOguy
Copy link
Contributor Author

Don't you think that, at the very least, the basic example given in the documentation should be a sensible default that would log all of the messages from the user's application?

@timgraham
Copy link
Member

I'm not sure. There are many ways to do logging. The example assumes a logger named 'myproject' but an application could just as well use a 'django' logger.

@theGOTOguy
Copy link
Contributor Author

Sure, you could use a 'django' logger, but again, that's not what you do in your example. You use the more standard

logger = logging.getLogger(__name__)

Even if you don't like my specific wording, I think it's worth either including an example that would log all messages from someone's own Django app since that is probably the desired behavior from almost all developers, or otherwise to clarify that 'django' does not necessarily catch all log messages coming from a Django app, and that the app name is a better catch-all logger.

@timgraham
Copy link
Member

Which version of the documentation are you reading? Does it have the clarifications from f21915b? (Django 2.0 and later)

I guess I could see adding something to the effect of, "If your application logs messages with different logger names, you'll have to add those names to your configuration." but it seems kind of obvious to me when it's phrased like that.

@theGOTOguy
Copy link
Contributor Author

@carltongibson
Copy link
Member

carltongibson commented Jan 30, 2019

I see a lot of people struggle with this.

I think the example that would be handy would be a simple root logger config that sent everything to the console hander. With something like "Here's a simple configuration if you just want to log everything to the console in development..."

I think once people get that working, they're able to go beyond that with relative ease, learning from the other examples and the Python docs that are linked.

@theGOTOguy
Copy link
Contributor Author

I've updated the docs in the PR, modifying only the second example to use the root logger ('') instead of 'django'. Since this example was meant to be useful for development, it is probably the most appropriate one to modify in this way since in development users will likely wish to see all log messages printed to the console, not just Django's.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hmmm. How about make do with a sentence after the existing example with a "The django here refers to..., as discussed in Naming Loggers above..." perhaps with a discussion of the root logger in Naming Loggers?

(I can have a think about exacting wording if you like, but I don't think we're far off, giving the whole thing another read. But yes, people do have issues...)

I prefer to use an explicit 'root'.

@theGOTOguy
Copy link
Contributor Author

I tried 'root' in my tests, and doesn't echo any log information at all to the console (though, this was in a project using Django 2.0.3). Is this explicit 'root' logger new in Django 2.1?

@ngnpope
Copy link
Member

ngnpope commented Feb 3, 2019

I prefer to use an explicit 'root'.

That will not work. Propagation of loggers works based on string prefixes, which is why the root logger is an empty string.

Is this explicit 'root' logger new in Django 2.1?

Please don't take this the wrong way, but the suggestion and this response suggest that you both need to thoroughly read the Python logging documentation to understand how it works. I have struggled in the past with this too!


I'm -1 on changing the configuration by default, but clarifying how things work, within reason, makes sense. This documentation is about configuration of logging for Django, not any third-party package and the Python documentation should be sufficient. That is not to say that developers cannot explicitly add additional loggers in their configuration. Also, adding the root logger without disabling propagation on other loggers could lead to duplicate messages and extra I/O.

The django logger serves a useful purpose and contains messages logger by the framework that would otherwise be swallowed. If developers use __name__ for their logger and their project is called myproject then the examples in the documentation provide a logger for that and everything for the project with be captured by that logger.

@ngnpope
Copy link
Member

ngnpope commented Feb 3, 2019

...project is called myproject then the examples in the documentation...

Apologies, I see your first commit proposed adding something like this which seems reasonable. Am just not keen on a catch-all using the root logger.

@carltongibson
Copy link
Member

I prefer to use an explicit 'root'.

That will not work. Propagation of loggers works based on string prefixes, which is why the root logger is an empty string.

Sorry, what I meant was setting the 'root' key explicitly in the config dictionary. (As documented here.) I use this regularly (so it definitely works 🙂).

This kind of thing:

    'root': {
        'level': 'INFO',
        'handlers': ['console', ],
    }

But, this is sibling to the loggers key, not a child of it. I should have been clearer.

Thinking about this, I reckon™ that the issue users have is too-quickly scanning to the code example and not understanding the hierarchy. Hence my thought about pointing back to the Naming loggers section, so there's more encouragement to read it.

@theGOTOguy
Copy link
Contributor Author

theGOTOguy commented Feb 3, 2019

Ah, sorry, yeah, I put 'root' under the 'loggers' hierarchy. I thought that seemed wrong, but that maybe some alias was being added by Django behind the scenes.

So, to be clear, would the preferred example use the root logger:

LOGGING = {
    'version': 1,
    'disable_existing_loggers': False,
    'handlers': {
        'console': {
            'class': 'logging.StreamHandler',
        },
    },
    'root': {
        'level': 'level': os.getenv('DJANGO_LOG_LEVEL', 'INFO'),
        'handlers': ['console']
    },
}

or would it be more desirable to use my original example (which uses myproject, like another example in the logging documentation)?

    LOGGING = {
        'version': 1,
        'disable_existing_loggers': False,
        'handlers': {
            'console': {
                'class': 'logging.StreamHandler',
            },
        },
        'loggers': {
            'django': {
                'handlers': ['console'],
                'level': os.getenv('DJANGO_LOG_LEVEL', 'INFO'),
            },
            'myproject': {
                'handlers': ['console'],
                'level': os.getenv('DJANGO_LOG_LEVEL', 'INFO'),
            },            
        },
    }

Once there's some consensus on the preferred example, I can update the documentation to suit.

@ngnpope
Copy link
Member

ngnpope commented Feb 3, 2019

But, this is sibling to the loggers key, not a child of it. I should have been clearer.

Gotcha.

I guess it comes back to Tim's point about reading the Python logging documentation. At the end of the day, Django does nothing particularly special with logging that isn't detailed in the existing documentation and we need to draw the line somewhere before we copy all of Python's logging documentation into Django.

@theGOTOguy
Copy link
Contributor Author

I don't mean to suggest that the Python logging documentation should be part of Django's, but rather just that the example that most developers are going to cut-and-paste into their own settings should be one that is going to give reasonable behavior, which is to report the log messages for their own application either to a file or to the console.

@carltongibson
Copy link
Member

carltongibson commented Feb 4, 2019

My issue with the myproject addition is that it probably isn't going to solve users' problem:

Typical layout:

manage.py
myapp
mynewapp
myproject

A startproject and a couple of startapp calls.

Doing it the 'myproject' way, you probably want three loggers to capture all your app output. As such I'm not sure it's much of an improvement.

For me, I'd add the 'root' example first with "Here's the simplest configuration to output everything to the console".

Then just have the existing examples, with the ordinal numbers changed.

I appreciate people should read the Python docs and How-to on logging, but people do read those, and still struggle to get a working config.

(The 'root' example isn't that silly either. It's a great way to get logging to whatever process manager is in play.)

@theGOTOguy
Copy link
Contributor Author

That sounds like a solid plan to me. Certainly, if this had been the first example in the documentation it would have saved me some time. The newest commit reflects your suggestion.

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

This looks kind of how I'd write it. (As per previous comments. 🙂)

So...

  1. What do others think? Do you see how it helps the beginner get logging up and running before they understand all about Python logging (which is a bit opaque IMO)
  2. Just where we introduce the django logger in the Second... example, under the code, would a sentence like "Here django refers to the logger name and will include any child loggers, as described in Naming loggers above" go well? (i.e. linking back to help guide folks to actually reading it.)

Thanks for the effort @theGOTOguy!

Copy link
Member

@carltongibson carltongibson left a comment

Choose a reason for hiding this comment

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

Hi @theGOTOguy. Thanks again for this. Sorry it's sat here for so long: because there was no attached ticket it slipped through the gaps.

I've adjusted the patch a little bit and rebased on the latest master.

I think it's a nice clarification. Welcome aboard! ⛵️

@carltongibson carltongibson changed the title Update to logging documentation Made logging config examples more accessible. Mar 11, 2020
- Show an initial example configuring the root logger to output to the console.
- Then add more logging from the django named logger.
- Then show the file and more complex examples.

Adjusted surrounding text for reading flow.

Co-authored-by: Carlton Gibson <carlton.gibson@noumenal.es>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants