Child loggers

[vincepri]

/kind feature

Describe the solution you'd like
A common use case for loggers is to provide a default context. An example is to prepend some common values to each log line. I've seen other loggers are able to create a child logger of some sort with a predefined set of variables which are used as suffix for each log line.

I'd imagine this approach to have some sort of limitations, for example how many variables are allowed to be set as context. This approach would have the following requirements:

• The global logger has a 1 to many relationship with many child loggers.
• Child loggers have only have a single parent.
• No hard to debug multi-layer nested loggers.
• Sinks (re: Interface based approach #19) are only managed at the global logger.

Thoughts?

[vincepri]

@dims do you have any thoughts around this? I'm happy to start some work on it if there is some consensus on how to proceed on it and some guidance.

[neolit123]

A common use case for loggers is to provide a default context. An example is to prepend some common values to each log line. I've seen other loggers are able to create a child logger of some sort with a predefined set of variables which are used as suffix for each log line.

isn't this solved by the structured logging proposed by https://github.com/go-logr/logr

[vincepri]

Maybe? I haven't looked at logr yet, although in this proposal the child logger would be instantiated from the klog package.

[DirectXMan12]

This looks very similar to how we use logr in controller-runtime. You can check out our code (sigs.k8s.io/controller-runtime/pkg/runtime/log), but the TL;DR is:

logr defines two methods on a logr.Logger object (the main interface):

// instantiates a new sub-logger, appending the name to the set of names
WithName(string) logr.Logger
// instantiates a new logger, adding/setting the given values to the set of values for this logger
WithValues(kvPairs ...interface{}) logr.Logger

Then, in the CR package mentioned above, we define

// magically set up some stuff that allows delayed setting of the actual logr.Logger implementation
var Log logr.Logger = [some magic]

This enables patterns like

import logf "sigs.k8s.io/controller-runtime/pkg/runtime/log"
// in a controller

var log = logf.Log.WithName("controllers").WithName("foo-controller")

// later on
func (r *FooController) Reconcile(req reconcile.Request) (reconcile.Response, error) {
    log := log.WithValues("foo", req.NamespacedName)

   log.Info("starting reconcile")]
   // more stuff
}

[vincepri]

@DirectXMan12 Thanks for the example, that looks pretty much what I was thinking! Any thoughts on bringing that logic in klog?

[neolit123]

logr defines two methods on a logr.Logger object (the main interface):

yep, this was pretty much what i meant with.

isn't this solved by the structured logging proposed by https://github.com/go-logr/logr

[neolit123]

Thanks for the example, that looks pretty much what I was thinking! Any thoughts on bringing that logic in klog

i somehow think this should be on the klog implementer side.
not sure if it should be in core klog. 🤔

[DirectXMan12]

I think we need to figure out how klog relates to logr at some point. One option is that klog implements logr's interfaces, which allows us to keep kube libraries logging-implementation-agnostic (since logr is just an interface). That's what I lean towards, but that definitely needs broader discussion.

[dims]

@DirectXMan12 who should we loop in for this discussion? (just throw it out on -dev mailing list)?

[DirectXMan12]

it's probably a -dev or KEP-worth discussion, yeah.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[vincepri]

/remove-lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[detiber]

/remove-lifecycle rotten

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[detiber]

@vincepri does the current implementation of klogr meet your needs for this issue?

[vincepri]

That’s probably ok for now, we can close this one out

/close

[k8s-ci-robot]

@vincepri: Closing this issue.

In response to this:

That’s probably ok for now, we can close this one out

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.