log: add option to deactivate zap stack trace generation

[vboulineau]

Add an option to deactivate stack trace generation, similar to an existing option in Zap itself.
https://github.com/uber-go/zap/blob/v1.10.0/config.go#L202

For some of our public operators, we would like to keep logs clean and tidy to improve readability/investigations for our customers

[estroz]

Instead of adding a new flag, it may be more convenient to add a --zap-level=disabled.

Con: it doesn't exactly align with zap log levels
Pro: it adds the desired functionality without cluttering the CLI

IMO it is ok to have CLI decoupled from configuration. @joelanford @hasbro17 @jmrodri thoughts?

[estroz]

@vboulineau also make sure to run

$ make gen-cli-doc

before pushing so docs are generated correctly.

[joelanford]

@estroz --zap-level is the level at which log messages are written to the output. If I understand correctly, this flag is for disabling the stacktrace, which is a different thing. For example, you might want to see logs starting at the first debug level, but stacktraces starting only at the panic level.

@vboulineau We've recently been contributing to the upstream controller-runtime zap package, with the goal being to phase out SDK's log package.

One of the options currently exposed there is the stacktrace level. Based on that, I'm curious if you think a --zap-stacktrace-level flag would work. You could set --zap-stacktrace-level=panic, which would still log a stacktrace, but only on when using the panic log level (which also causes the program to panic).

If we go this direction, it aligns well with the upstream zap logging package, it provides more fine-grained stacktrace configuration, and it (I think) still meets your use case of cleaning up the logs by allowing you to set it to a level such that it that will only show up in catastrophic situations.

What do you think?

[vboulineau]

@joelanford I guess it makes sense to log stack trace on panic, as it's likely to be an unexpected behaviour, I can rework the PR to add this flag instead!

Update: PR updated with a stacktrace level. Unfortunately I had to create another 'level' type to avoid cases of stackLevel being < logLevel and to have proper default value in the doc generation (error), I recognised it's not super clean but I was not able to find something better (feel free if you have a better idea).

[camilamacedo86]

Hi @vboulineau,

See that these changes are not passing in the lint check:

Running golangci-lint
pkg/log/zap/logger.go:53:13: struct of size 64 bytes could be of size 56 bytes (maligned)
type config struct {
^

To check it locally, specifically, run ./hack/tests/check-lint.sh ci
To solve move the sample bool for the latest position of the struct.

[vboulineau]

Updated, thanks for picking that up!

[vboulineau]

Hey team, do you think this could be merged for 0.16? We were hoping to have it in 0.15 but it seems the release happened already.

[hasbro17]

Suggested change

	- Add a new option to set the minimum loglevel that triggers stack trace generation in logs (`--zap-stacktrace-level`) ([#2319](https://github.com/operator-framework/operator-sdk/pull/2319))
	- Add a new option to set the minimum log level that triggers stack trace generation in logs (`--zap-stacktrace-level`) ([#2319](https://github.com/operator-framework/operator-sdk/pull/2319))

[vboulineau]

Fixed, thank you

[joelanford]

@vboulineau Can you fix the merge conflicts? I think it requires re-running make gen-cli-doc

[hasbro17]

@vboulineau SGTM other than rebasing and the typo in the CHANGELOG.

[vboulineau]

Thanks for the reviews, rebased and fix CHANGELOG, should be good to go.

[hasbro17]

LGTM

@vboulineau Thanks for working on this. We should be able to get this in the next release now.