terminal/logflags: Added SetLoggerFactory(LoggerFactory) (#3257)

* terminal/logflags: Added `SetLoggerFactory(LoggerFactory)`

This change will enable people who want to embed Delve into their applications to adjust the logging better to their needs.

* terminal/logflags: Added `SetLoggerFactory(LoggerFactory)`

Added changes from code review.

* terminal/logflags: Added `SetLoggerFactory(LoggerFactory)`

Reworked requested changes.

* terminal/logflags: Added `SetLoggerFactory(LoggerFactory)`

Reworked requested changes.

pkg/logflags/logflags.go

@@ -29,17 +29,20 @@ var minidump = false
 
 var logOut io.WriteCloser
 
-func makeLogger(flag bool, fields logrus.Fields) *logrus.Entry {
-	logger := logrus.New().WithFields(fields)
-	logger.Logger.Formatter = &textFormatter{}
+func makeLogger(flag bool, fields Fields) Logger {
+	if lf := loggerFactory; lf != nil {
+		return lf(flag, fields, logOut)
+	}
+	logger := logrus.New().WithFields(logrus.Fields(fields))
+	logger.Logger.Formatter = DefaultFormatter()
 	if logOut != nil {
 		logger.Logger.Out = logOut
 	}
-	logger.Logger.Level = logrus.DebugLevel
-	if !flag {
-		logger.Logger.Level = logrus.ErrorLevel
+	logger.Logger.Level = logrus.ErrorLevel
+	if flag {
+		logger.Logger.Level = logrus.DebugLevel
 	}
-	return logger
+	return &logrusLogger{logger}
 }
 
 // Any returns true if any logging is enabled.
@@ -54,8 +57,8 @@ func GdbWire() bool {
 }
 
 // GdbWireLogger returns a configured logger for the gdbserial wire protocol.
-func GdbWireLogger() *logrus.Entry {
-	return makeLogger(gdbWire, logrus.Fields{"layer": "gdbconn"})
+func GdbWireLogger() Logger {
+	return makeLogger(gdbWire, Fields{"layer": "gdbconn"})
 }
 
 // Debugger returns true if the debugger package should log.
@@ -64,8 +67,8 @@ func Debugger() bool {
 }
 
 // DebuggerLogger returns a logger for the debugger package.
-func DebuggerLogger() *logrus.Entry {
-	return makeLogger(debugger, logrus.Fields{"layer": "debugger"})
+func DebuggerLogger() Logger {
+	return makeLogger(debugger, Fields{"layer": "debugger"})
 }
 
 // LLDBServerOutput returns true if the output of the LLDB server should be
@@ -80,14 +83,24 @@ func DebugLineErrors() bool {
 	return debugLineErrors
 }
 
+// DebugLineLogger returns a logger for the dwarf/line package.
+func DebugLineLogger() Logger {
+	return makeLogger(debugLineErrors, Fields{"layer": "dwarf-line"})
+}
+
 // RPC returns true if RPC messages should be logged.
 func RPC() bool {
 	return rpc
 }
 
 // RPCLogger returns a logger for RPC messages.
-func RPCLogger() *logrus.Entry {
-	return makeLogger(rpc, logrus.Fields{"layer": "rpc"})
+func RPCLogger() Logger {
+	return rpcLogger(rpc)
+}
+
+// rpcLogger returns a logger for RPC messages set to a specific minimal log level.
+func rpcLogger(flag bool) Logger {
+	return makeLogger(flag, Fields{"layer": "rpc"})
 }
 
 // DAP returns true if dap package should log.
@@ -96,8 +109,8 @@ func DAP() bool {
 }
 
 // DAPLogger returns a logger for dap package.
-func DAPLogger() *logrus.Entry {
-	return makeLogger(dap, logrus.Fields{"layer": "dap"})
+func DAPLogger() Logger {
+	return makeLogger(dap, Fields{"layer": "dap"})
 }
 
 // FnCall returns true if the function call protocol should be logged.
@@ -105,8 +118,8 @@ func FnCall() bool {
 	return fnCall
 }
 
-func FnCallLogger() *logrus.Entry {
-	return makeLogger(fnCall, logrus.Fields{"layer": "proc", "kind": "fncall"})
+func FnCallLogger() Logger {
+	return makeLogger(fnCall, Fields{"layer": "proc", "kind": "fncall"})
 }
 
 // Minidump returns true if the minidump loader should be logged.
@@ -114,8 +127,8 @@ func Minidump() bool {
 	return minidump
 }
 
-func MinidumpLogger() *logrus.Entry {
-	return makeLogger(minidump, logrus.Fields{"layer": "core", "kind": "minidump"})
+func MinidumpLogger() Logger {
+	return makeLogger(minidump, Fields{"layer": "core", "kind": "minidump"})
 }
 
 // WriteDAPListeningMessage writes the "DAP server listening" message in dap mode.
@@ -139,8 +152,7 @@ func writeListeningMessage(server string, addr net.Addr) {
 	if tcpAddr == nil || tcpAddr.IP.IsLoopback() {
 		return
 	}
-	logger := RPCLogger()
-	logger.Logger.Level = logrus.WarnLevel
+	logger := rpcLogger(true)
 	logger.Warnln("Listening for remote connections (connections are not authenticated nor encrypted)")
 }
 
@@ -217,12 +229,17 @@ func Close() {
 	}
 }
 
-// textFormatter is a simplified version of logrus.TextFormatter that
+// DefaultFormatter provides a simplified version of logrus.TextFormatter that
 // doesn't make logs unreadable when they are output to a text file or to a
 // terminal that doesn't support colors.
-type textFormatter struct {
+func DefaultFormatter() logrus.Formatter {
+	return textFormatterInstance
 }
 
+type textFormatter struct{}
+
+var textFormatterInstance = &textFormatter{}
+
 func (f *textFormatter) Format(entry *logrus.Entry) ([]byte, error) {
 	var b *bytes.Buffer
 	if entry.Buffer != nil {

pkg/logflags/logflags_test.go

@@ -0,0 +1,117 @@
+package logflags
+
+import (
+	"bytes"
+	"io"
+	"reflect"
+	"testing"
+
+	"github.com/sirupsen/logrus"
+)
+
+func TestMakeLogger_usingLoggerFactory(t *testing.T) {
+	if loggerFactory != nil {
+		t.Fatalf("expected loggerFactory to be nil; but was <%v>", loggerFactory)
+	}
+	defer func() {
+		loggerFactory = nil
+	}()
+	if logOut != nil {
+		t.Fatalf("expected logOut to be nil; but was <%v>", logOut)
+	}
+	logOut = &bufferWriter{}
+	defer func() {
+		logOut = nil
+	}()
+
+	expectedLogger := &logrusLogger{}
+	SetLoggerFactory(func(flag bool, fields Fields, out io.Writer) Logger {
+		if flag != true {
+			t.Fatalf("expected flag to be <%v>; but was <%v>", true, flag)
+		}
+		if len(fields) != 1 || fields["foo"] != "bar" {
+			t.Fatalf("expected fields to be {'foo':'bar'}; but was <%v>", fields)
+		}
+		if out != logOut {
+			t.Fatalf("expected out to be <%v>; but was <%v>", logOut, out)
+		}
+		return expectedLogger
+	})
+
+	actual := makeLogger(true, Fields{"foo": "bar"})
+	if actual != expectedLogger {
+		t.Fatalf("expected actual to <%v>; but was <%v>", expectedLogger, actual)
+	}
+}
+
+func TestMakeLogger_usingDefaultBehavior(t *testing.T) {
+	if loggerFactory != nil {
+		t.Fatalf("expected loggerFactory to be nil; but was <%v>", loggerFactory)
+	}
+	if logOut != nil {
+		t.Fatalf("expected logOut to be nil; but was <%v>", logOut)
+	}
+	logOut = &bufferWriter{}
+	defer func() {
+		logOut = nil
+	}()
+
+	actual := makeLogger(false, Fields{"foo": "bar"})
+
+	actualEntry, expectedType := actual.(*logrusLogger)
+	if !expectedType {
+		t.Fatalf("expected actual to be of type <%v>; but was <%v>", reflect.TypeOf((*logrus.Entry)(nil)), reflect.TypeOf(actualEntry))
+	}
+	if actualEntry.Entry.Logger.Level != logrus.ErrorLevel {
+		t.Fatalf("expected actualEntry.Entry.Logger.Level to be <%v>; but was <%v>", logrus.ErrorLevel, actualEntry.Logger.Level)
+	}
+	if actualEntry.Entry.Logger.Out != logOut {
+		t.Fatalf("expected actualEntry.Entry.Logger.Out to be <%v>; but was <%v>", logOut, actualEntry.Logger.Out)
+	}
+	if actualEntry.Entry.Logger.Formatter != textFormatterInstance {
+		t.Fatalf("expected actualEntry.Entry.Logger.Formatter to be <%v>; but was <%v>", textFormatterInstance, actualEntry.Logger.Formatter)
+	}
+	if len(actualEntry.Entry.Data) != 1 || actualEntry.Entry.Data["foo"] != "bar" {
+		t.Fatalf("expected actualEntry.Entry.Data to be {'foo':'bar'}; but was <%v>", actualEntry.Data)
+	}
+}
+
+func TestMakeLogger_usingDefaultBehaviorAndFlagged(t *testing.T) {
+	if loggerFactory != nil {
+		t.Fatalf("expected loggerFactory to be nil; but was <%v>", loggerFactory)
+	}
+	if logOut != nil {
+		t.Fatalf("expected logOut to be nil; but was <%v>", logOut)
+	}
+	logOut = &bufferWriter{}
+	defer func() {
+		logOut = nil
+	}()
+
+	actual := makeLogger(true, Fields{"foo": "bar"})
+
+	actualEntry, expectedType := actual.(*logrusLogger)
+	if !expectedType {
+		t.Fatalf("expected actual to be of type <%v>; but was <%v>", reflect.TypeOf((*logrus.Entry)(nil)), reflect.TypeOf(actualEntry))
+	}
+	if actualEntry.Entry.Logger.Level != logrus.DebugLevel {
+		t.Fatalf("expected actualEntry.Entry.Logger.Level to be <%v>; but was <%v>", logrus.DebugLevel, actualEntry.Logger.Level)
+	}
+	if actualEntry.Entry.Logger.Out != logOut {
+		t.Fatalf("expected actualEntry.Entry.Logger.Out to be <%v>; but was <%v>", logOut, actualEntry.Logger.Out)
+	}
+	if actualEntry.Entry.Logger.Formatter != textFormatterInstance {
+		t.Fatalf("expected actualEntry.Entry.Logger.Formatter to be <%v>; but was <%v>", textFormatterInstance, actualEntry.Logger.Formatter)
+	}
+	if len(actualEntry.Entry.Data) != 1 || actualEntry.Entry.Data["foo"] != "bar" {
+		t.Fatalf("expected actualEntry.Entry.Data to be {'foo':'bar'}; but was <%v>", actualEntry.Data)
+	}
+}
+
+type bufferWriter struct {
+	bytes.Buffer
+}
+
+func (bw bufferWriter) Close() error {
+	return nil
+}

pkg/logflags/logger.go

@@ -0,0 +1,77 @@
+package logflags
+
+import (
+	"github.com/sirupsen/logrus"
+	"io"
+)
+
+// Logger represents a generic interface for logging inside of
+// Delve codebase.
+type Logger interface {
+	// WithField returns a new Logger enriched with the given field.
+	WithField(key string, value interface{}) Logger
+	// WithFields returns a new Logger enriched with the given fields.
+	WithFields(fields Fields) Logger
+	// WithError returns a new Logger enriched with the given error.
+	WithError(err error) Logger
+
+	Debugf(format string, args ...interface{})
+	Infof(format string, args ...interface{})
+	Printf(format string, args ...interface{})
+	Warnf(format string, args ...interface{})
+	Warningf(format string, args ...interface{})
+	Errorf(format string, args ...interface{})
+	Fatalf(format string, args ...interface{})
+	Panicf(format string, args ...interface{})
+
+	Debug(args ...interface{})
+	Info(args ...interface{})
+	Print(args ...interface{})
+	Warn(args ...interface{})
+	Warning(args ...interface{})
+	Error(args ...interface{})
+	Fatal(args ...interface{})
+	Panic(args ...interface{})
+
+	Debugln(args ...interface{})
+	Infoln(args ...interface{})
+	Println(args ...interface{})
+	Warnln(args ...interface{})
+	Warningln(args ...interface{})
+	Errorln(args ...interface{})
+	Fatalln(args ...interface{})
+	Panicln(args ...interface{})
+}
+
+// LoggerFactory is used to create new Logger instances.
+// SetLoggerFactory can be used to configure it.
+//
+// The given parameters fields and out can be both be nil.
+type LoggerFactory func(flag bool, fields Fields, out io.Writer) Logger
+
+var loggerFactory LoggerFactory
+
+// SetLoggerFactory will ensure that every Logger created by this package, will be now created
+// by the given LoggerFactory. Default behavior will be a logrus based Logger instance using DefaultFormatter.
+func SetLoggerFactory(lf LoggerFactory) {
+	loggerFactory = lf
+}
+
+// Fields type wraps many fields for Logger
+type Fields map[string]interface{}
+
+type logrusLogger struct {
+	*logrus.Entry
+}
+
+func (l *logrusLogger) WithField(key string, value interface{}) Logger {
+	return &logrusLogger{l.Entry.WithField(key, value)}
+}
+
+func (l *logrusLogger) WithFields(fields Fields) Logger {
+	return &logrusLogger{l.Entry.WithFields(logrus.Fields(fields))}
+}
+
+func (l *logrusLogger) WithError(err error) Logger {
+	return &logrusLogger{l.Entry.WithError(err)}
+}

pkg/proc/bininfo.go

@@ -34,7 +34,6 @@ import (
 	"github.com/go-delve/delve/pkg/logflags"
 	"github.com/go-delve/delve/pkg/proc/debuginfod"
 	"github.com/hashicorp/golang-lru/simplelru"
-	"github.com/sirupsen/logrus"
 )
 
 const (
@@ -111,7 +110,7 @@ type BinaryInfo struct {
 	// Go 1.17 register ABI is enabled.
 	regabi bool
 
-	logger *logrus.Entry
+	logger logflags.Logger
 }
 
 var (
@@ -944,7 +943,7 @@ func (image *Image) Close() error {
 	return err2
 }
 
-func (image *Image) setLoadError(logger *logrus.Entry, fmtstr string, args ...interface{}) {
+func (image *Image) setLoadError(logger logflags.Logger, fmtstr string, args ...interface{}) {
 	image.loadErrMu.Lock()
 	image.loadErr = fmt.Errorf(fmtstr, args...)
 	image.loadErrMu.Unlock()
@@ -1586,7 +1585,7 @@ func (bi *BinaryInfo) setGStructOffsetElf(image *Image, exe *elf.File, wg *sync.
 	}
 }
 
-func getSymbol(image *Image, logger *logrus.Entry, exe *elf.File, name string) *elf.Symbol {
+func getSymbol(image *Image, logger logflags.Logger, exe *elf.File, name string) *elf.Symbol {
 	symbols, err := exe.Symbols()
 	if err != nil {
 		image.setLoadError(logger, "could not parse ELF symbols: %v", err)
@@ -2069,11 +2068,7 @@ func (bi *BinaryInfo) loadDebugInfoMaps(image *Image, debugInfoBytes, debugLineB
 			if hasLineInfo && lineInfoOffset >= 0 && lineInfoOffset < int64(len(debugLineBytes)) {
 				var logfn func(string, ...interface{})
 				if logflags.DebugLineErrors() {
-					logger := logrus.New().WithFields(logrus.Fields{"layer": "dwarf-line"})
-					logger.Logger.Level = logrus.DebugLevel
-					logfn = func(fmt string, args ...interface{}) {
-						logger.Printf(fmt, args)
-					}
+					logfn = logflags.DebugLineLogger().Printf
 				}
 				cu.lineInfo = line.Parse(compdir, bytes.NewBuffer(debugLineBytes[lineInfoOffset:]), image.debugLineStr, logfn, image.StaticBase, bi.GOOS == "windows", bi.Arch.PtrSize())
 			}

pkg/proc/gdbserial/gdbserver_conn.go

@@ -17,7 +17,6 @@ import (
 
 	"github.com/go-delve/delve/pkg/logflags"
 	"github.com/go-delve/delve/pkg/proc"
-	"github.com/sirupsen/logrus"
 )
 
 type gdbConn struct {
@@ -49,7 +48,7 @@ type gdbConn struct {
 
 	useXcmd bool // forces writeMemory to use the 'X' command
 
-	log *logrus.Entry
+	log logflags.Logger
 }
 
 var ErrTooManyAttempts = errors.New("too many transmit attempts")

service/dap/server.go

@@ -43,8 +43,6 @@ import (
 	"github.com/go-delve/delve/service/debugger"
 	"github.com/go-delve/delve/service/internal/sameuser"
 	"github.com/google/go-dap"
-
-	"github.com/sirupsen/logrus"
 )
 
 // Server implements a DAP server that can accept a single client for
@@ -167,7 +165,7 @@ type Config struct {
 	*service.Config
 
 	// log is used for structured logging.
-	log *logrus.Entry
+	log logflags.Logger
 	// StopTriggered is closed when the server is Stop()-ed.
 	// Can be used to safeguard against duplicate shutdown sequences.
 	StopTriggered chan struct{}

service/debugger/debugger.go

@@ -31,7 +31,6 @@ import (
 	"github.com/go-delve/delve/pkg/proc/gdbserial"
 	"github.com/go-delve/delve/pkg/proc/native"
 	"github.com/go-delve/delve/service/api"
-	"github.com/sirupsen/logrus"
 )
 
 var (
@@ -70,7 +69,7 @@ type Debugger struct {
 	targetMutex sync.Mutex
 	target      *proc.TargetGroup
 
-	log *logrus.Entry
+	log logflags.Logger
 
 	running      bool
 	runningMutex sync.Mutex

service/rpccommon/server.go

@@ -25,7 +25,6 @@ import (
 	"github.com/go-delve/delve/service/internal/sameuser"
 	"github.com/go-delve/delve/service/rpc1"
 	"github.com/go-delve/delve/service/rpc2"
-	"github.com/sirupsen/logrus"
 )
 
 // ServerImpl implements a JSON-RPC server that can switch between two
@@ -45,7 +44,7 @@ type ServerImpl struct {
 	s2 *rpc2.RPCServer
 	// maps of served methods, one for each supported API.
 	methodMaps []map[string]*methodType
-	log        *logrus.Entry
+	log        logflags.Logger
 }
 
 type RPCCallback struct {
@@ -216,7 +215,7 @@ func isExportedOrBuiltinType(t reflect.Type) bool {
 //
 //	func (rcvr ReceiverType) Method(in InputType, out *ReplyType) error
 //	func (rcvr ReceiverType) Method(in InputType, cb service.RPCCallback)
-func suitableMethods(rcvr interface{}, methods map[string]*methodType, log *logrus.Entry) {
+func suitableMethods(rcvr interface{}, methods map[string]*methodType, log logflags.Logger) {
 	typ := reflect.TypeOf(rcvr)
 	rcvrv := reflect.ValueOf(rcvr)
 	sname := reflect.Indirect(rcvrv).Type().Name()