Skip to content
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

contextual logging #296

Merged
merged 4 commits into from Mar 15, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
246 changes: 246 additions & 0 deletions contextual.go
@@ -0,0 +1,246 @@
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package klog

import (
"context"

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

// This file provides the implementation of
// https://github.com/kubernetes/enhancements/tree/master/keps/sig-instrumentation/1602-structured-logging
//
// SetLogger and ClearLogger were originally added to klog.go and got moved
// here. Contextual logging adds a way to retrieve a Logger for direct logging
// without the logging calls in klog.go.
//
// The global variables are expected to be modified only during sequential
// parts of a program (init, serial tests) and therefore are not protected by
// mutex locking.

var (
// contextualLoggingEnabled controls whether contextual logging is
// active. Disabling it may have some small performance benefit.
contextualLoggingEnabled = true

// globalLogger is the global Logger chosen by users of klog, nil if
// none is available.
globalLogger *Logger

// contextualLogger defines whether globalLogger may get called
// directly.
contextualLogger bool

// klogLogger is used as fallback for logging through the normal klog code
// when no Logger is set.
klogLogger logr.Logger = logr.New(&klogger{})
)

// SetLogger sets a Logger implementation that will be used as backing
// implementation of the traditional klog log calls. klog will do its own
// verbosity checks before calling logger.V().Info. logger.Error is always
// called, regardless of the klog verbosity settings.
//
// If set, all log lines will be suppressed from the regular output, and
// redirected to the logr implementation.
// Use as:
// ...
// klog.SetLogger(zapr.NewLogger(zapLog))
//
// To remove a backing logr implemention, use ClearLogger. Setting an
// empty logger with SetLogger(logr.Logger{}) does not work.
//
// Modifying the logger is not thread-safe and should be done while no other
// goroutines invoke log calls, usually during program initialization.
func SetLogger(logger logr.Logger) {
SetLoggerWithOptions(logger)
}

// SetLoggerWithOptions is a more flexible version of SetLogger. Without
// additional options, it behaves exactly like SetLogger. By passing
// ContextualLogger(true) as option, it can be used to set a logger that then
// will also get called directly by applications which retrieve it via
// FromContext, Background, or TODO.
//
// Supporting direct calls is recommended because it avoids the overhead of
// routing log entries through klogr into klog and then into the actual Logger
// backend.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func SetLoggerWithOptions(logger logr.Logger, opts ...LoggerOption) {
globalLogger = &logger
var o loggerOptions
for _, opt := range opts {
opt(&o)
}
contextualLogger = o.contextualLogger
}

// ContextualLogger determines whether the logger passed to
// SetLoggerWithOptions may also get called directly. Such a logger cannot rely
// on verbosity checking in klog.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func ContextualLogger(enabled bool) LoggerOption {
return func(o *loggerOptions) {
o.contextualLogger = enabled
}
}

// LoggerOption implements the functional parameter paradigm for
// SetLoggerWithOptions.
//
// Experimental
//
// Notice: This type is EXPERIMENTAL and may be changed or removed in a
// later release.
type LoggerOption func(o *loggerOptions)

type loggerOptions struct {
contextualLogger bool
}

// SetContextualLogger does the same as SetLogger, but in addition the
// logger may also get called directly by code that retrieves it
// with FromContext, TODO or Background. The logger therefore must
// implements its own verbosity checking.
func SetContextualLogger(logger logr.Logger) {

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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:

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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))

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

  • 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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

globalLogger = &logger
contextualLogger = true
}

// ClearLogger removes a backing Logger implementation if one was set earlier
// with SetLogger.
//
// Modifying the logger is not thread-safe and should be done while no other
// goroutines invoke log calls, usually during program initialization.
func ClearLogger() {
globalLogger = nil
}

// EnableContextualLogging controls whether contextual logging is enabled.
// By default it is enabled. When disabled, FromContext avoids looking up
// the logger in the context and always returns the global logger.
// LoggerWithValues, LoggerWithName, and NewContext become no-ops
// and return their input logger respectively context. This may be useful
// to avoid the additional overhead for contextual logging.
//
// This must be called during initialization before goroutines are started.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func EnableContextualLogging(enabled bool) {
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 really need this?

Copy link
Author

Choose a reason for hiding this comment

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

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.

contextualLoggingEnabled = enabled
}

// FromContext retrieves a logger set by the caller or, if not set,
// falls back to the program's global logger (a Logger instance or klog
// itself).
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func FromContext(ctx context.Context) Logger {
if contextualLoggingEnabled {
if logger, err := logr.FromContext(ctx); err == nil {
return logger
}
}

return Background()
}

// TODO can be used as a last resort by code that has no means of
// receiving a logger from its caller. FromContext or an explicit logger
// parameter should be used instead.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func TODO() Logger {
return Background()
}

// Background retrieves the fallback logger. It should not be called before
// that logger was initialized by the program and not by code that should
// better receive a logger via its parameters. TODO can be used as a temporary
// solution for such code.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func Background() Logger {
if globalLogger != nil && contextualLogger {
return *globalLogger
}

return klogLogger
}

// LoggerWithValues returns logger.WithValues(...kv) when
// contextual logging is enabled, otherwise the logger.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func LoggerWithValues(logger Logger, kv ...interface{}) Logger {
if contextualLoggingEnabled {
return logger.WithValues(kv...)
}
return logger
}

// LoggerWithName returns logger.WithName(name) when contextual logging is
// enabled, otherwise the logger.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func LoggerWithName(logger Logger, name string) Logger {
if contextualLoggingEnabled {
return logger.WithName(name)
}
return logger
}

// NewContext returns logr.NewContext(ctx, logger) when
// contextual logging is enabled, otherwise ctx.
//
// Experimental
//
// Notice: This function is EXPERIMENTAL and may be changed or removed in a
// later release.
func NewContext(ctx context.Context, logger Logger) context.Context {
if contextualLoggingEnabled {
return logr.NewContext(ctx, logger)
}
return ctx
}
43 changes: 43 additions & 0 deletions contextual_test.go
@@ -0,0 +1,43 @@
/*
Copyright 2022 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package klog_test

import (
"fmt"

"github.com/go-logr/logr"
"k8s.io/klog/v2"
)

func ExampleSetLogger() {
// Logger is only used as backend, Background() returns klogr.
klog.SetLogger(logr.Discard())
fmt.Printf("logger after SetLogger: %T\n", klog.Background().GetSink())

// Logger is only used as backend, Background() returns klogr.
klog.SetLoggerWithOptions(logr.Discard(), klog.ContextualLogger(false))
fmt.Printf("logger after SetLoggerWithOptions with ContextualLogger(false): %T\n", klog.Background().GetSink())

// Logger is used as backend and directly.
klog.SetLoggerWithOptions(logr.Discard(), klog.ContextualLogger(true))
fmt.Printf("logger after SetLoggerWithOptions with ContextualLogger(true): %T\n", klog.Background().GetSink())

// Output:
// logger after SetLogger: *klog.klogger
// logger after SetLoggerWithOptions with ContextualLogger(false): *klog.klogger
// logger after SetLoggerWithOptions with ContextualLogger(true): logr.discardLogSink
}
55 changes: 53 additions & 2 deletions examples/output_test/output_test.go
Expand Up @@ -85,7 +85,7 @@ func TestKlogrOutput(t *testing.T) {
})
}

// TestKlogrStackText tests klogr -> klog -> text logger.
// TestKlogrStackText tests klogr.klogr -> klog -> text logger.
func TestKlogrStackText(t *testing.T) {
newLogger := func(out io.Writer, v int, vmodule string) logr.Logger {
// Backend: text output.
Expand All @@ -104,7 +104,7 @@ func TestKlogrStackText(t *testing.T) {
test.Output(t, test.OutputConfig{NewLogger: newLogger, SupportsVModule: true})
}

// TestKlogrStackKlogr tests klogr -> klog -> zapr.
// TestKlogrStackKlogr tests klogr.klogr -> klog -> zapr.
//
// This exposes whether verbosity is passed through correctly
// (https://github.com/kubernetes/klog/issues/294) because klogr logging
Expand Down Expand Up @@ -136,6 +136,57 @@ func TestKlogrStackZapr(t *testing.T) {
test.Output(t, test.OutputConfig{NewLogger: newLogger, ExpectedOutputMapping: mapping})
}

// TestKlogrInternalStackText tests klog.klogr (the simplified version used for contextual logging) -> klog -> text logger.
func TestKlogrInternalStackText(t *testing.T) {
newLogger := func(out io.Writer, v int, vmodule string) logr.Logger {
// Backend: text output.
config := textlogger.NewConfig(
textlogger.Verbosity(v),
textlogger.Output(out),
)
if err := config.VModule().Set(vmodule); err != nil {
panic(err)
}
klog.SetLogger(textlogger.NewLogger(config))

// Frontend: internal klogr.
return klog.NewKlogr()
}
test.Output(t, test.OutputConfig{NewLogger: newLogger, SupportsVModule: true})
}

// TestKlogrInternalStackKlogr tests klog.klogr (the simplified version used for contextual logging) -> klog -> zapr.
//
// This exposes whether verbosity is passed through correctly
// (https://github.com/kubernetes/klog/issues/294) because klogr logging
// records that.
func TestKlogrInternalStackZapr(t *testing.T) {
mapping := test.ZaprOutputMappingIndirect()

// klogr doesn't warn about invalid KVs and just inserts
// "(MISSING)".
for key, value := range map[string]string{
`I output.go:<LINE>] "odd arguments" akey="avalue" akey2="(MISSING)"
`: `{"caller":"test/output.go:<LINE>","msg":"odd arguments","v":0,"akey":"avalue","akey2":"(MISSING)"}
`,

`I output.go:<LINE>] "both odd" basekey1="basevar1" basekey2="(MISSING)" akey="avalue" akey2="(MISSING)"
`: `{"caller":"test/output.go:<LINE>","msg":"both odd","v":0,"basekey1":"basevar1","basekey2":"(MISSING)","akey":"avalue","akey2":"(MISSING)"}
`,
} {
mapping[key] = value
}

newLogger := func(out io.Writer, v int, vmodule string) logr.Logger {
// Backend: zapr as configured in k8s.io/component-base/logs/json.
klog.SetLogger(newZaprLogger(out, v))

// Frontend: internal klogr.
return klog.NewKlogr()
}
test.Output(t, test.OutputConfig{NewLogger: newLogger, ExpectedOutputMapping: mapping})
}

func newZaprLogger(out io.Writer, v int) logr.Logger {
encoderConfig := &zapcore.EncoderConfig{
MessageKey: "msg",
Expand Down