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.

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

🐛 pkg/log: Set Log to NullLogger after 30 seconds #1309

Merged
merged 1 commit into from
Jan 6, 2021

Conversation

alvaroaleman
Copy link
Member

@alvaroaleman alvaroaleman commented Dec 24, 2020

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.

Fixes #1122

/assign @DirectXMan12 @vincepri

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Dec 24, 2020
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 24, 2020
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 24, 2020
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 alvaroaleman changed the title ⚠ pkg/log: Default to NullLogger instead of DelegatingLogger 🐛 pkg/log: Set Log to NullLogger after 30 seconds Dec 24, 2020

var (
loggerWasSetLock sync.Mutex
loggerWasSet bool
Copy link
Member

Choose a reason for hiding this comment

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

We could also use an atomic bool?

Copy link
Member Author

Choose a reason for hiding this comment

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

atomic bool?

Copy link
Member

Choose a reason for hiding this comment

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

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{})
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah that makes sense, I was thinking when we're transitioning we could print an error to stderr via the stdlib log package

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

@vincepri vincepri Jan 4, 2021

Choose a reason for hiding this comment

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

That sounds good for now yeah, I was more brainstorming for later / long term

@vincepri
Copy link
Member

vincepri commented Jan 6, 2021

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 6, 2021
@k8s-ci-robot
Copy link
Contributor

[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:
  • OWNERS [alvaroaleman,vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@vincepri
Copy link
Member

vincepri commented Jan 6, 2021

/milestone v0.8.x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DelegatingLogger seems to be a memory hog
4 participants