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

Improve default log configuration (DM-7955) #20

Merged
merged 1 commit into from Oct 20, 2016
Merged

Conversation

andy-slac
Copy link
Contributor

Default message format changed to "loggername LEVEL: message" and
default logging level is set to INFO. I think I also managed to simplify
configuration process so it looks simpler now. Documentation is updated
to reflect all changes.

Copy link

@hsinfang hsinfang left a comment

Choose a reason for hiding this comment

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

Thanks for updating the documentation!

And I like the updated configuration process; it does look simpler.

* If LSST_LOG_CONFIG envvar is defined and points to existing file then
* use that file to configure (can be either XML or Properties file).
* Otherwise use pre-defined configuration - add console appender to root
* logger using pattern layout with the above pattern, level set to WARN.

Choose a reason for hiding this comment

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

INFO instead of WARN?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, indeed. I started implementing Robert's proposal but then switched back to INFO, so it wen out of synch in few places.

Test basic log output. Since the default threshold is INFO, the
TRACE message is not emitted.
Test basic log output with default configuration.
Since the default threshold is WARN, the INFO, DEBUG, or TRACE

Choose a reason for hiding this comment

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

INFO instead of WARN as default in this patch

import logging
lgr = logging.getLogger()
lgr.setLevel(logging.INFO)
lgr.addHandler(log.LogHandler())
log.configure()
log.setLevel('', log.INFO)

Choose a reason for hiding this comment

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

I'm not sure if these two lines are needed. This logger's level is set 3 lines above (lgr.setLevel(logging.INFO)).

Copy link
Contributor Author

@andy-slac andy-slac Oct 20, 2016

Choose a reason for hiding this comment

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

These are two separate loggers, one is Python logging logger which is set to WARNING level by default so we need to set its level to enable logging INFO messages. The log is LSST logger, this again was needed when I started doing WARN as default but with default set to INFO it's not really need, I'll remove it.

- single `ConsoleAppender` is added to the root logger,
- the output of the appender is sent to standard output,
- the output is formatted using a `PatternLayout` set to the pattern `"%c %p: %m%n"`,
- logging level is set to `INFO`, suppressing `DEBUG` and `TRACE` messages by default.

Choose a reason for hiding this comment

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

It might be good to update the descriptions later around line 211 about the BasicConfigurator example too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated that too.

Default message format changed to "loggername LEVEL: message" and
default logging level is set to INFO. I think I also managed to simplify
configuration process so it looks simpler now. Documentation is updated
to reflect all changes.
@andy-slac andy-slac merged commit ad3b865 into master Oct 20, 2016
@ktlim ktlim deleted the tickets/DM-7955 branch August 25, 2018 05:18
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.

None yet

2 participants