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

Use logger from context in Reconcile() #550

Merged
merged 3 commits into from
Jan 8, 2021
Merged

Use logger from context in Reconcile() #550

merged 3 commits into from
Jan 8, 2021

Conversation

ChunyiLyu
Copy link
Contributor

@ChunyiLyu ChunyiLyu commented Jan 7, 2021

Relates to previous PR: #542

Summary Of Changes

With controller-runtime 0.7, Reconcile() takes a context.Context object with a logger. The logger is pre populated with controller name, gvk, object name and namespace. Operator using this logger reduces redundancy because operator no longer needs to create its own logger and it doesn't need to include rmq name and namespace manually in every single log.

Before this change, a sample log could look like:

2021-01-07T12:46:12.232Z	INFO	rabbitmqcluster-controller	Finished reconciling RabbitmqCluster	{“namespace”: “rabbitmq-system”, “name”: “sample”}

After this change, the same log will look like:

2021-01-07T16:27:38.661Z	INFO	controller-runtime.manager.controller.rabbitmqcluster	Finished reconciling	{"reconciler group": "rabbitmq.com", "reconciler kind": "RabbitmqCluster", "name": "sample", "namespace": "rabbitmq-system"}

Additional Context

Related controller runtime PR: kubernetes-sigs/controller-runtime#1054

Local Testing

Unit, integration, and system tests all passed.

- with controller-runtime 0.7, Reconcile() takes a
context with a logger. The logger is pre populated with
controller name, gvk, object name and namespace.
Copy link
Member

@MirahImage MirahImage left a comment

Choose a reason for hiding this comment

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

This is quite a nice improvement enabled by the context logger.

@ansd ansd self-requested a review January 8, 2021 13:36
Copy link
Collaborator

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

We are passing the logger to the helper functions that need to log, in addition to the context. I'm wondering if we could do log.FromContext(ctx) inside the helper functions and get the same logger, given that we are already passing the context to this functions?

Also, remove logger from method signatures since it's already part of
context.
@ansd
Copy link
Member

ansd commented Jan 8, 2021

@ChunyiLyu I took the liberty to push 2 more commits. Feel free to amend or revert if you think they don't make sense :)
@Zerpet, your suggestion is done in 1541c61.

@ansd ansd requested a review from Zerpet January 8, 2021 14:33
@ChunyiLyu
Copy link
Contributor Author

@ansd thanks David! I will squash the commit and merge after @Zerpet approves ☺️

@ansd ansd merged commit 9b239ea into main Jan 8, 2021
@ansd ansd deleted the context-logger branch January 8, 2021 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants