Skip to content

[v7r2] Make subLogger more flexible#4610

Merged
fstagni merged 1 commit into
DIRACGrid:integrationfrom
aldbr:rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs
Aug 14, 2020
Merged

[v7r2] Make subLogger more flexible#4610
fstagni merged 1 commit into
DIRACGrid:integrationfrom
aldbr:rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs

Conversation

@aldbr

@aldbr aldbr commented May 15, 2020

Copy link
Copy Markdown
Contributor

In response to: #4524

This PR makes gLogger more "flexible":

  • Use the logging level as it should be used*
  • The level of the Backend objects can now be set in the configuration
  • Possibility to set the level of the subloggers
  • Possibility to set the log record format of the sublogger (showHeaders() and showThreadIDs())
  • Two new methods to change the log record format:
    • showTimeStamps(): display or hide the timestamp
    • showContexts(): display or hide the component and the logger name
  • No more need to create a custom Formatter to wrap a Formatter just to use it in a backend
  • Deprecated methods have been deleted
  • Tests have been extended and are now made for pytest
  • Documentation has also been updated

BEGINRELEASENOTES
*FrameworkSystem
NEW: new methods to change the log record format: showTimeStamps and showContexts
CHANGE: make gLogger more flexible: level and format options can be set by subloggers
ENDRELEASENOTES

*About the level change:

The level is currently managed in a strange way:

  • the logger level is always debug
  • Logging.setLevel() just change the level of the Backends (so of the handlers) attached to the Logging

This was done in order to make sure that the centralized logging could not be disabled: all logs from all Logging objects go to a MessageQueue for instance.
In practice, subloggers don't have any Backend and just propagate the logs to LoggingRoot that spreads the logs across its Backends.
Thus, Logging.setLevel() has no effect from a sublogger.

In this PR, the Logging level is attached to the logger level to better respect the standard mechanism:

  • Logging.setLevel() works from every sublogger and change the level of its underlying logger
  • Backend level can still be changed from the configuration to only allow log records from a certain level

This means that the centralized logging can only get logs that respects the following condition: log record level > logger level:

  • the lower the Logging level, the more logs you will get in the centralized logging system
  • to only get logs from a certain level in a specific Backend, make sure to set its LogLevel in the configuration

@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from 38f1678 to d834d34 Compare May 16, 2020 12:37
@fstagni fstagni requested review from chaen and removed request for andresailer, atsareg and fstagni May 18, 2020 09:43
@fstagni

fstagni commented May 18, 2020

Copy link
Copy Markdown
Contributor

This is IMHO ok.

@andresailer andresailer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

some comments for tests and docstrings.

Comment thread FrameworkSystem/private/standardLogging/Formatter/BaseFormatter.py Outdated
Comment thread FrameworkSystem/private/standardLogging/Formatter/BaseFormatter.py Outdated
Comment thread FrameworkSystem/private/standardLogging/LoggingRoot.py
Comment thread FrameworkSystem/private/standardLogging/LoggingRoot.py Outdated
Comment thread FrameworkSystem/private/standardLogging/LoggingRoot.py
Comment thread FrameworkSystem/private/standardLogging/test/Test_Logging_Levels.py
Comment thread FrameworkSystem/private/standardLogging/test/Test_Logging_Levels.py Outdated
@chaen

chaen commented May 19, 2020

Copy link
Copy Markdown
Contributor

(from what I gather from private discussion) To make things very clear (@aldbr you should state that in the PR description), this means that there would be no way anymore (and rightly so) to set two backend to gLogger (say, stdout and logstash) and have the stdout running in level INFO while logstash runs DEBUG. Correct ?

@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from d834d34 to d1a0abf Compare May 19, 2020 14:52
@aldbr

aldbr commented May 19, 2020

Copy link
Copy Markdown
Contributor Author

(from what I gather from private discussion) To make things very clear (@aldbr you should state that in the PR description), this means that there would be no way anymore (and rightly so) to set two backend to gLogger (say, stdout and logstash) and have the stdout running in level INFO while logstash runs DEBUG. Correct ?

Alright, I've just added a comment about the level at the end of the PR description.

Well, yes and no.
You can still set the Backend level from the configuration, but it doesn't mean that you are going to get all the logs from this level.
For instance, let's say stdout level is INFO and logstash one is DEBUG:

logger.setLevel('notice')
logger.debug('message')

# stdout

# logstash

The log record 'message' is blocked at the Logging (logger) level because DEBUG < NOTICE and will not appear in the Backends.

logger.setLevel('debug')
logger.debug('message')

# stdout

# logstash
message

Here the log record is not blocked by the Logging because DEBUG = DEBUG, and can go the Backends (handler):

  • In stdout: DEBUG < INFO, the log record will not appear
  • In logstash: DEBUG = DEBUG, the log record will appear

Is it clear?

@chaen chaen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am happy if this goes to v7r1, since we (LHCb) will run more hackaton with it. But I believe FranceGrille (@atsareg) and GridPP (@marianne013 ) are already quite ahead with the use of v7r1, so it's only fair if they decide if it can go or not.

Comment thread Resources/LogBackends/MessageQueueBackend.py Outdated
Comment thread FrameworkSystem/private/standardLogging/Formatter/JsonFormatter.py
Comment thread FrameworkSystem/private/standardLogging/Logging.py Outdated
Comment thread FrameworkSystem/private/standardLogging/test/TestLogUtilities.py Outdated
@marianne013

Copy link
Copy Markdown
Contributor

We are only running v7r1 in pre-prod, but please don't break it any more than it already is.

@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from d1a0abf to 5e195d8 Compare May 27, 2020 07:50
@chaen

chaen commented May 27, 2020

Copy link
Copy Markdown
Contributor

@marianne013 😆 So that means v7r2 ? Fair enough.

@fstagni

fstagni commented Jun 8, 2020

Copy link
Copy Markdown
Contributor

@aldbr can you please rebase to v7r2? Then this can be approved and merged.

@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from 5e195d8 to 5d626f1 Compare June 8, 2020 11:21
@aldbr aldbr requested a review from zmathe as a code owner June 8, 2020 11:21
@aldbr aldbr changed the base branch from rel-v7r1 to integration June 8, 2020 11:21
@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch 2 times, most recently from d9af906 to 523bfb4 Compare June 9, 2020 07:53
@aldbr aldbr changed the title [v7r1?r2?] Make subLogger more flexible [v7r2] Make subLogger more flexible Jun 9, 2020
@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch 2 times, most recently from 4b4837c to c9f801b Compare June 18, 2020 07:23
@atsareg atsareg closed this Jun 29, 2020
@atsareg atsareg reopened this Jun 29, 2020
@atsareg

atsareg commented Jun 29, 2020

Copy link
Copy Markdown
Contributor

Please, rebase on the current integration branch

@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from c9f801b to f7490ee Compare July 1, 2020 05:54
@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from f7490ee to ad7a227 Compare July 1, 2020 06:15
fstagni
fstagni previously approved these changes Jul 7, 2020
@aldbr aldbr force-pushed the rel-v7r1_CHANGE_setLevelShowHeadersAndShowThreadIDs branch from ad7a227 to 5cf9340 Compare July 22, 2020 14:31
@fstagni fstagni closed this Aug 13, 2020
@fstagni fstagni reopened this Aug 13, 2020
@fstagni

fstagni commented Aug 13, 2020

Copy link
Copy Markdown
Contributor

I guess the test is failing because of cddf80c

@fstagni fstagni merged commit 048e10d into DIRACGrid:integration Aug 14, 2020
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.

6 participants