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.

Sign up for GitHub

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

Jump to bottom

logging: enhanced log level setting interface #16021

Merged
merged 1 commit into from Jun 3, 2021
Merged

logging: enhanced log level setting interface #16021

merged 1 commit into from Jun 3, 2021

Conversation

mvisonneau
Copy link
Contributor

@mvisonneau mvisonneau commented May 5, 2021 •
edited

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
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.

pkg/logging/logging.go Outdated Show resolved Hide resolved
pkg/logging/logging.go Outdated Show resolved Hide resolved
@joestringer
Copy link
Member

^ 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
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
Copy link
Contributor Author

mvisonneau commented May 7, 2021 •
edited

@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
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
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
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
Copy link
Contributor Author

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
Member

rolinh commented May 26, 2021

test-me-please

@rolinh
Copy link
Member

rolinh commented Jun 1, 2021

test-me-please

@rolinh
Copy link
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
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.

None yet
9 participants
@mvisonneau @joestringer @rolinh @glibsm @nathanjsweet @borkmann @christarazi @aditighag @aanm