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

structured-logging: enable KobjSlice usage and warn Kobjs usage #1

Merged
merged 1 commit into from
Sep 22, 2022
Merged

structured-logging: enable KobjSlice usage and warn Kobjs usage #1

merged 1 commit into from
Sep 22, 2022

Conversation

harshanarayana
Copy link
Contributor

@harshanarayana harshanarayana commented Jul 1, 2022 •
edited
Loading

This pull request enables detection and warning on now deprecated KObjs. It is suggested to use the KObjSlice instead of using Kobjs for better logging performance Since kubernetes/kubernetes#110724 is now merged, We can reap the benefits of the change that went into the klog via kubernetes/klog#322

Fixes Part of kubernetes/kubernetes#110737

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 1, 2022
@k8s-ci-robot
Copy link
Contributor

Welcome @harshanarayana!

It looks like this is your first PR to kubernetes-sigs/logtools 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/logtools has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 1, 2022
pohly
pohly requested changes Aug 5, 2022
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

I think we need also a test case where KObjs is flagged as a function that shouldn't be used.

@harshanarayana
Copy link
Contributor Author

I think we need also a test case where KObjs is flagged as a function that shouldn't be used.

@pohly Sorry, I have been away for a few days. I will take a look at this and get the update published this week

@harshanarayana harshanarayana force-pushed the cleanup/GIT-110737/logging-improvements branch from 10e7f93 to 95c3097 Compare August 31, 2022 16:57
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 31, 2022
@harshanarayana
Copy link
Contributor Author

@pohly PTAL. I've added the required test as suggested

@harshanarayana harshanarayana requested review from pohly and removed request for thockin August 31, 2022 16:58
@harshanarayana
Copy link
Contributor Author

/cc @thockin

@harshanarayana harshanarayana force-pushed the cleanup/GIT-110737/logging-improvements branch from 95c3097 to 76fb3f2 Compare August 31, 2022 17:00
pohly
pohly reviewed Sep 2, 2022
View reviewed changes
logcheck/testdata/src/onlyAllowContextual/onlyAllowContextual.go Outdated
@@ -29,4 +29,5 @@ func doNotAlllowKlog() {
klog.InfoS("test log") // want `function "InfoS" should not be used, convert to contextual logging`
klog.ErrorS(nil, "test log") // want `function "ErrorS" should not be used, convert to contextual logging`
klog.V(1).Infof("test log") // want `function "V" should not be used, convert to contextual logging` `function "Infof" should not be used, convert to contextual logging`
klog.KObjs(nil) // want `function "KObjs" should not be used, convert to contextual logging`
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we want a better warning here. "convert to contextual logging" is misleading when printed for a line which uses contextual logging.

This implies that we need separate code for it and cannot simply add it to the blacklisted functions.

Copy link
Contributor

Choose a reason for hiding this comment

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

At that point it probably needs a separate flag for enabling the check.

I'm leaning towards enabling it by default, even if that means that we need to update code in Kubernetes before we can switch to the newer logtools, or disable the check initially in Kubernetes' logcheck.conf.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am fine with it. Do you have any preferences on how you would like this to look like @pohly ?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, not really.

This was referenced Sep 2, 2022
Closed
Closed
@harshanarayana harshanarayana force-pushed the cleanup/GIT-110737/logging-improvements branch from 76fb3f2 to 3a1631d Compare September 19, 2022 14:03
@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 Sep 19, 2022
@harshanarayana
Copy link
Contributor Author

@pohly PTAL when time Permits. I've added a few flag for the deprecation check and is enabled by default right now.

@harshanarayana harshanarayana force-pushed the cleanup/GIT-110737/logging-improvements branch from 3a1631d to 6e0d37e Compare September 19, 2022 14:05
@harshanarayana harshanarayana force-pushed the cleanup/GIT-110737/logging-improvements branch from 6e0d37e to 32f29ca Compare September 21, 2022 16:32
pohly
pohly requested changes Sep 21, 2022
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

The code looks good now, but I remembered that https://github.com/kubernetes-sigs/logtools/blob/main/logcheck/README.md has documentation for the available checks - can you add the new one there?

@harshanarayana harshanarayana force-pushed the cleanup/GIT-110737/logging-improvements branch from 32f29ca to f2a2dfa Compare September 21, 2022 17:45
@harshanarayana
Copy link
Contributor Author

The code looks good now, but I remembered that https://github.com/kubernetes-sigs/logtools/blob/main/logcheck/README.md has documentation for the available checks - can you add the new one there?

@pohly Updated the README. It was also missing the verbosity-zero check. Included that as well

@pohly
Copy link
Contributor

pohly commented Sep 22, 2022

Keeping documentation up-to-date... always easy to forget. Everyone should just read the source code! 😅

Thanks for updating everything.

pohly
pohly approved these changes Sep 22, 2022
View reviewed changes
Copy link
Contributor

@pohly pohly left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: harshanarayana, pohly

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 added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 22, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6f17d45 into kubernetes-sigs:main Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pohly pohly pohly approved these changes

@thockin thockin Awaiting requested review from thockin

Assignees

@pohly pohly

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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@harshanarayana @k8s-ci-robot @pohly