add klog.v(0) and logger.v(0) check #4

yangjunmyfm192085 · 2022-08-27T16:41:48Z

as mentioned in #2
Calling V with zero as parameter just causes additional overhead.
we want a way to enforce that this code pattern doesn't get added back in some future PR

yangjunmyfm192085 · 2022-08-27T16:44:19Z

/cc @pohly

pohly

This already looks pretty good. Some comments...

logcheck/pkg/logcheck.go

yangjunmyfm192085 · 2022-08-30T03:16:27Z

@pohly Thank you for the very detailed review, some scenarios were not considered at first, and now it has been updated.
could you please take a look again? thanks

pohly · 2022-08-30T06:12:20Z

logcheck/pkg/logcheck.go

+					Pos:     fexpr.Fun.Pos(),
+					Message: msg,
+				})
+			}


Can you move the pass.Report down into checkForVerboseZero to avoid the code duplication? If we ever decide to make it better (for example, by suggesting a code update), then we only need to make that change in one place.

checkForVerboseZero can call a isVerboseZero that is equivalent to the current function.

Yeah, this part is duplicated.
I have optimized it.

pohly · 2022-08-30T07:50:21Z

logcheck/pkg/logcheck.go

@@ -36,6 +36,7 @@ const (
 	parametersCheck  = "parameters"
 	contextualCheck  = "contextual"
 	withHelpersCheck = "with-helpers"
+	verboseZeroCheck = "verbose-zero"


One last thing: should this be "verbosity" instead of "verbose"?

I think V stands for "verbosity" of the log message, but I might be wrong (not a native speaker).

A log message is "verbose", but it has "verbosity zero".

yangjunmyfm192085 · 2022-08-30T08:38:42Z

@pohly I think you are right.
I checked the explanation of V in the code.

  V reports whether verbosity at the call site is at least the requested level.

So V should be verbosity here, I have updated. thanks.

pohly

/lgtm
/approve

Thanks a lot!

k8s-ci-robot · 2022-08-30T11:19:26Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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 · 2022-08-30T11:34:42Z

logcheck/pkg/logcheck.go

+		return true
+	}
+
+	if id, ok := subCallExpr.Args[0].(*ast.Ident); ok && id.Obj.Kind == 2 {


I'm not sure what is nil, but when I tried this change on top of your kubernetes/kubernetes#111708 I got a nil pointer access:

ERRO [runner] Panic: logcheck: package "vclib" (isInitialPkg: true, needAnalyzeSource: true): runtime error: invalid memory address or nil pointer dereference: goroutine 18650 [running]: runtime/debug.Stack() /nvme/gopath/go-1.19/src/runtime/debug/stack.go:24 +0x65 github.com/golangci/golangci-lint/pkg/golinters/goanalysis.(*action).analyzeSafe.func1() /nvme/gopath/pkg/mod/github.com/golangci/golangci-lint@v1.48.0/pkg/golinters/goanalysis/runner_action.go:101 +0x155 panic({0x153a9e0, 0x2024fd0}) /nvme/gopath/go-1.19/src/runtime/panic.go:884 +0x212 sigs.k8s.io/logtools/logcheck/pkg.isVerbosityZero({0x18ff6c0?, 0xc01b9c4000?}) /nvme/gopath/pkg/mod/sigs.k8s.io/logtools@v0.2.1-0.20220830112100-f68289b649a5/logcheck/pkg/logcheck.go:544 +0xb5

To reproduce, check out your Kubernetes branch and patch it:

diff --git a/hack/tools/go.mod b/hack/tools/go.mod index 8728f260aa4..ffcc1ab1519 100644 --- a/hack/tools/go.mod +++ b/hack/tools/go.mod @@ -11,7 +11,7 @@ require ( github.com/google/go-flow-levee v0.1.5 gotest.tools/gotestsum v1.6.4 honnef.co/go/tools v0.3.3 - sigs.k8s.io/logtools v0.1.0 + sigs.k8s.io/logtools v0.2.1-0.20220830112100-f68289b649a5 sigs.k8s.io/zeitgeist v0.2.0 ) diff --git a/hack/tools/go.sum b/hack/tools/go.sum index 0dc6f65d798..c39cafc35cd 100644 --- a/hack/tools/go.sum +++ b/hack/tools/go.sum @@ -829,6 +829,7 @@ github.com/yuin/goldmark v1.1.32/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9de github.com/yuin/goldmark v1.2.1/go.mod h1:3hX8gzYuyVAZsxl0MRgGTJEmQBFcNTphYh9decYSb74= github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= github.com/yuin/goldmark v1.4.1/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1Zlc8k= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= gitlab.com/bosi/decorder v0.2.3 h1:gX4/RgK16ijY8V+BRQHAySfQAb354T7/xQpDB2n10P0= gitlab.com/bosi/decorder v0.2.3/go.mod h1:9K1RB5+VPNQYtXtTDAzd2OEftsZb1oV0IrJrzChSdGE= go.etcd.io/bbolt v1.3.2/go.mod h1:IbVyRI1SCnLcuJnV2u8VeU0CEYM7e686BmAb1XKL+uU= @@ -1077,6 +1078,7 @@ golang.org/x/sys v0.0.0-20220114195835-da31bd327af9/go.mod h1:oPkhp1MJrh7nUepCBc golang.org/x/sys v0.0.0-20220319134239-a9b59b0215f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220328115105-d36c6a25d886/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220412211240-33da011f77ad/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220702020025-31831981b65f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220715151400-c0bba94af5f8/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f h1:v4INt8xihDGvnrfjMDVXGxw9wrfxYyCjk0KbXjhR55s= @@ -1368,6 +1370,10 @@ rsc.io/quote/v3 v3.1.0/go.mod h1:yEA65RcK8LyAZtP9Kv3t0HmxON59tX3rD+tICJqUlj0= rsc.io/sampler v1.3.0/go.mod h1:T1hPZKmBbMNahiBKFy5HrXp6adAjACjK9JXDnKaTXpA= sigs.k8s.io/logtools v0.1.0 h1:BWdoZu2PK6/lj5QhSXBGlHFu3d/ST1HlINXwE/CkJRQ= sigs.k8s.io/logtools v0.1.0/go.mod h1:/Rp/yzQWyUgsNCRb1HKRnrVujUV9CmzksOlsQR2OVvw= +sigs.k8s.io/logtools v0.2.0 h1:FMy0Td6HnWP+Iqq9QWEZigS130aZSMtlhRn5pcZbwCo= +sigs.k8s.io/logtools v0.2.0/go.mod h1:2HGcCK1vi9YvsBoUDMXvrf588j890iRtsymJX2biE0Q= +sigs.k8s.io/logtools v0.2.1-0.20220830112100-f68289b649a5 h1:dy8gOoe/fHFqY1vN16XTgl/S8TIpuyxkE0wCTUr1UV4= +sigs.k8s.io/logtools v0.2.1-0.20220830112100-f68289b649a5/go.mod h1:2HGcCK1vi9YvsBoUDMXvrf588j890iRtsymJX2biE0Q= sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= sigs.k8s.io/yaml v1.2.0 h1:kr/MCeFWJWTwyaHoR9c8EjH9OumOmoF9YGiZd7lFm/Q= sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc=

Then run hack/verify-golangci-lint.sh. That also shows that there are several more code lines which need to be modified.

If you want to try out a modified logcheck then you can point towards a local copy with:

(cd hack/tools && go mod edit -replace sigs.k8s.io/logtools=<absolute path>/logtools/)

No code survives first contact with the real world 😁

Let me analyze why

Hi, @pohly , This is an interesting issue, but I don't reproduce it.
I don't know what scenario will appear that obj is nil.
In the pr below I have added protection, do we need to continue the analysis or could we merge this pr?
#5

It's indeed a bit odd. I can only reproduce it by running hack/verify-golangci-lint.sh, but not when invoking logcheck directly.

And now I cannot reproduce it anymore. Perhaps it was a problem with the golangci-lint cache.

Hi, @pohly ,I think I caught the issue.
When Constants of value is defined in different files, there will be cases where it cannot be parsed directly
such as
https://github.com/kubernetes/kubernetes/blob/2b2be7fa6b69c7591a31475fc4fb1dcff362a128/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/datastore.go#L62

https://github.com/kubernetes/kubernetes/blob/2b2be7fa6b69c7591a31475fc4fb1dcff362a128/staging/src/k8s.io/legacy-cloud-providers/vsphere/vclib/constants.go#L43

Do we need to continue dealing with this scenario, is it a bit complicated?
Or do we just need to fix panic?

I think it is sufficient to avoid the panic. Please reopen #5 and add an explanation in the code why the additional if conditions are needed, then I can merge it.

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 27, 2022

k8s-ci-robot requested review from serathius and thockin August 27, 2022 16:41

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 27, 2022

yangjunmyfm192085 force-pushed the addverbose-zerochekc branch from 4fb10d3 to 038c0b9 Compare August 27, 2022 16:43

k8s-ci-robot requested a review from pohly August 27, 2022 16:44

yangjunmyfm192085 mentioned this pull request Aug 28, 2022

use klog.InfoS instead of klog.V(0).InfoS kubernetes/kubernetes#111708

Merged

pohly requested changes Aug 29, 2022

View reviewed changes

yangjunmyfm192085 force-pushed the addverbose-zerochekc branch 2 times, most recently from 4befb0d to 33ca26b Compare August 30, 2022 03:14

yangjunmyfm192085 force-pushed the addverbose-zerochekc branch from 33ca26b to f7558df Compare August 30, 2022 05:05

pohly reviewed Aug 30, 2022

View reviewed changes

yangjunmyfm192085 force-pushed the addverbose-zerochekc branch from f7558df to 8b816a4 Compare August 30, 2022 06:43

pohly reviewed Aug 30, 2022

View reviewed changes

yangjunmyfm192085 force-pushed the addverbose-zerochekc branch 2 times, most recently from 937b8ad to 9bb3130 Compare August 30, 2022 08:33

add klog.v(0) and logger.v(0) check

7fe4221

yangjunmyfm192085 force-pushed the addverbose-zerochekc branch from 9bb3130 to 7fe4221 Compare August 30, 2022 08:35

pohly approved these changes Aug 30, 2022

View reviewed changes

k8s-ci-robot assigned pohly Aug 30, 2022

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 Aug 30, 2022

k8s-ci-robot merged commit f68289b into kubernetes-sigs:main Aug 30, 2022

pohly reviewed Aug 30, 2022

View reviewed changes

yangjunmyfm192085 mentioned this pull request Aug 31, 2022

fix VerbosityZero check panic #5

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add klog.v(0) and logger.v(0) check #4

add klog.v(0) and logger.v(0) check #4

yangjunmyfm192085 commented Aug 27, 2022

yangjunmyfm192085 commented Aug 27, 2022

pohly left a comment

yangjunmyfm192085 commented Aug 30, 2022

pohly Aug 30, 2022

yangjunmyfm192085 Aug 30, 2022 •

edited

pohly Aug 30, 2022

yangjunmyfm192085 commented Aug 30, 2022 •

edited

pohly left a comment

k8s-ci-robot commented Aug 30, 2022

pohly Aug 30, 2022

pohly Aug 30, 2022

yangjunmyfm192085 Aug 30, 2022

yangjunmyfm192085 Aug 31, 2022

pohly Sep 2, 2022

pohly Sep 2, 2022

yangjunmyfm192085 Sep 9, 2022

pohly Sep 9, 2022

add klog.v(0) and logger.v(0) check #4

add klog.v(0) and logger.v(0) check #4

Conversation

yangjunmyfm192085 commented Aug 27, 2022

yangjunmyfm192085 commented Aug 27, 2022

pohly left a comment

Choose a reason for hiding this comment

yangjunmyfm192085 commented Aug 30, 2022

Choose a reason for hiding this comment

yangjunmyfm192085 Aug 30, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yangjunmyfm192085 commented Aug 30, 2022 • edited

pohly left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Aug 30, 2022

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

yangjunmyfm192085 Aug 30, 2022 •

edited

yangjunmyfm192085 commented Aug 30, 2022 •

edited