address PR feedback

log.go

@@ -40,14 +40,12 @@ func Logger(system string) *ZapEventLogger {
 		system = "undefined"
 	}
 
-	logger := getLogger(system)
-
-	return &ZapEventLogger{system: system, SugaredLogger: *logger}
+	return getLogger(system)
 }
 
 // ZapEventLogger implements the EventLogger and wraps a zap Sugared Logger.
 type ZapEventLogger struct {
-	zap.SugaredLogger
+	*zap.SugaredLogger
 	system string
 }
 
@@ -56,8 +54,13 @@ type ZapEventLogger struct {
 // that are provided to the logger via SetFieldsOnAllLoggers and these
 // only last for the life time of this particular ZapEventLogger instance.
 // Note: the fields will be passed to any children loggers of this logger.
-func (zel *ZapEventLogger) SetFieldsOnLogger(name string, args ...interface{}) {
-	zel.SugaredLogger = *zel.With(args...)
+func (zel *ZapEventLogger) SetFieldsOnLogger(args ...interface{}) {
+	loggerMutex.Lock()
+	defer loggerMutex.Unlock()
+
+	newSugaredLogger := zel.With(args...)
+	loggers[zel.system].SugaredLogger = newSugaredLogger
+	//zel.SugaredLogger = newSugaredLogger
 }
 
 // FormatRFC3339 returns the given time in UTC with RFC3999Nano format.

log_test.go

@@ -16,12 +16,11 @@ func clearGoLogEnvVars() {
 	os.Unsetenv(envLoggingFmt)
 	os.Unsetenv(envLoggingCfg)
 	os.Unsetenv(envLoggingFile)
-	os.Unsetenv(envTracingFile)
 }
 
 func TestSetupLogging(t *testing.T) {
 	t.Run("default logger configuration", func(t *testing.T) {
-		defer Cleanup()
+		defer cleanup()
 		defer clearGoLogEnvVars()
 		clearGoLogEnvVars()
 		SetupLogging()
@@ -68,7 +67,7 @@ func TestSetupLogging(t *testing.T) {
 	})
 
 	t.Run("env var logger configuration", func(t *testing.T) {
-		defer Cleanup()
+		defer cleanup()
 		defer clearGoLogEnvVars()
 		clearGoLogEnvVars()
 		wantLvl := "info"
@@ -122,7 +121,7 @@ func TestSetupLogging(t *testing.T) {
 	})
 
 	t.Run("zap json config logger configuration", func(t *testing.T) {
-		defer Cleanup()
+		defer cleanup()
 		defer clearGoLogEnvVars()
 		clearGoLogEnvVars()
 		wantLvl := "debug"
@@ -171,7 +170,7 @@ func TestSetupLogging(t *testing.T) {
 	})
 
 	t.Run("zap json config with env var override", func(t *testing.T) {
-		defer Cleanup()
+		defer cleanup()
 		defer clearGoLogEnvVars()
 		clearGoLogEnvVars()
 		wantLvl := "error"

logexample_test.go

-package log_test
+package log
 
 import (
 	"os"
-
-	logging "github.com/ipfs/go-log/v2"
 )
 
 func ExampleLogger() {
-	logging.Cleanup()
+	cleanup()
 	os.Setenv("GOLOG_LOG_CONFIG", "example_config.json")
-	logging.SetupLogging()
-	defer logging.Cleanup()
+	SetupLogging()
+	defer cleanup()
 
-	log := logging.Logger("parent")
+	log := Logger("parent")
 	log.Info("test log from parent")
 
 	// Output:
@@ -20,14 +18,14 @@ func ExampleLogger() {
 }
 
 func ExampleZapEventLogger_SetFieldsOnLogger() {
-	logging.Cleanup()
+	cleanup()
 	os.Setenv("GOLOG_LOG_CONFIG", "example_config.json")
-	logging.SetupLogging()
-	defer logging.Cleanup()
+	SetupLogging()
+	defer cleanup()
 
-	log := logging.Logger("parent")
+	log := Logger("parent")
 	log.Info("test log from parent without hostname")
-	log.SetFieldsOnLogger("parent", "hostname", "host-1")
+	log.SetFieldsOnLogger("hostname", "host-1")
 	log.Info("test log from parent")
 
 	childlog := log.Named("child")
@@ -39,18 +37,21 @@ func ExampleZapEventLogger_SetFieldsOnLogger() {
 }
 
 func ExampleSetFieldsOnAllLoggers() {
-	logging.Cleanup()
+	cleanup()
 	os.Setenv("GOLOG_LOG_CONFIG", "example_config.json")
-	logging.SetupLogging()
-	defer logging.Cleanup()
+	SetupLogging()
+	defer cleanup()
 
-	logging.SetFieldsOnAllLoggers("hostname", "host-1")
+	log1 := Logger("sys1")
+	log2 := Logger("sys2")
 
-	log := logging.Logger("parent")
-	log.Info("test log from parent")
-	childlog := log.Named("child")
-	childlog.Info("test log from child")
+	SetFieldsOnAllLoggers("hostname", "host-1", "other", "fields")
+	// Any further calls to SetFieldsOnAllLoggers will be ignored
+	SetFieldsOnAllLoggers("ignored", "true")
+
+	log1.Info("test log from sys1")
+	log2.Info("test log from sys2")
 	// Output:
-	// {"level":"info","logger":"parent","message":"test log from parent","hostname":"host-1"}
-	// {"level":"info","logger":"parent.child","message":"test log from child","hostname":"host-1"}
+	// {"level":"info","logger":"sys1","message":"test log from sys1","hostname":"host-1","other":"fields"}
+	// {"level":"info","logger":"sys2","message":"test log from sys2","hostname":"host-1","other":"fields"}
 }

setup.go

@@ -31,9 +31,6 @@ const (
 	envLoggingFmt  = "GOLOG_LOG_FMT"
 	envLoggingCfg  = "GOLOG_LOG_CONFIG" // /path/to/file
 	envLoggingFile = "GOLOG_FILE"       // /path/to/file
-
-	// envTracingFile is deprecated.
-	envTracingFile = "GOLOG_TRACING_FILE" //nolint: unused
 )
 
 // ErrNoSuchLogger is returned when the util pkg is asked for a non existant logger
@@ -41,12 +38,9 @@ var ErrNoSuchLogger = errors.New("Error: No such logger")
 
 // loggers is the set of loggers in the system
 var loggerMutex sync.RWMutex
-var loggers = make(map[string]*zap.SugaredLogger)
+var loggers = make(map[string]*ZapEventLogger)
 var levels = make(map[string]zap.AtomicLevel)
-
-// fields contains key/value pairs that will be added to all
-// loggers.
-var fields = make([]interface{}, 0)
+var fields = make(map[string]interface{})
 
 var zapCfg = zap.NewProductionConfig()
 
@@ -124,19 +118,40 @@ func setupLogging(jsonCfg []byte) {
 	}
 }
 
-// SetFieldOnAllLoggers adds the provided key value args as fields to
+var setFieldsOnce sync.Once
+
+// SetFieldsOnAllLoggers adds the provided key value args as fields to
 // all loggers.
-// Must be called before Logger, i.e. in an init() function.
+// SetFieldsOnAllLoggers can only be called ONCE, and it should be by the main
+// application and not by any libraries.
+// Should be called in an init() function, i.e. before any libraries start logging.
 // SetFieldOnAllLoggers will panic if the length of args is not even.
 func SetFieldsOnAllLoggers(args ...interface{}) {
-	if len(args)%2 != 0 {
-		panic(fmt.Errorf("SetFieldOnAllLoggers: length of args must be an even number as it represents key/value pairs: len %d", len(args)))
-	}
+	setFieldsOnce.Do(func() {
+		if len(args)%2 != 0 {
+			panic(fmt.Errorf("SetFieldOnAllLoggers: length of args must be an even number: len %d", len(args)))
+		}
 
-	loggerMutex.Lock()
-	defer loggerMutex.Unlock()
+		loggerMutex.Lock()
+		defer loggerMutex.Unlock()
+
+		// deduplicate field names in case SetFieldOnAllLoggers
+		// is called more than once
+		for i := 0; i <= len(args)/2; i += 2 {
+			fields[args[i].(string)] = args[i+1]
+		}
+		var fs []interface{}
+
+		// flatten map to array
+		for n, v := range fields {
+			fs = append(fs, n, v)
+		}
+
+		for name, l := range loggers {
+			loggers[name].SugaredLogger = l.With(fs...)
+		}
 
-	fields = append(fields, args...)
+	})
 }
 
 // SetDebugLogging calls SetAllLoggers with logging.DEBUG
@@ -217,30 +232,33 @@ func GetSubsystems() []string {
 	return subs
 }
 
-func getLogger(name string) *zap.SugaredLogger {
+func getLogger(name string) *ZapEventLogger {
 	loggerMutex.Lock()
 	defer loggerMutex.Unlock()
-	log, ok := loggers[name]
-	if !ok {
-		levels[name] = zap.NewAtomicLevelAt(zapCfg.Level.Level())
-		zapCfg.Level = levels[name]
-		newlog, err := zapCfg.Build()
-		if err != nil {
-			panic(err)
-		}
-		log = newlog.Named(name).Sugar().With(fields...)
-		loggers[name] = log
+	if l, ok := loggers[name]; ok {
+		return l
 	}
 
-	return log
+	levels[name] = zap.NewAtomicLevelAt(zapCfg.Level.Level())
+	zapCfg.Level = levels[name]
+	newlog, err := zapCfg.Build()
+	if err != nil {
+		panic(err)
+	}
+	log := newlog.Named(name).Sugar()
+
+	l := &ZapEventLogger{system: name, SugaredLogger: log}
+	loggers[name] = l
+
+	return l
 }
 
-// Cleanup is for testing purposes only.
-// Cleanup resets the package state.
-func Cleanup() {
+// cleanup is for testing purposes only.
+// cleanup resets the package state.
+func cleanup() {
 	loggerMutex.Lock()
 	defer loggerMutex.Unlock()
-	loggers = make(map[string]*zap.SugaredLogger)
+	loggers = make(map[string]*ZapEventLogger)
 	levels = make(map[string]zap.AtomicLevel)
-	fields = make([]interface{}, 0)
+	fields = make(map[string]interface{})
 }