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
Make logging.InitializeDefaultLogger private #29495
Make logging.InitializeDefaultLogger private #29495
Conversation
86cbd02
to
4e820dc
Compare
/test |
The release notes are user-facing and thus ideally should document the impact a user will see because of this change. How about saying what issue this PR fixes? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI failures in Test/EndpointSuite/TestEndpointLogFormat
look related.
4e820dc
to
bdab87c
Compare
Ah good point, I'll update it. |
bdab87c
to
350c424
Compare
/test |
The function InitializeDefaultLogger creates a new logger with the default settings and returns it. This commit removes a call to this function that doesn't save the return value, essentially calling it for no reason. Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
The DefaultLogger variable in the logging module serves as a parent logger which all other loggers can be derived from. It is initialized using the InitializeDefaultLogger function and then adjusted on startup based on user configuration. Users should not call InitializeDefaultLogger to create a parent logger for their logger, since the logger returned by InitializeDefaultLogger will always use the hardcoded defaults. For example, the logger returned will always be of level INFO, even if a user has enabled debug logging. To make this clear, this commit renames InitializeDefaultLogger to initializeDefaultLogger to signal that it should not be used outside of the logging module. Fixes: cilium#29215 Signed-off-by: Ryan Drew <ryan.drew@isovalent.com>
350c424
to
7cdd9e6
Compare
Rebased to pull in #29455 fixing ci-runtime |
/test |
According to #29544 (comment) it seems that #29215 also occurs on v1.13 (and presumably also v1.14). I've added the corresponding labels but I assume that they might need manual backports. |
Sounds good @tklauser, I'll keep a look out. |
@learnitall Indeed this will need manual backports, as the changes depend on changes made in #26327 and #23971 which were not backported. I will skip this PR in the current round of backports and added the |
Hi, FYI I opened a backport PR for 1.13 since this bug affects us in that version: #29700 |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
This PR renames the function
logging.InitializeDefaultLogger
tologging.initializeDefaultLogger
in order to make it private to the logging module. This function should only be used to initialize thelogging.DefaultLogger
variable, and should not be used by users as a parent logger. This is because the logger returned bylogging.InitializeDefaultLogger
will always use the hard-coded default logging settings, whilelogging.DefaultLogger
is adjusted based on the user configuration.The changes in
pkg/endpoint/log.go
remove the use oflogging.InitializeDefaultLogger
while also fixing #29215.Fixes: #29215