logcheck: support banning non-contextual functions and methods #25

pohly · 2023-11-23T13:58:52Z

Sometimes there are APIs with different variants where one variant is fine in code which doesn't support contextual logging but should be replaced with some other variant if that is the goal.

For example, k8s.io/apimachinery/pkg/util/runtime.HandleError is the old-style function. We cannot deprecate it because it works fine in most existing code. But the upcoming HandleErrorWithContext is what code in Kubernetes that has (or will be) converted to contextual logging should use.

We also cannot forbid it through forbidigo, because then the logcheck.conf with its definition of which code is "contextual" would be ignored. Besides, maintaining a config for forbidigo would be continual effort that would need to be replicated by other projects consuming the package with these APIs.

A better solution is to:

mark these non-contextual APIs through a //logcheck:context // <why> comment
detect those comments in logcheck
warn about using them in contextual code

k8s-ci-robot · 2023-11-23T13:58:58Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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:

~~OWNERS~~ [pohly]

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

pohly · 2023-11-23T14:28:05Z

It also works in Kubernetes:

diff --git a/staging/src/k8s.io/client-go/tools/cache/shared_informer.go b/staging/src/k8s.io/client-go/tools/cache/shared_informer.go
index 9851b197240..36af7ca07f0 100644
--- a/staging/src/k8s.io/client-go/tools/cache/shared_informer.go
+++ b/staging/src/k8s.io/client-go/tools/cache/shared_informer.go
@@ -170,7 +170,7 @@ type SharedInformer interface {
        // Run starts and runs the shared informer, returning after it stops.
        // The informer will be stopped when stopCh is closed.
        //
-       // RunWithContext should be used instead in k8s.io/kubernetes source code.
+       //logcheck:context // RunWithContext should be used instead in code which uses contextual logging.
        Run(stopCh <-chan struct{})
        // RunWithContext starts and runs the shared informer, returning after it stops.
        // The informer will be stopped when the context is canceled.

$ logcheck -config hack/logcheck.conf ./pkg/controller/...
...
/nvme/gopath/src/k8s.io/kubernetes/pkg/controller/job/job_controller_test.go:4642:57: RunWithContext should be used instead in code which uses contextual logging.
/nvme/gopath/src/k8s.io/kubernetes/pkg/controller/job/job_controller_test.go:4668:17: RunWithContext should be used instead in code which uses contextual logging.
/nvme/gopath/src/k8s.io/kubernetes/pkg/controller/job/job_controller_test.go:5195:51: RunWithContext should be used instead in code which uses contextual logging.
/nvme/gopath/src/k8s.io/kubernetes/pkg/controller/replicaset/replica_set_test.go:712:45: RunWithContext should be used instead in code which uses contextual logging.

fatsheep9146 · 2023-11-23T23:12:29Z

logcheck/testdata/src/k8s.io/apimachinery/pkg/util/runtime/runtime.go

+}
+
+type Handler interface {
+	//logcheck:context//Use HandleErrorWithContext instead in code which supports contextual logging.


small nits
I think
//logcheck:context //Use HandleErrorWithContext instead in code which supports contextual logging.
may be better which is same as described in README.md

This is intentionally checking that the code handles also the case that there is no space before the //.

fatsheep9146 · 2023-11-24T07:13:09Z

logcheck/README.md

@@ -56,6 +56,14 @@ through that.
 klog calls that are needed to manage contextual logging, for example
 `klog.Background`, are still allowed.

+Functions or methods that have a comment of the form
+
+   //logcheck:context // Foo should not be used in code which supports contextual logging.


Just curious, I think this default explanation wants to express the Foo function does not support contextual logging, so the developer should not use this function any more, they should use the new version function which supports contextual logging, am I understanding this correctly?

Correct. The default doesn't know what the replacement is, so it can't mention it. I think in practice one should always add such a comment.

Yes, I think you're right.
This Foo should not be used in code which supports contextual logging expression makes me a little confused about whether Foo supports contextual logging or not, when I first see it. Now it's clear.

mengjiao-liu · 2023-11-24T03:03:56Z

logcheck/README.md

+   //logcheck:context // Foo should not be used in code which supports contextual logging.
+   func Foo() { ... }


This code example is best selected using the Code block, otherwise the format is messy

Right. I didn't indent enough. Will fix.

mengjiao-liu · 2023-11-24T03:10:48Z

logcheck/README.md

+   //logcheck:context // Foo should not be used in code which supports contextual logging.
+   func Foo() { ... }
+
+are also not allowed. The additional explanation is optional. The default is


I'm a little confused. Functions or methods that have a comment of the form should be allowed from the PR context. Why is the description here not allowed?

"not allowed" in code where checking for contextual logging is enabled. The description (the additional explanation after logcheck:context) is allowed.

Does this clarify it?

I understand, but it is best to add clarification above to avoid user confusion.

I already updated the README.md with my comment. If it needs further wording changes, can you suggest something?

Just my personal suggestion：

When contextual logging is enabled, a function or method has no the //logcheck:context // <why> comment will not be allowed by log check verification. Conversely, a function or method that has the //logcheck:context // <why> comment will be allowed by log check verification, and the additional explanation you have defined (// <why>) will be output as a warning message. The additional explanation is optional. The default is the example text below.

Here is an example:

//logcheck:context // Foo should not be used in code which supports contextual logging. func Foo() { ... }

It's actually the other way around: having that comment indicates that the function is not allowed.

I've rewritten the section. I hope it is clearer now?

Regarding "not allowed": the comment also could be logcheck:nocontext with the same semantic. Not sure whether that would be better.

Nice job! It does seem clearer after the rewrite.

LGTM it and we can do a release? 😄

pohly · 2023-11-27T12:31:04Z

/hold

When I try this on the entire Kubernetes code base, I get warnings about unstructured log calls in packages where those should be allowed. Need to investigate...

pohly · 2023-11-27T12:39:49Z

/unhold

False alarm. kubernetes/kubernetes#121757 needs to be merged first, then logcheck can be updated.

dgrisonnet · 2023-11-30T18:00:43Z

/triage accepted

Sometimes there are APIs with different variants where one variant is fine in code which doesn't support contextual logging but should be replaced with some other variant if that is the goal. For example, k8s.io/apimachinery/pkg/util/runtime.HandleError is the old-style function. We cannot deprecate it because it works fine in most existing code. But the upcoming HandleErrorWithContext is what code in Kubernetes that has (or will be) converted to contextual logging should use. We also cannot forbid it through forbidigo, because then the logcheck.conf with its definition of which code is "contextual" would be ignored. Besides, maintaining a config for forbidigo would be continual effort that would need to be replicated by other projects consuming the package with these APIs. A better solution is to: - mark these non-contextual APIs through a `//logcheck:context // <why>` comment - detect those comments in logcheck - warn about using them in contextual code

strings.CutPrefix is useful and only available in >= 1.20. golangci-lint then complains about the out-dated ioutil.

mengjiao-liu · 2023-12-06T10:06:22Z

/lgtm

k8s-ci-robot requested a review from serathius November 23, 2023 13:58

k8s-ci-robot requested a review from thockin November 23, 2023 13:58

pohly force-pushed the logcheck-context-comment branch from b51cd08 to a4e5ad0 Compare November 23, 2023 14:00

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2023

pohly mentioned this pull request Nov 23, 2023

apimachinery runtime: support contextual logging kubernetes/kubernetes#121970

Merged

pohly force-pushed the logcheck-context-comment branch 2 times, most recently from 8fccd67 to 8a7f645 Compare November 23, 2023 15:09

fatsheep9146 reviewed Nov 23, 2023

View reviewed changes

fatsheep9146 reviewed Nov 24, 2023

View reviewed changes

mengjiao-liu reviewed Nov 24, 2023

View reviewed changes

pohly force-pushed the logcheck-context-comment branch from 8a7f645 to c704227 Compare November 24, 2023 10:51

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2023

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 27, 2023

k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Nov 30, 2023

pohly added 2 commits December 5, 2023 10:29

build: bump minimum Go to 1.20

0eec4af

strings.CutPrefix is useful and only available in >= 1.20. golangci-lint then complains about the out-dated ioutil.

pohly force-pushed the logcheck-context-comment branch from c704227 to 0eec4af Compare December 5, 2023 09:29

k8s-ci-robot assigned mengjiao-liu Dec 6, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Dec 6, 2023

k8s-ci-robot merged commit a41e815 into kubernetes-sigs:main Dec 6, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

logcheck: support banning non-contextual functions and methods #25

logcheck: support banning non-contextual functions and methods #25

pohly commented Nov 23, 2023

k8s-ci-robot commented Nov 23, 2023

pohly commented Nov 23, 2023

fatsheep9146 Nov 23, 2023

pohly Nov 24, 2023

fatsheep9146 Nov 24, 2023

pohly Nov 24, 2023

fatsheep9146 Nov 24, 2023

mengjiao-liu Nov 24, 2023

pohly Nov 24, 2023

mengjiao-liu Nov 24, 2023

pohly Nov 24, 2023

mengjiao-liu Nov 28, 2023

pohly Nov 28, 2023

mengjiao-liu Dec 4, 2023

pohly Dec 5, 2023

pohly Dec 5, 2023

mengjiao-liu Dec 6, 2023

pohly Dec 6, 2023

pohly commented Nov 27, 2023

pohly commented Nov 27, 2023

dgrisonnet commented Nov 30, 2023

mengjiao-liu commented Dec 6, 2023

		//logcheck:context // Foo should not be used in code which supports contextual logging.
		func Foo() { ... }

logcheck: support banning non-contextual functions and methods #25

logcheck: support banning non-contextual functions and methods #25

Conversation

pohly commented Nov 23, 2023

k8s-ci-robot commented Nov 23, 2023

pohly commented Nov 23, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pohly commented Nov 27, 2023

pohly commented Nov 27, 2023

dgrisonnet commented Nov 30, 2023

mengjiao-liu commented Dec 6, 2023