Allow custom implementation of toZapLevel function

[nilebox]

The standard toZapLevel implementation has a few issues:

• Messages at V(2) and above are mapped to negative (non-existent) zapcore.Level below Debug
• This also means that even if zap is configured at zapcore.DebugLevel, all log messages from V(2) and above will be dropped

The recommended workaround is to set zapcore.Level(-4) to see log messages up to V(4) verbosity: #84 (comment)
However this also means that all messages logged as .Debug(...) directly to zap logger will be preserved as well.
In addition, there is no consistency between libraries on which verbosity levels map to Info or Debug (e.g. client-go and OpenTelemetry), as mentioned in the issue above.

Adding support for custom toZapLevel implementation would allow to:

• configure desired mappings for each library's logr use which we redirect logs from to zap
• manage desired verbosity of logs for each library separately, and also separate from the direct use of zap

[nilebox]

This test case illustrates the main problem I'm running into, which a custom toZapLevel solves in test cases below.

[pohly]

The recommended workaround is to set zapcore.Level(-4) to see log messages up to V(4) verbosity: #84 (comment)
However this also means that all messages logged as .Debug(...) directly to zap logger will be preserved as well.

Which isn't a problem, is it? logger.V(4) is "klog.V(4).InfoS - Debug level verbosity" (https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md), so zapcore.Level(-4) is asking for debug log output and that is what is being written.

In addition, there is no consistency between libraries on which verbosity levels map to Info or Debug (e.g. client-go and OpenTelemetry), as mentioned in the issue above.

This is a valid problem. logr didn't recommend V() levels. When writing code which is meant to co-exist with Kubernetes, then https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/logging.md should be used.

Adding support for custom toZapLevel implementation would allow to: [...]

I need to look at your implementation, but I see one issue with this: doesn't this customization need to know who is calling it? We cannot simply map V(8) to zapcore.Debug because the caller might be code following the Kubernetes conventions and logging that output is not recommended.

[pohly]

Can you add an example of some real-world usage of this new feature?

[nilebox]

I have described the real world example in #84 (comment)

[nilebox]

Also see even more detailed example below: #86 (comment)

[pohly]

The key point that needs to be called out is that there will be several different zapr instances with the same zap backend in the same program: one for each package or set of packages with its own conventions regarding the logr level. If this is possible, then this option makes sense.

Where it breaks down is when different packages use the same logr instance (for example, by getting it from the context) and then use inconsistent log levels. That is what I was wondering about in #86 (comment).

[nilebox]

Where it breaks down is when different packages use the same logr instance (for example, by getting it from the context) and then use inconsistent log levels.

Agreed, this can only be fixed by addressing the inconsistency between the original libraries.
Even then, I believe the customisable toZapLevel function is a useful improvement:

• The default behaviour is preserved as is, no hidden or breaking changes
• It allows to fix the mapping for klog (I believe the current default implementation is still wrong to consider V(0) the only equivalent of Info and V(1) to be the only equivalent of Debug and V(2) and above not even captured by standard zap level at all), i.e. allows

	klogToZapLevel := func(lvl int) zapcore.Level {
		if lvl >= 4 {
			return zapcore.DebugLevel
		}
		if lvl > 0 {
			return zapcore.InfoLevel
		}
		return zapcore.WarnLevel
	}

to be used globally instead of the default if preferred.

I also think it's better to recommend defining custom toZapLevel (i.e. make zapr work for the standard zap config) over zap.NewAtomicLevelAt(zapcore.Level(-2)) suggested in https://github.com/go-logr/zapr?tab=readme-ov-file#increasing-verbosity (i.e. having to change the zap config itself), but it is up to you.

[pohly]

I believe the customisable toZapLevel function is a useful improvement

Yes, let's merge this. We just need to be sure that developers understand when and how to use it.

I also think it's better to recommend defining custom toZapLevel (i.e. make zapr work for the standard zap config) over zap.NewAtomicLevelAt(zapcore.Level(-2)) suggested in https://github.com/go-logr/zapr?

That depends a bit on how the zap backend gets configured. In component-base/logs we map the program's -v parameter to the zapcore.Level and the current mapping works as intended.

Can you update the documentation as part of your PR?

[nilebox]

I see one issue with this: doesn't this customization need to know who is calling it?

No, this is not an issue.
I configure zap logger in my application first the standard way (not using zapr yet at all):

config := zap.NewProductionConfig()
config.Level.SetLevel(zapcore.InfoLevel)
logger, _ := config.Build()

Then I configure forwarding client-go logs to zap:

klog.SetLogger(zapr.NewLoggerWithOptions(logger, ToZapLevel(klogToZapLevel))

And then I configure forwarding OpenTelemetry logs to zap:

opentelemetry_global.SetLogger(zapr.NewLoggerWithOptions(logger, ToZapLevel(otelToZapLevel))

You can find implementations of klogToZapLevel and otelToZapLevel in #84 (comment)

[nilebox]

@pohly looks like this PR requires some maintainer's approval to get the builds running?

[pohly]

Looks like GitHub actions are no longer working. I won't be able to get to that before sometime next week.

[pohly]

@nilebox: I merged #87, but GitHub is not picking up the new tests for this PR. Can you rebase?