contextual logging

[pohly]

What this PR does / why we need it:

Contextual logging enables several new use cases as shown in https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging.

This PR is an implementation of the logging side of the revised KEP (currently pending in review). A PR with the logcheck changes will be filed separately.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Related-to: kubernetes/enhancements#3077

Special notes for your reviewer:

We probably should merge the KEP change first.

This PR is best looked at commit-by-commit.

Release note:

Additional API calls for [contextual logging](https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/3077-contextual-logging).

[pohly]

I need to call out one thing here: this requirement that direct log calls are also okay is new. Some loggers that were installed with SetLogger for use as backend might not work as expected because they don't check verbosity.

For example, the original JSON logger from component-base/logs didn't check verbosity and instead relied entirely on klog to do that for it. If called directly, all log entries would have gotten emitted.

Given that SetLogger is still relatively new, I feel that this change of semantics is acceptable. But if we want to be absolutely sure about not breaking anyone, we can also do this instead:

• keep SetLogger as it is (i.e. loggers will only be called as backend)
• add a new SetContextualLogger (same semantic as described here)
• both functions set globalLogger, but also a boolean
• Background returns klogr (i.e. logging goes through klog) when SetLogger was used, otherwise the logger

[pohly]

@serathius pointed out that SetLogger actually predates structured logging and was used already earlier to redirect Kubernetes output into other loggers.

A code search indeed finds users who rely on verbosity filtering in klog:

• https://github.com/projectcontour/contour/blob/b9413e8851be0ff8dfb230ef872a015b168e990b/internal/k8s/log.go#L78-L84
• https://github.com/Azure/secrets-store-csi-driver-provider-azure/blob/07b6ace393f32fd57a0c1f51e62155e4c2fc499a/cmd/main.go#L56 (the old JSON logger without verbosity)

Let's play it safe and use SetContextualLogger.

[dims]

/assign @thockin @serathius

[serathius]

Why have two different way to switch contextualLogger on? I think that having one switch like EnableContextualLogging is enough. Having two ways makes creates inconsistency and needless discussion on which option should be used.

[pohly]

It's just a feeling, but replacing klog.SetContextualLogger(logger) with

klog.SetLogger(logger)
klog.EnableContextualLogging(true)

doesn't send a clear message to me that logger must meet certain criteria for this to work.

Also, EnableContextualLogging as it is defined right now is about something else and cannot be used to enable the additional usage of the logger.

It's on by default and controls the behavior of calls like FromContext. It's meaning is not "the logger can be used as contextual logger". We would need a separate klog.UseLoggerAsContextualLogger call for that - or something like that. I can't think of a good name and prefer to keep SetContextualLogger.

[serathius]

It's just a feeling, but replacing klog.SetContextualLogger(logger) with

klog.SetLogger(logger)
klog.EnableContextualLogging(true)

doesn't send a clear message to me that logger must meet certain criteria for this to work.

If I remember this criteria, it was that provided logger needs to execute verbosity checks for itself. Right? I think the logr interface already expects that. I don't think we should be guided by rushed implementation for K8s json logging that skipped this.

Just want to make sure we don't bring old baggage to the API.

[pohly]

If I remember this criteria, it was that provided logger needs to execute verbosity checks for itself. Right?

Correct.

I don't think we should be guided by rushed implementation for K8s json logging that skipped this.

We can't be sure that other implementations didn't do the same. Searching for it with https://grep.app/search?q=klog.SetLogger finds several places that'll get broken if we change the requirements for the logger passed to SetLogger:

• https://github.com/projectcontour/contour/blob/d231033fc25e8caf1137282ae8d49cbab562160f/internal/k8s/log.go#L78-L83
• https://github.com/cri-o/cri-o/blob/f1d3a46642377d9bf4cb79255020b07578023540/internal/log/klog.go#L77

[pohly]

I think the logr interface already expects that.

The interface only expects that there is an Enabled function. It does not specify a specific semantic like "compare against your verbosity threshold", therefore an implementation that always returns true is valid. klog did specify a semantic ("verbosity check done by klog, logger should always log what it gets") and we shouldn't change that for SetLogger.

[dims]

hmm. i keep forgetting that. how about the LogSink? roughly

func SetLogger(logger logr.Logger) {
    // call logger.GetSink() and do the
    // interface/method check here
}

func AddSupportsContext(logger logr.Logger) logr.Logger {
     sink := logger.GetSink()
     return logger.WithSink(wrapper{logger})
}

type wrapper struct {
   logr.LogSink
}

func (w wrapper) SupportsContext() bool {
    return true
}

// usage:

klog.SetLogger(klog.AddSupportsContext(logger))

[pohly]

That works. See additional commit for the result.

I find it rather ugly and it's also fragile (wrapping doesn't survive a logger.WithValues, for example), but it gets the job done. I can live with this.

[pohly]

As I noticed while working on this, wrapping like this is dropping the hidden logr.Logger.level field. To me this looks more and more like a hack.

[pohly]

• Make SetContextualLogger an experimental/temporary function that will be removed.
• Identify and file issue to github projects that didn't implement proper verbosity checking.
• Remove SetContextualLogger when all the cases are fixed.

That means whoever wants two use contextual logging and directly call the logger must migrate twice: first to SetContextualLogger because SetLogger will not do what they want, then back to SetLogger once we have changed it. That doesn't sound appealling.

Another alternative is:

• introduce SetContextualLogger as experimental (state of this PR before removing it)
• when we consider it stable, remove the "experimental" tag
• at the same time or later, mark SetLogger as deprecated
• remove it when it no longer seems to be used

I don't know whether we can do that within the klog/v2. I'm also not sure whether "clean API" is worth all the trouble for the community.

Yet another alternative: adding optional functional parameters to SetLogger, without adding SetContextualLogger. The only parameter will be ContextualLogger(enabled bool). This is an API break in theory (function signature changes, so code which uses it for a function pointer breaks), but in practice the only usage seem to be direct calls and those continue to work without changes.

[pohly]

Another alternative is: [...] remove SetLogger when it no longer seems to be used

This implies that downstream users must change their code although it currently works fine for them. If they are happy with the current approach (klog flags + verbosity checks, Logger without verbosity check as backend), why should they change? Do we gain that much from the removal that it is worth breaking working code? I don't think so.

[serathius]

When we changed the default? I thought it was 0, or I misunderstand something?
Why have different default for testing?

[pohly]

Only the testing logger has a higher verbosity. klog still has 0.

The rationale is explained in the next sentence: at that level we get messages that will be useful to understand a failed test.

[pohly]

I've extended the explanations in the comment.

[serathius]

I understand the motivation, I'm not sure if this is new, have been discussed before or we are just adding in this PR because we can.

I don't think that increasing default verbosity is not a simple enough change that we can do it here. For example without understanding the impact of log volume on test infrastructure. I'm also not sure that a dependency should decide on changing the default logging verbosity for testing of k/k repo.

[pohly]

I think there is a misunderstanding. This klog/ktesting Logger is not changing anything in existing tests. Only unit tests that get converted as explained in the KEP and in the example in this repo are going to use it.

Unmodified tests will continue to log through the global klog logger, with the same verbosity as before.

[pohly]

Here's the example: https://github.com/kubernetes/klog/pull/296/files#diff-91dbb7e2e5d41b12cfae2565e8d22d69e19d403d850a6686ca7f786c6f605a28

[pohly]

/hold

Let's conclude the API discussion here, then update and merge the KEP first (kubernetes/enhancements#3222) before this one.

[serathius]

I'm not sure about registering testing flags in init. Have we talked with SIG testing about what is the best way to integrate logging into test framework?

[pohly]

This is a Golang problem, not a SIG Testing problem. I asked Tim and he suggested to ask on golang-nuts. I did that but got no responses.

[pohly]

I've also asked on SIG Testing, just in case that someone there has an opinion: https://kubernetes.slack.com/archives/C09QZ4DQB/p1646918432994899

[dims]

Do we need this to be public?

[pohly]

Only if we expect some LogSinks to implement it directly. They'll need it for compile-time checks that they implement the interface.

I'm fine with saying that only wrapping is supported.

[pohly]

I've pushed an update which doesn't have such an interface at all anymore.

[dims]

instead of this:

	logger = klog.AddSupportsContext(logger)
	klog.SetLogger(logger)

could we do this?:

klog.SetLogger(klog.WithContextLogging(logger))

[pohly]

I then need to change how the result of klog.Background is checked. It's doable.

[pohly]

This is slightly broken: we loose the level field.

If someone does SetLogger(AddSupportsContext(logger.V(1))), then what we do instead is SetLogger(AddSupportedContext(logger)).

I can see a way around that, but it's going to be very ugly.

[pohly]

No, my idea doesn't work. I wanted to call logger.Enabled and then see how LogSink.Enabled is called (it gets a level), but it's not possible to intercept that call.

[dims]

@pohly after sleeping on it, i am ok with adding SetContextualLogger one question i had was

• should SetLogger be deprecated
• if we do deprecate and remove SetLogger, how would people get the functionality they do with SetLogger? Should we add an additional parameter to SetContextualLogger?

thanks!

[pohly]

should SetLogger be deprecated

It serves a particular purpose (use Logger as backend, and only as backend) which is a valid use case. If we deprecate it, then we need a replacement. I don't see a need for deprecating it, but don't feel strongly about it.

if we do deprecate and remove SetLogger, how would people get the functionality they do with SetLogger? Should we add an additional parameter to SetContextualLogger?

If we do that, then I prefer a different name. Currently the name implies a certain semantic, it would be odd to use it when that semantic is not desired.

The alternative is SetLoggerWithOptions(logger logr.Logger, opts ...klog.LoggerOptions), i.e. functional parameters, with an option ContextualLogger(enabled bool). It could be off by default, then SetLoggerWithOptions(logger) will be a direct replacement for a deprecated SetLogger.

[dims]

The alternative is SetLoggerWithOptions(logger logr.Logger, opts ...klog.LoggerOptions), i.e. functional parameters, with an option ContextualLogger(enabled bool). It could be off by default, then SetLoggerWithOptions(logger) will be a direct replacement for a deprecated SetLogger.

So ... i really like this approach! +1 from me.

[pohly]

I also like SetLoggerWithOptions best. Implemented.

[dims]

Squash please @pohly

[dims]

Do we really need this?

[pohly]

It was an explicit request during the KEP review that Kubernetes must have a feature gate for the new functionality. This call here is needed for that feature gate.

The effect is IMHO minimal, but we still need it to comply with the Kubernetes policies.

[dims]

@thockin we worked through a bunch of things here, Can you please review now?

[pohly]

I squashed into stand-alone commits where each of them makes sense by itself.

I also update the corresponding KEP to match the API that we agreed upon here: kubernetes/enhancements#3222

@thockin if you agree with this PR, can you also review that KEP update?

[thockin]

KEP approved. I didn't read this code too deeply because I lack context on it, but it seems roughly the right shape and form.

[dims]

/approve
/lgtm

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dims, pohly

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [dims]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[pohly]

/hold cancel

KEP pending approval, but that's just a matter of getting the right okays now.