autoscaler icon indicating copy to clipboard operation
autoscaler copied to clipboard

Published 20 hours ago • verified publisher icon kubernetes

Refactor logging in recommender, updater, and target components

Open omerap12 opened this issue 1 year ago • 13 comments

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

To align the logging with the best practices and guidelines established within the Kubernetes ecosystem.

Which issue(s) this PR fixes:

As part of https://github.com/kubernetes/autoscaler/issues/6905, I'm splitting this into multiple PRs rather than submitting a single large PR.

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

None

omerap12 avatar Sep 18 '24 12:09 omerap12

Hi @omerap12. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot avatar Sep 18 '24 12:09 k8s-ci-robot

/ok-to-test

adrianmoisey avatar Sep 18 '24 16:09 adrianmoisey

/lgtm

adrianmoisey avatar Sep 19 '24 05:09 adrianmoisey

Hey @jbartosik, can you approve this? /assign jbartosik

adrianmoisey avatar Sep 24 '24 08:09 adrianmoisey

Cool to see structured logging is an option now!

kwiesmueller avatar Sep 24 '24 16:09 kwiesmueller

/lgtm

adrianmoisey avatar Sep 26 '24 08:09 adrianmoisey

/unassign @jbartosik /assign

voelzmo avatar Oct 18 '24 08:10 voelzmo

Thanks for going through all the files! A few small patches to add more context.

Also in this change: there's a few occasions of klog.Warningf, could you also take care of those, please? Thanks!

Thanks for your review! Regarding klog.Warningf, there is no klog.WarningS so I'm converting all klog.Warningf to klog.V(0).InfoS is that ok? (please see here)

omerap12 avatar Oct 18 '24 12:10 omerap12

Thanks for going through all the files! A few small patches to add more context. Also in this change: there's a few occasions of klog.Warningf, could you also take care of those, please? Thanks!

Thanks for your review! Regarding klog.Warningf, there is no klog.WarningS so I'm converting all klog.Warningf to klog.V(0).InfoS is that ok? (please see here)

That sounds good to me. Thanks for the pointer to the original discussion!

voelzmo avatar Oct 22 '24 07:10 voelzmo

Thanks for going through all the files! A few small patches to add more context. Also in this change: there's a few occasions of klog.Warningf, could you also take care of those, please? Thanks!

Thanks for your review! Regarding klog.Warningf, there is no klog.WarningS so I'm converting all klog.Warningf to klog.V(0).InfoS is that ok? (please see here)

That sounds good to me. Thanks for the pointer to the original discussion!

No problem :) Updated.

omerap12 avatar Oct 22 '24 07:10 omerap12

/lgtm /approve

Thanks!

voelzmo avatar Oct 22 '24 07:10 voelzmo

/lgtm /approve

voelzmo avatar Oct 24 '24 11:10 voelzmo

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: omerap12, voelzmo

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 avatar Oct 24 '24 11:10 k8s-ci-robot