🐛 pkg/log: Set Log to NullLogger after 30 seconds #1309
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
approved
alvaroaleman
force-pushed
the
remove-delg
branch
from
95f7e32
to
5697ad5
Compare
k8s-ci-robot
added
size/M
The delegating Logger will cause a high number of memory allocations when no actual Logger is set. This change makes us set a NullLogger after 30 seconds if none was set yet to avoid that.
alvaroaleman
force-pushed
the
remove-delg
branch
from
5697ad5
to
ad718e3
Compare
alvaroaleman
changed the title
|
|
||
| var ( | ||
| loggerWasSetLock sync.Mutex | ||
| loggerWasSet bool |
There was a problem hiding this comment.
We could also use an atomic bool?
There was a problem hiding this comment.
Meant something like https://golang.org/pkg/sync/atomic/#CompareAndSwapInt64, this is really low priority though
| loggerWasSetLock.Lock() | ||
| defer loggerWasSetLock.Unlock() | ||
| if !loggerWasSet { | ||
| Log.Fulfill(NullLogger{}) |
There was a problem hiding this comment.
Do we want to print out a warning or error of some sort before transitioning the logger?
It seems that the delegating logger default might not be the best choice if it could potentially fill up memory if used for a long period of time.
We could also change direction here and default to a well known logger instead?
There was a problem hiding this comment.
Do we want to print out a warning or error of some sort before transitioning the logger?
Hm good idea, but what logger should we use? Generally, this doesn't change anything unless someone changed the logger after more than 30 seconds before which I think is unlikely
It seems that the delegating logger default might not be the best choice if it could potentially fill up memory if used for a long period of time.
See the description in the PR, we need it because various packages construct a logger via init which always happens before any non-controller-runtime code is executed. Without the delegating logger it is impossible to set the logger for those. It is only inefficient if no logger is set.
We could also change direction here and default to a well known logger instead?
Sure
There was a problem hiding this comment.
Yeah that makes sense, I was thinking when we're transitioning we could print an error to stderr via the stdlib log package
There was a problem hiding this comment.
The alternative is to just default to klogr and let folks use a different logger if they want to, we could do the init of the default logger early when the package loads up, which should solve the problem described above
There was a problem hiding this comment.
I don't think it properly solves the problem, because right now you can set the global base logger for everything via SetLogger (which IMHO is convenient and useful). What you describe would only allow to override the default logger within the components.
Also, c-r doesn't use klogr anywhere that I am aware, it uses zap with a logr.Logger wrapper.
So right now the whole thing is documented as not logging anything if SetLogger is not called, so changing that would make this PR break compatibility, hence I would prefer to stick with the NullLogger so this can be backported. WDYT?
There was a problem hiding this comment.
That sounds good for now yeah, I was more brainstorming for later / long term
|
/approve |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, vincepri The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/milestone v0.8.x |
Fixes #1122
/assign @DirectXMan12 @vincepri