Repository: kubernetes/klog
Pull request #97 (2019-09-19 12:36:11)

Revert "Merge pull request #20 from munnerz/logr"
This reverts commit cd60aa438f9c750641d5feacd2999e58df8af28f, reversing
changes made to a6a74fbce3a592242b0fc24cd93fd98a4cea0a98.

**What this PR does / why we need it**:
Reverts API-breaking changes in advance of klog/v2

After merging this, the plan is to tag v1.0.0 and switch the module declaration to k8.io/klog/v2 before making breaking changes.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
xref https://github.com/kubernetes/client-go/issues/677 https://github.com/Azure/phippyandfriends/issues/30 https://github.com/DataDog/dd-trace-go/pull/482 https://github.com/kubernetes/client-go/issues/658 https://github.com/kubernetes/client-go/issues/656 https://github.com/kubernetes/client-go/issues/654 https://github.com/kubernetes/client-go/issues/652

See discussion in https://github.com/kubernetes/klog/pull/20#issuecomment-526824589

/cc @dims @munnerz @deads2k @DirectXMan12 


Patch:
diff --git a/klog.go b/klog.go
index 4eff736..2712ce0 100644
--- a/klog.go
+++ b/klog.go
@@ -87,8 +87,6 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
-
-	"github.com/go-logr/logr"
 )
 
 // severity identifies the sort of log: info, warning etc. It also implements
@@ -499,9 +497,6 @@ type loggingT struct {
 
 	// If true, add the file directory to the header
 	addDirHeader bool
-
-	// If set, all output will be redirected unconditionally to the provided logr.Logger
-	logr logr.Logger
 }
 
 // buffer holds a byte Buffer for reuse. The zero value is ready for use.
@@ -684,68 +679,44 @@ func (buf *buffer) someDigits(i, d int) int {
 	return copy(buf.tmp[i:], buf.tmp[j:])
 }
 
-func (l *loggingT) println(s severity, logr logr.InfoLogger, args ...interface{}) {
+func (l *loggingT) println(s severity, args ...interface{}) {
 	buf, file, line := l.header(s, 0)
-	// if logr is set, we clear the generated header as we rely on the backing
-	// logr implementation to print headers
-	if logr != nil {
-		l.putBuffer(buf)
-		buf = l.getBuffer()
-	}
 	fmt.Fprintln(buf, args...)
-	l.output(s, logr, buf, file, line, false)
+	l.output(s, buf, file, line, false)
 }
 
-func (l *loggingT) print(s severity, logr logr.InfoLogger, args ...interface{}) {
-	l.printDepth(s, logr, 1, args...)
+func (l *loggingT) print(s severity, args ...interface{}) {
+	l.printDepth(s, 1, args...)
 }
 
-func (l *loggingT) printDepth(s severity, logr logr.InfoLogger, depth int, args ...interface{}) {
+func (l *loggingT) printDepth(s severity, depth int, args ...interface{}) {
 	buf, file, line := l.header(s, depth)
-	// if logr is set, we clear the generated header as we rely on the backing
-	// logr implementation to print headers
-	if logr != nil {
-		l.putBuffer(buf)
-		buf = l.getBuffer()
-	}
 	fmt.Fprint(buf, args...)
 	if buf.Bytes()[buf.Len()-1] != '\n' {
 		buf.WriteByte('\n')
 	}
-	l.output(s, logr, buf, file, line, false)
+	l.output(s, buf, file, line, false)
 }
 
-func (l *loggingT) printf(s severity, logr logr.InfoLogger, format string, args ...interface{}) {
+func (l *loggingT) printf(s severity, format string, args ...interface{}) {
 	buf, file, line := l.header(s, 0)
-	// if logr is set, we clear the generated header as we rely on the backing
-	// logr implementation to print headers
-	if logr != nil {
-		l.putBuffer(buf)
-		buf = l.getBuffer()
-	}
 	fmt.Fprintf(buf, format, args...)
 	if buf.Bytes()[buf.Len()-1] != '\n' {
 		buf.WriteByte('\n')
 	}
-	l.output(s, logr, buf, file, line, false)
+	l.output(s, buf, file, line, false)
 }
 
 // printWithFileLine behaves like print but uses the provided file and line number.  If
 // alsoLogToStderr is true, the log message always appears on standard error; it
 // will also appear in the log file unless --logtostderr is set.
-func (l *loggingT) printWithFileLine(s severity, logr logr.InfoLogger, file string, line int, alsoToStderr bool, args ...interface{}) {
+func (l *loggingT) printWithFileLine(s severity, file string, line int, alsoToStderr bool, args ...interface{}) {
 	buf := l.formatHeader(s, file, line)
-	// if logr is set, we clear the generated header as we rely on the backing
-	// logr implementation to print headers
-	if logr != nil {
-		l.putBuffer(buf)
-		buf = l.getBuffer()
-	}
 	fmt.Fprint(buf, args...)
 	if buf.Bytes()[buf.Len()-1] != '\n' {
 		buf.WriteByte('\n')
 	}
-	l.output(s, logr, buf, file, line, alsoToStderr)
+	l.output(s, buf, file, line, alsoToStderr)
 }
 
 // redirectBuffer is used to set an alternate destination for the logs
@@ -765,18 +736,6 @@ func (rb *redirectBuffer) Write(bytes []byte) (n int, err error) {
 	return rb.w.Write(bytes)
 }
 
-// SetLogger will set the backing logr implementation for klog.
-// If set, all log lines will be supressed from the regular Output, and
-// redirected to the logr implementation.
-// All log lines include the 'severity', 'file' and 'line' values attached as
-// structured logging values.
-// Use as:
-//   ...
-//   klog.SetLogger(zapr.NewLogger(zapLog))
-func SetLogger(logr logr.Logger) {
-	logging.logr = logr
-}
-
 // SetOutput sets the output destination for all severities
 func SetOutput(w io.Writer) {
 	logging.mu.Lock()
@@ -804,7 +763,7 @@ func SetOutputBySeverity(name string, w io.Writer) {
 }
 
 // output writes the data to the log files and releases the buffer.
-func (l *loggingT) output(s severity, log logr.InfoLogger, buf *buffer, file string, line int, alsoToStderr bool) {
+func (l *loggingT) output(s severity, buf *buffer, file string, line int, alsoToStderr bool) {
 	l.mu.Lock()
 	if l.traceLocation.isSet() {
 		if l.traceLocation.match(file, line) {
@@ -812,15 +771,7 @@ func (l *loggingT) output(s severity, log logr.InfoLogger, buf *buffer, file str
 		}
 	}
 	data := buf.Bytes()
-	if log != nil {
-		// TODO: set 'severity' and caller information as structured log info
-		// keysAndValues := []interface{}{"severity", severityName[s], "file", file, "line", line}
-		if s == errorLog {
-			l.logr.Error(nil, string(data))
-		} else {
-			log.Info(string(data))
-		}
-	} else if l.toStderr {
+	if l.toStderr {
 		os.Stderr.Write(data)
 	} else {
 		if alsoToStderr || l.alsoToStderr || s >= l.stderrThreshold.get() {
@@ -1125,7 +1076,7 @@ func (lb logBridge) Write(b []byte) (n int, err error) {
 	}
 	// printWithFileLine with alsoToStderr=true, so standard log messages
 	// always appear on standard error.
-	logging.printWithFileLine(severity(lb), logging.logr, file, line, true, text)
+	logging.printWithFileLine(severity(lb), file, line, true, text)
 	return len(b), nil
 }
 
@@ -1157,23 +1108,13 @@ func (l *loggingT) setV(pc uintptr) Level {
 
 // Verbose is a boolean type that implements Infof (like Printf) etc.
 // See the documentation of V for more information.
-type Verbose struct {
-	enabled bool
-	logr    logr.InfoLogger
-}
-
-func newVerbose(level Level, b bool) Verbose {
-	if logging.logr == nil {
-		return Verbose{b, nil}
-	}
-	return Verbose{b, logging.logr.V(int(level))}
-}
+type Verbose bool
 
 // V reports whether verbosity at the call site is at least the requested level.
-// The returned value is a struct of type Verbose, which implements Info, Infoln
+// The returned value is a boolean of type Verbose, which implements Info, Infoln
 // and Infof. These methods will write to the Info log if called.
 // Thus, one may write either
-//	if glog.V(2).Enabled() { klog.Info("log this") }
+//	if klog.V(2) { klog.Info("log this") }
 // or
 //	klog.V(2).Info("log this")
 // The second form is shorter but the first is cheaper if logging is off because it does
@@ -1189,7 +1130,7 @@ func V(level Level) Verbose {
 
 	// Here is a cheap but safe test to see if V logging is enabled globally.
 	if logging.verbosity.get() >= level {
-		return newVerbose(level, true)
+		return Verbose(true)
 	}
 
 	// It's off globally but it vmodule may still be set.
@@ -1201,145 +1142,138 @@ func V(level Level) Verbose {
 		logging.mu.Lock()
 		defer logging.mu.Unlock()
 		if runtime.Callers(2, logging.pcs[:]) == 0 {
-			return newVerbose(level, false)
+			return Verbose(false)
 		}
 		v, ok := logging.vmap[logging.pcs[0]]
 		if !ok {
 			v = logging.setV(logging.pcs[0])
 		}
-		return newVerbose(level, v >= level)
+		return Verbose(v >= level)
 	}
-	return newVerbose(level, false)
-}
-
-// Enabled will return true if this log level is enabled, guarded by the value
-// of v.
-// See the documentation of V for usage.
-func (v Verbose) Enabled() bool {
-	return v.enabled
+	return Verbose(false)
 }
 
 // Info is equivalent to the global Info function, guarded by the value of v.
 // See the documentation of V for usage.
 func (v Verbose) Info(args ...interface{}) {
-	if v.enabled {
-		logging.print(infoLog, v.logr, args...)
+	if v {
+		logging.print(infoLog, args...)
 	}
 }
 
 // Infoln is equivalent to the global Infoln function, guarded by the value of v.
 // See the documentation of V for usage.
 func (v Verbose) Infoln(args ...interface{}) {
-	if v.enabled {
-		logging.println(infoLog, v.logr, args...)
+	if v {
+		logging.println(infoLog, args...)
 	}
 }
 
 // Infof is equivalent to the global Infof function, guarded by the value of v.
 // See the documentation of V for usage.
 func (v Verbose) Infof(format string, args ...interface{}) {
-	if v.enabled {
-		logging.printf(infoLog, v.logr, format, args...)
+	if v {
+		logging.printf(infoLog, format, args...)
 	}
 }
 
 // Info logs to the INFO log.
 // Arguments are handled in the manner of fmt.Print; a newline is appended if missing.
 func Info(args ...interface{}) {
-	logging.print(infoLog, logging.logr, args...)
+	logging.print(infoLog, args...)
 }
 
 // InfoDepth acts as Info but uses depth to determine which call frame to log.
 // InfoDepth(0, "msg") is the same as Info("msg").
 func InfoDepth(depth int, args ...interface{}) {
-	logging.printDepth(infoLog, logging.logr, depth, args...)
+	logging.printDepth(infoLog, depth, args...)
 }
 
 // Infoln logs to the INFO log.
 // Arguments are handled in the manner of fmt.Println; a newline is always appended.
 func Infoln(args ...interface{}) {
-	logging.println(infoLog, logging.logr, args...)
+	logging.println(infoLog, args...)
 }
 
 // Infof logs to the INFO log.
 // Arguments are handled in the manner of fmt.Printf; a newline is appended if missing.
 func Infof(format string, args ...interface{}) {
-	logging.printf(infoLog, logging.logr, format, args...)
+	logging.printf(infoLog, format, args...)
 }
 
 // Warning logs to the WARNING and INFO logs.
 // Arguments are handled in the manner of fmt.Print; a newline is appended if missing.
 func Warning(args ...interface{}) {
-	logging.print(warningLog, logging.logr, args...)
+	logging.print(warningLog, args...)
 }
 
 // WarningDepth acts as Warning but uses depth to determine which call frame to log.
 // WarningDepth(0, "msg") is the same as Warning("msg").
 func WarningDepth(depth int, args ...interface{}) {
-	logging.printDepth(warningLog, logging.logr, depth, args...)
+	logging.printDepth(warningLog, depth, args...)
 }
 
 // Warningln logs to the WARNING and INFO logs.
 // Arguments are handled in the manner of fmt.Println; a newline is always appended.
 func Warningln(args ...interface{}) {
-	logging.println(warningLog, logging.logr, args...)
+	logging.println(warningLog, args...)
 }
 
 // Warningf logs to the WARNING and INFO logs.
 // Arguments are handled in the manner of fmt.Printf; a newline is appended if missing.
 func Warningf(format string, args ...interface{}) {
-	logging.printf(warningLog, logging.logr, format, args...)
+	logging.printf(warningLog, format, args...)
 }
 
 // Error logs to the ERROR, WARNING, and INFO logs.
 // Arguments are handled in the manner of fmt.Print; a newline is appended if missing.
 func Error(args ...interface{}) {
-	logging.print(errorLog, logging.logr, args...)
+	logging.print(errorLog, args...)
 }
 
 // ErrorDepth acts as Error but uses depth to determine which call frame to log.
 // ErrorDepth(0, "msg") is the same as Error("msg").
 func ErrorDepth(depth int, args ...interface{}) {
-	logging.printDepth(errorLog, logging.logr, depth, args...)
+	logging.printDepth(errorLog, depth, args...)
 }
 
 // Errorln logs to the ERROR, WARNING, and INFO logs.
 // Arguments are handled in the manner of fmt.Println; a newline is always appended.
 func Errorln(args ...interface{}) {
-	logging.println(errorLog, logging.logr, args...)
+	logging.println(errorLog, args...)
 }
 
 // Errorf logs to the ERROR, WARNING, and INFO logs.
 // Arguments are handled in the manner of fmt.Printf; a newline is appended if missing.
 func Errorf(format string, args ...interface{}) {
-	logging.printf(errorLog, logging.logr, format, args...)
+	logging.printf(errorLog, format, args...)
 }
 
 // Fatal logs to the FATAL, ERROR, WARNING, and INFO logs,
 // including a stack trace of all running goroutines, then calls os.Exit(255).
 // Arguments are handled in the manner of fmt.Print; a newline is appended if missing.
 func Fatal(args ...interface{}) {
-	logging.print(fatalLog, logging.logr, args...)
+	logging.print(fatalLog, args...)
 }
 
 // FatalDepth acts as Fatal but uses depth to determine which call frame to log.
 // FatalDepth(0, "msg") is the same as Fatal("msg").
 func FatalDepth(depth int, args ...interface{}) {
-	logging.printDepth(fatalLog, logging.logr, depth, args...)
+	logging.printDepth(fatalLog, depth, args...)
 }
 
 // Fatalln logs to the FATAL, ERROR, WARNING, and INFO logs,
 // including a stack trace of all running goroutines, then calls os.Exit(255).
 // Arguments are handled in the manner of fmt.Println; a newline is always appended.
 func Fatalln(args ...interface{}) {
-	logging.println(fatalLog, logging.logr, args...)
+	logging.println(fatalLog, args...)
 }
 
 // Fatalf logs to the FATAL, ERROR, WARNING, and INFO logs,
 // including a stack trace of all running goroutines, then calls os.Exit(255).
 // Arguments are handled in the manner of fmt.Printf; a newline is appended if missing.
 func Fatalf(format string, args ...interface{}) {
-	logging.printf(fatalLog, logging.logr, format, args...)
+	logging.printf(fatalLog, format, args...)
 }
 
 // fatalNoStacks is non-zero if we are to exit without dumping goroutine stacks.
@@ -1350,25 +1284,25 @@ var fatalNoStacks uint32
 // Arguments are handled in the manner of fmt.Print; a newline is appended if missing.
 func Exit(args ...interface{}) {
 	atomic.StoreUint32(&fatalNoStacks, 1)
-	logging.print(fatalLog, logging.logr, args...)
+	logging.print(fatalLog, args...)
 }
 
 // ExitDepth acts as Exit but uses depth to determine which call frame to log.
 // ExitDepth(0, "msg") is the same as Exit("msg").
 func ExitDepth(depth int, args ...interface{}) {
 	atomic.StoreUint32(&fatalNoStacks, 1)
-	logging.printDepth(fatalLog, logging.logr, depth, args...)
+	logging.printDepth(fatalLog, depth, args...)
 }
 
 // Exitln logs to the FATAL, ERROR, WARNING, and INFO logs, then calls os.Exit(1).
 func Exitln(args ...interface{}) {
 	atomic.StoreUint32(&fatalNoStacks, 1)
-	logging.println(fatalLog, logging.logr, args...)
+	logging.println(fatalLog, args...)
 }
 
 // Exitf logs to the FATAL, ERROR, WARNING, and INFO logs, then calls os.Exit(1).
 // Arguments are handled in the manner of fmt.Printf; a newline is appended if missing.
 func Exitf(format string, args ...interface{}) {
 	atomic.StoreUint32(&fatalNoStacks, 1)
-	logging.printf(fatalLog, logging.logr, format, args...)
+	logging.printf(fatalLog, format, args...)
 }
diff --git a/klogr/klogr.go b/klogr/klogr.go
index 3ca0030..e145950 100644
--- a/klogr/klogr.go
+++ b/klogr/klogr.go
@@ -149,7 +149,7 @@ func (l klogger) Info(msg string, kvList ...interface{}) {
 }
 
 func (l klogger) Enabled() bool {
-	return bool(klog.V(klog.Level(l.level)).Enabled())
+	return bool(klog.V(klog.Level(l.level)))
 }
 
 func (l klogger) Error(err error, msg string, kvList ...interface{}) {


Test patch:
diff --git a/klog_test.go b/klog_test.go
index 7f444dc..76a780c 100644
--- a/klog_test.go
+++ b/klog_test.go
@@ -286,13 +286,13 @@ func TestVmoduleOn(t *testing.T) {
 	defer logging.swap(logging.newBuffers())
 	logging.vmodule.Set("klog_test=2")
 	defer logging.vmodule.Set("")
-	if !V(1).Enabled() {
+	if !V(1) {
 		t.Error("V not enabled for 1")
 	}
-	if !V(2).Enabled() {
+	if !V(2) {
 		t.Error("V not enabled for 2")
 	}
-	if V(3).Enabled() {
+	if V(3) {
 		t.Error("V enabled for 3")
 	}
 	V(2).Info("test")
@@ -311,7 +311,7 @@ func TestVmoduleOff(t *testing.T) {
 	logging.vmodule.Set("notthisfile=2")
 	defer logging.vmodule.Set("")
 	for i := 1; i <= 3; i++ {
-		if V(Level(i)).Enabled() {
+		if V(Level(i)) {
 			t.Errorf("V enabled for %d", i)
 		}
 	}
@@ -375,7 +375,7 @@ func testVmoduleGlob(pat string, match bool, t *testing.T) {
 	defer logging.swap(logging.newBuffers())
 	defer logging.vmodule.Set("")
 	logging.vmodule.Set(pat)
-	if V(2).Enabled() != match {
+	if V(2) != Verbose(match) {
 		t.Errorf("incorrect match for %q: got %t expected %t", pat, V(2), match)
 	}
 }