Replace --debug and --trace with a fine-grained --log-level flag
#3703
Conversation
| if l == -1 { | ||
| // Zap uses "5" as the value for fatal. | ||
| // We need to pass in "-5" because `SetLevel` passes the negation. | ||
| log.SetLevel(-5) |
There was a problem hiding this comment.
This technically works because there's no logs with this level. Perhaps there's a better way to disable logging.
There was a problem hiding this comment.
Yeah, this is unfortunate if it's the best we can do. If it is, I think it's worth leaving an apologetic, explanatory comment.
There was a problem hiding this comment.
This works, but another idea is to use logr.Discard.
There was a problem hiding this comment.
I'm not familiar enough with the logging setup. How would you implement logr.Discard?
There was a problem hiding this comment.
I think this PR provides a cleaner interface to logging and is in a mergable state. I'm unfamiliar with logr.Discard as well. We can always follow-up if it turns out this change needs more.
There was a problem hiding this comment.
This is neat, thanks for doing it! I would like to also get the opinions of @zricethezav and @mcastorina.
| // Zap uses "5" as the value for fatal. | ||
| // We need to pass in "-5" because `SetLevel` passes the negation. |
There was a problem hiding this comment.
I have some dumb questions:
- What does "uses 5 as the value for fatal" mean here? Are fatal errors are logged at level 5?
- Why does
SetLevelpass the negation? That seems really strange.
There was a problem hiding this comment.
What does "uses 5 as the value for fatal" mean here? Are fatal errors are logged at level 5?
Zap's logging levels use 0 for info, with negatives increasing verbosity and 1-5 decreasing verbosity.
TruffleHog only seems to use levels <= 0, so setting the level to 5 effectively prevents anything from being logged.
Why does SetLevel pass the negation? That seems really strange.
My guess is that someone preferred the look of (+)1->5 for verbosity and not (-)1->5. 🤷♂️
There was a problem hiding this comment.
It originates from the combination of zap as the log implementation for logr.
logrdefines higher numbers as being more verbosezapdefines lower numbers as being more verbose
I left a comment about it in SetLevel:
Lines 27 to 29 in 2a01091
There was a problem hiding this comment.
Ah, I didn't realize it was two different implementations.
| if l == -1 { | ||
| // Zap uses "5" as the value for fatal. | ||
| // We need to pass in "-5" because `SetLevel` passes the negation. | ||
| log.SetLevel(-5) |
There was a problem hiding this comment.
Yeah, this is unfortunate if it's the best we can do. If it is, I think it's worth leaving an apologetic, explanatory comment.
Description:
This was motivated by the discussion in #3589.
It fixes #3668.
Checklist:
make test-community)?make lintthis requires golangci-lint)?