pkg/log/zap: adding helpers for zap logger configuration

[joelanford]

Description of the change:
Adds a helper to register flags for the zap logger configuration and create a logr.Logger based on those flags.

This is based on the idea I suggested in #870. I'm by no means set on this implementation, but wanted to get it out there to help with the discussion.

Motivation for the change:
See #870

[shawn-hurley]

I am oddly concerned about the meta flag zap-devel (maybe change this flag name or add a shorthand?) controlling the defaults for the other flags. At the very least we need to expose this behavior to the user via the help text IMO.

Overall this is on the right track and as discussed offline, we should see if the upstream zapr project may want to have their flags accessible in this way.

[joelanford]

@shawn-hurley For now, I've updated the --zap-devel help text to make it clear that it changes defaults of other flags. What did you have in mind for a different name and/or adding a shorthand?

The one concern I have about using a shorthand in this case is that we currently have this zap- prefix. So any shorthand we use kind of promotes the flag. Maybe that's just me being overly cautious though.

[shawn-hurley]

We can always add a shorthand, and your right about the prefix. Instead of zap-devel I might prefer zap-development-config or something more verbose but the help text is clear so if others are good I am fine.

[lilic]

Few questions.

[estroz]

Looks good overall but the factory seems unnecessary. Your suggestion is a good alternative.

[lilic]

lgtm

[hasbro17]

@joelanford I'm wondering what happens if you set --zap-level=warn.

From my recollection of zap levels this will set the minimum level to WarnLevel or +1. Meaning any logs at lower levels like InfoLevel (0) and Debug (-1) won't be shown.

I mentioned how zap levels map to logr/zapr levels in a previous comment.
Given that mapping, setting --zap-level to anything above Info won't be of much use since the highest priority level that you can log with logr is v(0) or Info (0). The rest are just Debug levels v(1)==>Debug (-1) v(2)==>Custom Debug (-2) and so on.

So I would propose this flag should only let you set Info and Debug levels: 0,-1,-2,-3,.... since this logger is always meant to be used in conjunction with zapr via logf.SetLogger(zap.Logger()).

Unless of course I'm completely off about my initial assumption of --zap-level=warn.

[joelanford]

I think you're correct. I'll verify and fix.

[joelanford]

Just checked and found a few things:

1. Error() is part of the logr.Logger interface, so at a minimum we would need debug, info, and error.
2. It's possible to get handles to each of the log levels using the logr.Logger interface :

   log.V(2).Info("DEBUG Level 2")
   log.V(1).Info("DEBUG Level 1")
   log.V(0).Info("INFO Level (with specific verbosity number)")
   log.Info("INFO Level (without specific verbosity number)")
   log.V(-1).Info("WARN Level")
   log.V(-2).Info("", "error", "ERROR Level (with specific verbosity number)")
   log.Error(errors.New("ERROR Level (without specific verbosity number)"), "")
   log.V(-3).Info("DPANIC Level")
   log.V(-4).Info("PANIC Level")
   log.V(-5).Info("FATAL Level")

3. It is not currently possible to set a log level to get log.V(2) (or higher DEBUG levels) to show up. Only log.V(1) will output debug logs. If we allow integers for the --zap-level flag, we could probably make higher DEBUG levels work.

[hasbro17]

Yeah I think it's fine if we allow integers for --zap-level.
What should the allowed range be though? I'm a little doubtful of the utility of allowing users to set it to anything above 0(info).
Say --zap-level=1 or warn. In this case all info(0) and debug(-1,-2,..) logs will not be shown.

[joelanford]

I would say that we should allow 0 to infinity (0 = info, 1 = debug, 2=debug2, etc.) for integers. That would mean there would be overlap for info/0 and debug/1 values, but I think that's better than seemingly switching arbitrarily from string to integer levels at 2 and above.

I think the integer should be positive and increasing for higher debug levels to match the integer that's used in the logr interface. So we'd end up allowing the following flag values:

• fatal
• panic
• dpanic
• error
• warn
• info (optionally allow 0?)
• debug (optionally allow 1?)
• 2
• 3
• etc...

[hasbro17]

Makes sense. Having integers for 0,1,2,3... is fine.
Logr discourages using anything other than info, error and integer debug levels so ideally users just stick to the integers.

But if for some reason users want to set the minimum level to warn that skips all info and debug logs, well I guess we'll allow that.

[joelanford]

For the record, I'm not opposed to slimming the accepted values down to the following to be more in line with logr:

• error
• info/0
• debug/1
• 2
• 3
• etc...

At that point we could probably name the flag --log-level, since it would be applicable to any logr implementation.

[joelanford]

@hasbro17 now we're back full circle. Hitting this panic trying to set the log level to higher debug levels: kubernetes-sigs/controller-runtime#167 (comment)

Zap has a well-defined set of levels between debug and fatal, so it makes sense that trying to use a log level outside of that range would fail.

I guess this means there's no need to support integers for the flag since it is zap-specific? Any opinions on using all of them or just the ones relevant to logr? I think I lean toward only the logr ones (debug, info, and error).

I'll find some place to document that log.V(2) (or higher) is not supported by the zap logger.

[hasbro17]

@joelanford I forgot about that bug. I was sure that zap supports custom debug levels like v(-2) and that bug was actually in zapr. But I'll need to take a closer look.

For now I agree that we can keep this simple and just provide debug, info, and error as configurable levels and be in line with logr conventions. And we can document log.V(2) as not being supported.

[hasbro17]

LGTM