klog icon indicating copy to clipboard operation
klog copied to clipboard

comment docs are out of date, recommend code style that kubernetes-sigs/logtools warns about

Open danwinship opened this issue 9 months ago • 2 comments

I tried doing

if klog.V(9) {
	klog.InfoS("Running nftables transaction", "transaction", tx.String())
}

which I know used to work, but apparently now it doesn't, so I checked the klog sources, and klog.go says:

// See the documentation for the V function for an explanation of these examples:
//
//	if klog.V(2) {
//		klog.Info("Starting transaction...")
//	}

Ignoring the comment, I dug further and found the Enabled() function and then it compiled, but it turns out that golangci-lint still doesn't like it:

ERROR: pkg/proxy/nftables/proxier.go:1617:2: the result of klog.V should be stored in a variable and then be used multiple times: if klogV := klog.V(); klogV.Enabled() { ... klogV.Info ... } (logcheck)
ERROR: 	if klog.V(9).Enabled() {
ERROR: 	^ 

(which, FTR, is a terrible API though I don't have any ideas on fixing it)

The whole introductory doc comment in klog.go needs a rewrite/update, which maybe ought to be copied to the README?

/kind documentation

danwinship avatar Apr 26 '24 13:04 danwinship

/triage accepted

The documentation is indeed considerably out-dated. Worse, in Kubernetes we have mostly moved away from using the klog APIs, so anyone coming to klog with the expectation that this is where "how to do logging in Kubernetes" is documented will be disappointed.

I'll probably need to write a "Kuberneter logging best practices" section and then leave the rest of the documentation in place for those projects which still use "traditional" klog.

pohly avatar Apr 29 '24 06:04 pohly

/assign

pohly avatar Apr 29 '24 06:04 pohly