Skip to content

✨ Add ability to set/get logger in/from a context.Context #1093

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 3, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions alias.go
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,11 @@ var (
// get any actual logging.
Log = log.Log

// LoggerFromContext returns a logger with predefined values from a context.Context.
//
// This is meant to be used with the context supplied in a struct that satisfies the Reconciler interface.
LoggerFromContext = log.FromContext

// SetLogger sets a concrete logging implementation for all deferred Loggers.
SetLogger = log.SetLogger
)
25 changes: 25 additions & 0 deletions pkg/log/log.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,15 @@ limitations under the License.
package log

import (
"context"

"github.com/go-logr/logr"
)

var (
contextKey = &struct{}{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

err...Is this common, can we just claim a key like this?

Copy link
Member Author

@vincepri vincepri Aug 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is the approach pointed out in #1054 (comment)

It's actually a pretty nice approach for this use case, it won't use a string which might cause issues and the pointer is used to determine equality, which users can't overwrite

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need the equality? I have to admit I am not an expert on how a context is supposed to be used but since it ends up being shared among code owned by different parties I intuitively thought it would make sense to somehow scope context keys to the project (e.G. via a string prefix) to avoid collisions that will result in unexpected behavior

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can always change it later, unless we don't want to merge this for some reason, I do like that for now this is private and not allowed to be overwritten, I'd rather have this go in given that it has been proven in another project to work well and iterate on it as we go

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From that comment thread, there are at least 2 different active projects linked that use the same approach (cert-manager and go-grpc-middleware), so +1 to this approach

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually didn't realize the context key is the pointer address and was concerned this might conflict with someone else also using an empty struct as identifier.
/lgtm

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this breaks when you have multiple of such context keys. They are likely to be identical as they can have the same address: https://play.golang.org/p/4uqlsUv5Xuf

The Go ref (https://golang.org/ref/spec#Size_and_alignment_guarantees) says:

Two distinct zero-size variables may have the same address in memory.

)

// SetLogger sets a concrete logging implementation for all deferred Loggers.
func SetLogger(l logr.Logger) {
Log.Fulfill(l)
Expand All @@ -46,3 +52,22 @@ func SetLogger(l logr.Logger) {
// to another logr.Logger. You *must* call SetLogger to
// get any actual logging.
var Log = NewDelegatingLogger(NullLogger{})

// FromContext returns a logger with predefined values from a context.Context.
func FromContext(ctx context.Context, keysAndValues ...interface{}) logr.Logger {
var log logr.Logger
if ctx == nil {
log = Log
} else {
lv := ctx.Value(contextKey)
log = lv.(logr.Logger)
}
log.WithValues(keysAndValues...)
return log
}

// IntoContext takes a context and sets the logger as one of its keys.
// Use FromContext function to retrieve the logger.
func IntoContext(ctx context.Context, log logr.Logger) context.Context {
return context.WithValue(ctx, contextKey, log)
}