Skip to content

Fatalw("...", err) logging pattern is invalid #5746

@grantr

Description

@grantr

#3866 added a bunch of logging statements that look like this:

logger.Fatalw("Version check failed: %v", err)

This is incorrect. Actually the err arg should be wrapped in a zap field:

logger.Fatalw("Version check failed: %v", zap.Error(err))

The sugared logger makes this extremely difficult to detect. One has to inspect the log message produced to see that it's wrong. This has come up before in knative/pkg#603 and #3345.

Eventing has the same issue since knative/eventing#1695.

<logging-rant> The non-sugared logger makes this a compile-time error instead of a runtime error at the cost of slightly more verbose (but type-safe and IMO easier to read) syntax. I much prefer it. </logging-rant>

In what area(s)?

/area monitoring

What version of Knative?

v0.9.0-123-gceaeb2350

Expected Behavior

When a fatal log occurs, it's logged successfully by zap.

Actual Behavior

Zap logs a message saying it's been called incorrectly:

{"level":"dpanic","ts":"2019-10-02T02:37:15.219Z","logger":"eventing-webhook","caller":"zap/sugar.go:210","msg":"Ignored key without a value.","commit":"a1a0981","knative.dev/controller":"eventing-webhook","ignored":"kubernetes version \"\" is not compatible, need at least \"v1.11.0\"","stacktrace":"main.main\n\t/home/prow/go/src/knative.dev/eventing/cmd/webhook/main.go:97\nruntime.main\n\t/usr/local/go/src/runtime/proc.go:200"}

(from knative/eventing#1991)

Steps to Reproduce the Problem

In this particular example, start any controller binary with the env var KUBERNETES_MIN_VERSION=1.15.0 or whatever version is greater than your cluster.

Metadata

Metadata

Assignees

No one assigned

    Labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions