Add check for InfoS & ErrorS parameters #229

mowangdk · 2021-03-17T04:15:15Z

What this PR does / why we need it:
Add check for valid keys and number of args for ErrorS/InfoS

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #219

Special notes for your reviewer:

None
Release note:

None

adisky · 2021-03-19T07:53:59Z

hack/tools/logcheck/main.go

+	if len(keyValues)%2 != 0 {
+		pass.Report(analysis.Diagnostic{
+			Pos:     fun.Pos(),
+			Message: fmt.Sprintf("Invalid Number of arguments for %s", funName),


I think we should not continue from here, no. of args are invalid because something is missed(key/value) in the log message,

e.g.

klog.V(2).InfoS("Starting container in a pod", "containerID", containerID, pod) Invalid Number of arguments for InfoS

here key is missed, it has value at that position. So no meaning of key type check, it will always fail.

I agree, if number of arguments doesn't match, we cannot be sure about what is missing. It could be value at the end of arguments, but also key in the middle of them.
This makes analysis that assumes that keys are positioned in certain way hard to understand. In example provided by @adisky we would say tath pod should be a string, which doesn't make sense.

get, will fixed on next push, Thanks for advice~

adisky · 2021-03-19T07:56:43Z

hack/tools/logcheck/main.go

+			if !isASCII {
+				pass.Report(analysis.Diagnostic{
+					Pos:     fun.Pos(),
+					Message: fmt.Sprintf("Invalid value for key %v, it's must be ascii", lit.Value),


Suggested change

Message: fmt.Sprintf("Invalid value for key %v, it's must be ascii", lit.Value),

Message: fmt.Sprintf("Invalid value for key %v, it must be ascii", lit.Value),

Same comment about the message. Let's give some background here.

Key positional arguments are expected to be lowerCamelCase alphanumeric strings. Please remove any non-Latin characters.

adisky · 2021-03-19T07:57:18Z

hack/tools/logcheck/main.go

+		} else {
+			pass.Report(analysis.Diagnostic{
+				Pos:     fun.Pos(),
+				Message: "Invalid value type for key",


can we print the key here?

It will be a bit complicated, because it has to check arg type

switch arg.(type) { case *ast.FuncLit: .... case *ast.CompositeLit: .... }

if you are ok with it, i will fix it.

adisky · 2021-03-19T07:58:28Z

hack/tools/logcheck/testdata/data.go

+	klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod")       // want `Invalid Number of arguments for InfoS`
+	klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Invalid Number of arguments for ErrorS`
+	klog.InfoS("Starting container in a pod", "测试", "containerID")                       // want `Invalid value for key`
+	klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID")                 // want `Invalid value for key`


how is the test passing here? here error message should be Invalid value for key, It must be ascii

Seems like it match by prefix~

adisky · 2021-03-19T08:05:05Z

hack/tools/logcheck/main.go

@@ -80,6 +82,13 @@ func checkForFunctionExpr(fun ast.Expr, pass *analysis.Pass) {
 		// extracting package name
 		pName, ok := selExpr.X.(*ast.Ident)

+		if ok && pName.Name == "klog" && !isUnstructured((fName)) {


Suggesting little reshuffling here.

if ok && pName.Name == "klog" { if isUnstructured((fName)) { ----- ---- } else { isKeysValid() } }

adisky · 2021-03-19T08:06:45Z

@mowangdk Thanks for working on this :) !!!. Tried it locally it is working fine
Some suggestions inline

serathius · 2021-03-19T11:57:53Z

hack/tools/logcheck/main.go

+	if len(keyValues)%2 != 0 {
+		pass.Report(analysis.Diagnostic{
+			Pos:     fun.Pos(),
+			Message: fmt.Sprintf("Invalid Number of arguments for %s", funName),


This static analysis will be used for static analysis of every K/K PR, which means that almost every contributor will at some point need to fix errors from it. This puts requirement that descriptions for errors should be clear enough that someone without any knowledge of Structured Logging should be able to understand it.

Instead of message nvalid Number of arguments for %s, we should to with something more actionable:
For example:

Uneven number of positional arguments. Additional arguments to InfoS should always be Key Value pairs. Please check if there is any key or value missing.

serathius · 2021-03-19T12:00:50Z

hack/tools/logcheck/main.go

+			if lit.Kind != token.STRING {
+				pass.Report(analysis.Diagnostic{
+					Pos:     fun.Pos(),
+					Message: fmt.Sprintf("Invalid value type for key %v", lit.Value),


Same commend about message as above. Could we go with something like:

Key positional arguments are expected to be inlined constant strings. Please replace variable provided with it's value.

serathius · 2021-03-19T12:10:38Z

hack/tools/logcheck/main.go

+					Message: fmt.Sprintf("Invalid value for key %v, it's must be ascii", lit.Value),
+				})
+			}
+		} else {


Let's move error handling block above and remove the else.

Can you reverse logic check on line 136

if lit, ok := arg.(*ast.BasicLit); ok {

to

if lit, ok := arg.(*ast.BasicLit); !ok {

serathius · 2021-03-19T12:11:32Z

hack/tools/logcheck/main.go

+		} else {
+			pass.Report(analysis.Diagnostic{
+				Pos:     fun.Pos(),
+				Message: "Invalid value type for key",


What kind of failures we expect here? Can we give some suggestions to users about fixing this?

mowangdk

Add new commit to fix some suggestions. ptal~ thanks a lot

mowangdk · 2021-03-19T11:38:59Z

hack/tools/logcheck/main.go

+		} else {
+			pass.Report(analysis.Diagnostic{
+				Pos:     fun.Pos(),
+				Message: "Invalid value type for key",


It will be a bit complicated, because it has to check arg type

switch arg.(type) { case *ast.FuncLit: .... case *ast.CompositeLit: .... }

if you are ok with it, i will fix it.

mowangdk · 2021-03-19T11:40:50Z

hack/tools/logcheck/testdata/data.go

+	klog.InfoS("Starting container in a pod", "containerID", "containerID", "pod")       // want `Invalid Number of arguments for InfoS`
+	klog.ErrorS(nil, "Starting container in a pod", "containerID", "containerID", "pod") // want `Invalid Number of arguments for ErrorS`
+	klog.InfoS("Starting container in a pod", "测试", "containerID")                       // want `Invalid value for key`
+	klog.ErrorS(nil, "Starting container in a pod", "测试", "containerID")                 // want `Invalid value for key`


Seems like it match by prefix~

mowangdk · 2021-03-20T02:02:22Z

hack/tools/logcheck/main.go

+	if len(keyValues)%2 != 0 {
+		pass.Report(analysis.Diagnostic{
+			Pos:     fun.Pos(),
+			Message: fmt.Sprintf("Invalid Number of arguments for %s", funName),


get, will fixed on next push, Thanks for advice~

serathius · 2021-03-20T07:45:19Z

/lgtm
Please mark the Outdated comments as resolved.
/assign @dims

dims · 2021-03-20T17:26:12Z

/approve

thanks @serathius

k8s-ci-robot · 2021-03-20T17:26:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, mowangdk, serathius

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~~ [dims]

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

Add check for InfoS & ErrorS parameters

Add check for InfoS & ErrorS parameters

4e4135c

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Mar 17, 2021

k8s-ci-robot requested review from detiber and pohly March 17, 2021 04:15

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 17, 2021

adisky reviewed Mar 19, 2021

View reviewed changes

adisky mentioned this pull request Mar 19, 2021

Fix structured logs for pkg/kubelet/network/dns/dns.go kubernetes/kubernetes#100393

Merged

serathius reviewed Mar 19, 2021

View reviewed changes

Fix by pr suggestions

a18bc97

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 20, 2021

mowangdk commented Mar 20, 2021

View reviewed changes

k8s-ci-robot assigned dims and serathius Mar 20, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 20, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 20, 2021

k8s-ci-robot merged commit e158208 into kubernetes:master Mar 20, 2021

pohly pushed a commit to pohly/klog that referenced this pull request Jun 10, 2022

Merge pull request kubernetes#229 from mowangdk/check_func_params

d00a4c1

Add check for InfoS & ErrorS parameters

pohly pushed a commit to pohly/logtools that referenced this pull request Jun 14, 2022

Merge pull request kubernetes/klog#229 from mowangdk/check_func_params

093af11

Add check for InfoS & ErrorS parameters

mowangdk mentioned this pull request Apr 2, 2024

REQUEST: New membership for mowangdk kubernetes/org#4860

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add check for InfoS & ErrorS parameters #229

Add check for InfoS & ErrorS parameters #229

mowangdk commented Mar 17, 2021

adisky Mar 19, 2021

serathius Mar 19, 2021

mowangdk Mar 20, 2021

adisky Mar 19, 2021

serathius Mar 19, 2021 •

edited

adisky Mar 19, 2021

mowangdk Mar 19, 2021

adisky Mar 19, 2021

mowangdk Mar 19, 2021

adisky Mar 19, 2021

adisky commented Mar 19, 2021

serathius Mar 19, 2021

serathius Mar 19, 2021

serathius Mar 19, 2021 •

edited

serathius Mar 19, 2021

mowangdk left a comment

mowangdk Mar 19, 2021

mowangdk Mar 19, 2021

mowangdk Mar 20, 2021

serathius commented Mar 20, 2021

dims commented Mar 20, 2021

k8s-ci-robot commented Mar 20, 2021

	Message: fmt.Sprintf("Invalid value for key %v, it's must be ascii", lit.Value),
	Message: fmt.Sprintf("Invalid value for key %v, it must be ascii", lit.Value),

Add check for InfoS & ErrorS parameters #229

Add check for InfoS & ErrorS parameters #229

Conversation

mowangdk commented Mar 17, 2021

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

serathius Mar 19, 2021 • edited

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

adisky commented Mar 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serathius Mar 19, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mowangdk left a 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

serathius commented Mar 20, 2021

dims commented Mar 20, 2021

k8s-ci-robot commented Mar 20, 2021

serathius Mar 19, 2021 •

edited

serathius Mar 19, 2021 •

edited