Skip to content

logging: enhanced log level setting interface#16021

Merged
aanm merged 1 commit intocilium:masterfrom
mvisonneau:logging_level_refactoring
Jun 3, 2021
Merged

logging: enhanced log level setting interface#16021
aanm merged 1 commit intocilium:masterfrom
mvisonneau:logging_level_refactoring

Conversation

@mvisonneau
Copy link
Copy Markdown
Member

@mvisonneau mvisonneau commented May 5, 2021 •
edited
Loading

Enhance the logging interface so that various levels of logrus logging can be used rather than just hard toggling between "info" and "debug".

Managing log level is now similarily done as the log format:

  • default is set with pkg/logging.DefaultLogLevel
  • getters and setters have been standardized and leveraged
    across other libraries too, eg:
    • SetLogLevel()
    • SetLogFormat()
    • GetLogLevel()
    • GetLogFormat()

Follow-up: #16002 & #16005

logging: enhanced log level setting interface

@mvisonneau mvisonneau requested a review from a team May 5, 2021 10:23
@mvisonneau mvisonneau requested review from a team as code owners May 5, 2021 10:23
@mvisonneau mvisonneau requested a review from a team May 5, 2021 10:23
@mvisonneau mvisonneau requested review from a team as code owners May 5, 2021 10:23
@maintainer-s-little-helper maintainer-s-little-helper Bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 5, 2021
@mvisonneau mvisonneau requested a review from glibsm May 5, 2021 10:23
@joestringer joestringer added the release-note/misc This PR makes changes that have no direct user impact. label May 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper Bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label May 6, 2021
joestringer
joestringer requested changes May 6, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@joestringer joestringer 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 the submission. At a high level, this PR proposes to "enhance" the logging logic but I couldn't figure out what the enhancement is. I noticed a couple of log messages which are converted over to structured logging which is a nice cleanup, and there's a bunch of other churn which I honestly didn't follow.

What exactly is being enhanced here?

Couple of other minor comments to fix up below.

Comment thread pkg/logging/logging.go Outdated
Comment thread pkg/logging/logging.go Outdated
@joestringer
Copy link
Copy Markdown
Member

joestringer commented May 6, 2021

^ I should maybe add, the new code does seem a bit simpler at a glance. If that's the goal then I think I follow. If there's other bits I'm missing, please mention them since I wasn't able to pick them out from a brief glance through the PR ;)

@rolinh
Copy link
Copy Markdown
Member

rolinh commented May 7, 2021

@joestringer This PR makes sense to me. The function ConfigureLogLevel(debug bool) always bugged me because in effect, as it only takes a boolean value, it does not allow configuring the log level but rather only allows setting to level to debug. With the refactoring done in this PR, we now have the option to truly set the logging level to any supported log level.
I also find the code easier to reason about with these changes. Code such as the one below feels awkward:

logging.ConfigureLogLevel(false) // Use 'true' for debugging

@mvisonneau mvisonneau force-pushed the logging_level_refactoring branch from ae4c93b to 4bdca22 Compare May 7, 2021 13:20
@mvisonneau
Copy link
Copy Markdown
Member Author

mvisonneau commented May 7, 2021 •
edited
Loading

@joestringer, yes indeed, apologies for the vague description 🙇! I reckon that as you and @rolinh highlighted it is pretty much about having a pkg/logging interface which makes more sense and is easier to leverage across the components.

At first it emerged from a bug which I found regarding the non interpretation of the --log-options.level flag (#16002). I started to look into it and figured their was an opportunity to enhance it. However, @anfernee & @ti-mo made me realize it would be better to distinguish the 2 changes (#16005), hence this proposal!

I have incorporated your couple suggestions and rebased 👍

joestringer
joestringer approved these changes May 7, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM.

nathanjsweet
nathanjsweet requested changes May 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@nathanjsweet nathanjsweet left a comment

Choose a reason for hiding this comment

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

I like these changes, but I would like a better worded commit message.

Enhance the interface being used in order to configure the logging level
across the different components. This is an attempt to make it easier to understand
and use across the board compared to the current way of doing it.

Is quite vague. Please be more specific. Someting like:

Enhance the logging interface so that various levels of logrus logging can be used rather than just hard toggling between "info" and "debug"...

etc.

christarazi
christarazi approved these changes May 11, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM pending Nate's suggestion

@mvisonneau
logging: enhanced log level setting interface
8727ec5 
Enhance the logging interface so that various levels
of logrus logging can be used rather than just hard
toggling between "info" and "debug".

Managing log `level` is now similarily done as the log `format`:
- default is set with pkg/logging.DefaultLogLevel
- getters and setters have been standardized and leveraged
  across other libraries too, eg:
  - SetLogLevel()
  - SetLogFormat()
  - GetLogLevel()
  - GetLogFormat()

Signed-off-by: Maxime VISONNEAU <maxime.visonneau@gmail.com>
@mvisonneau mvisonneau force-pushed the logging_level_refactoring branch from 4bdca22 to 8727ec5 Compare May 12, 2021 17:26
@mvisonneau
Copy link
Copy Markdown
Member Author

mvisonneau commented May 12, 2021

thanks for the review, I updated the commit details and rebased upon master 👍

@rolinh rolinh requested a review from nathanjsweet May 25, 2021 08:42
@rolinh
Copy link
Copy Markdown
Member

rolinh commented May 26, 2021

test-me-please

@rolinh
Copy link
Copy Markdown
Member

rolinh commented Jun 1, 2021

test-me-please

@rolinh
Copy link
Copy Markdown
Member

rolinh commented Jun 2, 2021

Hit flake #16159, marking as ready to merge.

@rolinh rolinh added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 2, 2021
@aanm aanm merged commit 7a45cd4 into cilium:master Jun 3, 2021
@mvisonneau mvisonneau deleted the logging_level_refactoring branch July 1, 2021 19:48
@mvisonneau mvisonneau mentioned this pull request Jul 1, 2021
Closed
@mhofstetter mhofstetter mentioned this pull request Jun 12, 2024
Merged
mhofstetter added a commit to mhofstetter/cilium that referenced this pull request Aug 15, 2024
@mhofstetter
config: fix disabling config 'Debug'
513bbc0 
Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that cilium#16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
@mhofstetter mhofstetter mentioned this pull request Aug 15, 2024
Merged
github-merge-queue Bot pushed a commit that referenced this pull request Aug 20, 2024
@mhofstetter @tklauser
config: fix disabling config 'Debug'
0f76833 
Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter added a commit that referenced this pull request Aug 20, 2024
@mhofstetter
config: fix disabling config 'Debug'
5b770e3 
[ upstream commit 0f76833 ]

Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter added a commit that referenced this pull request Aug 20, 2024
@mhofstetter
config: fix disabling config 'Debug'
8a1ae3b 
[ upstream commit 0f76833 ]

Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
mhofstetter added a commit that referenced this pull request Aug 20, 2024
@mhofstetter
config: fix disabling config 'Debug'
80f2781 
[ upstream commit 0f76833 ]

Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
ldelossa pushed a commit that referenced this pull request Aug 21, 2024
@mhofstetter @ldelossa
config: fix disabling config 'Debug'
c9f37c7 
[ upstream commit 0f76833 ]

Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
github-merge-queue Bot pushed a commit that referenced this pull request Aug 21, 2024
@mhofstetter @ldelossa
config: fix disabling config 'Debug'
5d11152 
[ upstream commit 0f76833 ]

Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
ldelossa pushed a commit that referenced this pull request Aug 21, 2024
@mhofstetter @ldelossa
config: fix disabling config 'Debug'
19a13d3 
[ upstream commit 0f76833 ]

Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that #16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
jrajahalme pushed a commit to jrajahalme/cilium that referenced this pull request Oct 13, 2024
@mhofstetter @viktor-kurchenko
config: fix disabling config 'Debug'
213d756 
Currently, disabling the config property `Debug`
(`cilium-dbg config Debug=disable`) doesn't change the log level
of the Cilium Agent (and proxies) back to the default log level.
The log level stays at `debug`.

The reason is that cilium#16021 changed the logging API from accepting the
loglevel to explicit methods to enable debug level or reset it to the
defaut log level. Hence it no longer supports toggling the log level.

This commit fixes this by explicitly reset the log level of the default
logger by calling `logging.SetDefaultLogLevel()` if the config option
`Debug` is disabled.

Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@glibsm glibsm glibsm approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@borkmann borkmann borkmann approved these changes

@joestringer joestringer joestringer approved these changes

@rolinh rolinh rolinh approved these changes

@christarazi christarazi christarazi approved these changes

@aditighag aditighag aditighag approved these changes

Assignees

No one assigned

Labels

ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@mvisonneau @joestringer @rolinh @glibsm @nathanjsweet @borkmann @christarazi @aditighag @aanm