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.

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

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

Open
danwinship opened this issue Apr 26, 2024 · 2 comments
Assignees
Labels
kind/documentation Categorizes issue or PR as related to documentation. triage/accepted Indicates an issue or PR is ready to be actively worked on.

Comments

@danwinship
Copy link

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

@k8s-ci-robot k8s-ci-robot added kind/documentation Categorizes issue or PR as related to documentation. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Apr 26, 2024
@pohly
Copy link

pohly commented Apr 29, 2024

/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.

@k8s-ci-robot 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 Apr 29, 2024
@pohly
Copy link

pohly commented Apr 29, 2024

/assign

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/documentation Categorizes issue or PR as related to documentation. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

No branches or pull requests

3 participants