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

🌱 Add controller-name as kv pair to logger instead of as name #1806

Merged
merged 1 commit into from
Feb 15, 2022
Merged

🌱 Add controller-name as kv pair to logger instead of as name #1806

merged 1 commit into from
Feb 15, 2022

Conversation

sbueringer
Copy link
Member

Signed-off-by: Stefan Büringer buringerst@vmware.com

Addresses #1794. More context in the initial post of #1794

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Feb 15, 2022
@sbueringer
Copy link
Member Author

/cc @alvaroaleman @vincepri

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Feb 15, 2022
alvaroaleman
alvaroaleman approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman 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 the idea but it is a breaking change as existing log queries will stop working. Are we comfortable with that?
/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 15, 2022
@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Feb 15, 2022
sbueringer
sbueringer commented Feb 15, 2022
View reviewed changes
pkg/controller/controller.go Outdated
@@ -135,7 +135,7 @@ func NewUnmanaged(name string, mgr manager.Manager, options Options) (Controller
CacheSyncTimeout: options.CacheSyncTimeout,
SetFields: mgr.SetFields,
Name: name,
Log: options.Log.WithName("controller").WithName(name),
Log: options.Log.WithValues("controller", name),
Copy link
Member Author

@sbueringer sbueringer Feb 15, 2022 •
edited

Choose a reason for hiding this comment

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

@alvaroaleman
Maybe we can/should set the name and k/v?

I just saw this user story in the kubernetes contextual logging KEP:

When kube-scheduler starts processing a pod, it creates a new logger with logger.WithValue("pod", klogr.KObj(pod)) and passes that around. While iterating over plugins in certain operations, another logger gets created with logger.WithName().WithName() and then is used when invoking that plugin. This adds a prefix to each log message which represents the call path to the specific log message, like for example NominatedPods/Filter/VolumeBinding.
https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging#story-1

To me it seemed strange to have a msg prefix with the controller. But looking at that user story, I can see why folks might want that.

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 and seems a good idea

@sbueringer
Add controller-name as kv pair instead of as name
b4d1c56 
Signed-off-by: Stefan Büringer buringerst@vmware.com
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 15, 2022
alvaroaleman
alvaroaleman approved these changes Feb 15, 2022
View reviewed changes
Copy link
Member

@alvaroaleman alvaroaleman left a comment

Choose a reason for hiding this comment

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

thanks!
/hold cancel

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Feb 15, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alvaroaleman, sbueringer

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot merged commit 5b2eaae into kubernetes-sigs:master Feb 15, 2022
@k8s-ci-robot k8s-ci-robot added this to the v0.10.x milestone Feb 15, 2022
@sbueringer sbueringer deleted the pr-controller-name-kv branch February 16, 2022 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvaroaleman alvaroaleman alvaroaleman approved these changes

@vincepri vincepri Awaiting requested review from vincepri

@alexeldeib alexeldeib Awaiting requested review from alexeldeib

@FillZpp FillZpp Awaiting requested review from FillZpp

Assignees

@alvaroaleman alvaroaleman

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/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Milestone
v0.10.x
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sbueringer @k8s-ci-robot @alvaroaleman