Skip to content

Commit

Permalink
refactor: Remove extra log levels (sourcenetwork#634)
Browse files Browse the repository at this point in the history 
Relevant issue(s)
Resolves sourcenetwork#612

Description
This PR removes Warn, FeedbackWarn and FeedbackDebug. It was initially going to remove Fatal and FatalE as well but other out of scope changes need to be done prior to considering this.

Keeping just this minor reduction for now and will revisit for v0.3.1.
  • Loading branch information
@fredcarle
fredcarle authored Jul 14, 2022
1 parent eed35ae commit c3405b9
Show file tree
Hide file tree
Showing 9 changed files with 27 additions and 76 deletions.
2 changes: 1 addition & 1 deletion cli/root.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ func init() {

rootCmd.PersistentFlags().String(
"loglevel", cfg.Logging.Level,
"log level to use. Options are debug, info, warn, error, fatal",
"log level to use. Options are debug, info, error, fatal",
)
err = viper.BindPFlag("logging.level", rootCmd.PersistentFlags().Lookup("loglevel"))
if err != nil {
Expand Down
2 changes: 0 additions & 2 deletions config/config.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,6 @@ func (cfg *Config) GetLoggingConfig() (logging.Config, error) {
loglvl = logging.Debug
case "info":
loglvl = logging.Info
case "warn":
loglvl = logging.Warn
case "error":
loglvl = logging.Error
case "fatal":
Expand Down
4 changes: 2 additions & 2 deletions config/config_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ var envVarsDifferentThanDefault = map[string]string{
"DEFRA_NET_RPCTIMEOUT": "90s",
"DEFRA_NET_PUBSUB": "false",
"DEFRA_NET_RELAY": "false",
"DEFRA_LOGGING_LEVEL": "warn",
"DEFRA_LOGGING_LEVEL": "info",
"DEFRA_LOGGING_STACKTRACE": "false",
"DEFRA_LOGGING_FORMAT": "json",
}
Expand Down Expand Up @@ -147,7 +147,7 @@ func TestEnvVariablesAllConsidered(t *testing.T) {
assert.Equal(t, "90s", cfg.Net.RPCTimeout)
assert.Equal(t, false, cfg.Net.PubSubEnabled)
assert.Equal(t, false, cfg.Net.RelayEnabled)
assert.Equal(t, "warn", cfg.Logging.Level)
assert.Equal(t, "info", cfg.Logging.Level)
assert.Equal(t, false, cfg.Logging.Stacktrace)
assert.Equal(t, "json", cfg.Logging.Format)
}
Expand Down
2 changes: 1 addition & 1 deletion config/configfile.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ net:
RPCMaxConnectionIdle: {{ .Net.RPCMaxConnectionIdle }}
logging:
# Log level. Options are debug, info, warn, error, fatal
# Log level. Options are debug, info, error, fatal
level: {{ .Logging.Level }}
# Include stacktrace in error and fatal logs
stacktrace: {{ .Logging.Stacktrace }}
Expand Down
21 changes: 0 additions & 21 deletions logging/logger.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -55,13 +55,6 @@ func (l *logger) Info(ctx context.Context, message string, keyvals ...KV) {
l.logger.Info(message, toZapFields(keyvals)...)
}

func (l *logger) Warn(ctx context.Context, message string, keyvals ...KV) {
l.syncLock.RLock()
defer l.syncLock.RUnlock()

l.logger.Warn(message, toZapFields(keyvals)...)
}

func (l *logger) Error(ctx context.Context, message string, keyvals ...KV) {
l.syncLock.RLock()
defer l.syncLock.RUnlock()
Expand Down Expand Up @@ -96,27 +89,13 @@ func (l *logger) FatalE(ctx context.Context, message string, err error, keyvals
l.logger.Fatal(message, toZapFields(kvs)...)
}

func (l *logger) FeedbackDebug(ctx context.Context, message string, keyvals ...KV) {
l.Debug(ctx, message, keyvals...)
if l.consoleLogger != nil {
l.consoleLogger.Println(message)
}
}

func (l *logger) FeedbackInfo(ctx context.Context, message string, keyvals ...KV) {
l.Info(ctx, message, keyvals...)
if l.consoleLogger != nil {
l.consoleLogger.Println(message)
}
}

func (l *logger) FeedbackWarn(ctx context.Context, message string, keyvals ...KV) {
l.Warn(ctx, message, keyvals...)
if l.consoleLogger != nil {
l.consoleLogger.Println(message)
}
}

func (l *logger) FeedbackError(ctx context.Context, message string, keyvals ...KV) {
l.Error(ctx, message, keyvals...)
if l.consoleLogger != nil {
Expand Down
10 changes: 4 additions & 6 deletions logging/logging.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ type Logger interface {
Debug(ctx context.Context, message string, keyvals ...KV)
// Info logs a message at info log level. Key-value pairs can be added.
Info(ctx context.Context, message string, keyvals ...KV)
// Warn logs a message at warn log level. Key-value pairs can be added.
Warn(ctx context.Context, message string, keyvals ...KV)
// Error logs a message at error log level. Key-value pairs can be added.
Error(ctx context.Context, message string, keyvals ...KV)
// ErrorErr logs a message and an error at error log level. Key-value pairs can be added.
Expand All @@ -45,12 +43,11 @@ type Logger interface {
Fatal(ctx context.Context, message string, keyvals ...KV)
// FatalE logs a message and an error at fatal log level. Key-value pairs can be added.
FatalE(ctx context.Context, message string, err error, keyvals ...KV)
// FeedbackDebug calls Debug and sends the message to stderr if logs are sent to a file.
FeedbackDebug(ctx context.Context, message string, keyvals ...KV)

// Feedback prefixed method ensure that messsages reach a user in case the logs are sent to a file.

// FeedbackInfo calls Info and sends the message to stderr if logs are sent to a file.
FeedbackInfo(ctx context.Context, message string, keyvals ...KV)
// FeedbackWarn calls Warn and sends the message to stderr if logs are sent to a file.
FeedbackWarn(ctx context.Context, message string, keyvals ...KV)
// FeedbackError calls Error and sends the message to stderr if logs are sent to a file.
FeedbackError(ctx context.Context, message string, keyvals ...KV)
// FeedbackErrorE calls ErrorE and sends the message to stderr if logs are sent to a file.
Expand All @@ -59,6 +56,7 @@ type Logger interface {
FeedbackFatal(ctx context.Context, message string, keyvals ...KV)
// FeedbackFatalE calls FatalE and sends the message to stderr if logs are sent to a file.
FeedbackFatalE(ctx context.Context, message string, err error, keyvals ...KV)

// Flush flushes any buffered log entries.
Flush() error
// ApplyConfig updates the logger with a new config.
Expand Down
50 changes: 14 additions & 36 deletions logging/logging_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -200,7 +200,6 @@ type LogLevelTestCase struct {

func logDebug(l Logger, c context.Context, m string) { l.Debug(c, m) }
func logInfo(l Logger, c context.Context, m string) { l.Info(c, m) }
func logWarn(l Logger, c context.Context, m string) { l.Warn(c, m) }
func logError(l Logger, c context.Context, m string) { l.Error(c, m) }
func logErrorE(l Logger, c context.Context, m string) { l.ErrorE(c, m, fmt.Errorf("test error")) }

Expand All @@ -209,30 +208,25 @@ func getLogLevelTestCase() []LogLevelTestCase {
{Debug, logDebug, "DEBUG", false, false, true},
{Debug, logDebug, "DEBUG", false, false, false},
{Debug, logInfo, "INFO", false, false, false},
{Debug, logWarn, "WARN", false, false, false},
{Debug, logError, "ERROR", false, false, false},
{Debug, logError, "ERROR", true, true, false},
{Debug, logErrorE, "ERROR", false, false, false},
{Debug, logErrorE, "ERROR", true, true, false},
{Info, logDebug, "", false, false, false},
{Info, logInfo, "INFO", false, false, true},
{Info, logInfo, "INFO", false, false, false},
{Info, logWarn, "WARN", false, false, false},
{Info, logError, "ERROR", false, false, false},
{Info, logError, "ERROR", true, true, false},
{Info, logErrorE, "ERROR", false, false, false},
{Info, logErrorE, "ERROR", true, true, false},
{Warn, logDebug, "", false, false, false},
{Warn, logInfo, "", false, false, false},
{Warn, logWarn, "WARN", false, false, true},
{Warn, logWarn, "WARN", false, false, false},
{Warn, logError, "ERROR", false, false, false},
{Warn, logError, "ERROR", true, true, false},
{Warn, logErrorE, "ERROR", false, false, false},
{Warn, logErrorE, "ERROR", true, true, false},
{Error, logDebug, "", false, false, false},
{Error, logInfo, "", false, false, false},
{Error, logWarn, "", false, false, false},
{Error, logError, "ERROR", false, false, true},
{Error, logError, "ERROR", false, false, false},
{Error, logError, "ERROR", true, true, false},
Expand All @@ -241,7 +235,6 @@ func getLogLevelTestCase() []LogLevelTestCase {
{Fatal, logDebug, "", false, false, true},
{Fatal, logDebug, "", false, false, false},
{Fatal, logInfo, "", false, false, false},
{Fatal, logWarn, "", false, false, false},
{Fatal, logError, "", false, false, false},
{Fatal, logErrorE, "", false, false, false},
}
Expand Down Expand Up @@ -528,52 +521,37 @@ func TestLogWritesMessagesToLogGivenUpdatedLogPath(t *testing.T) {
}
}

func logFeedbackDebug(l Logger, c context.Context, m string) { l.FeedbackDebug(c, m) }
func logFeedbackInfo(l Logger, c context.Context, m string) { l.FeedbackInfo(c, m) }
func logFeedbackWarn(l Logger, c context.Context, m string) { l.FeedbackWarn(c, m) }
func logFeedbackError(l Logger, c context.Context, m string) { l.FeedbackError(c, m) }
func logFeedbackErrorE(l Logger, c context.Context, m string) {
l.FeedbackErrorE(c, m, fmt.Errorf("test error"))
}

func getFeedbackLogLevelTestCase() []LogLevelTestCase {
return []LogLevelTestCase{
{Debug, logFeedbackDebug, "DEBUG", false, false, true},
{Debug, logFeedbackDebug, "DEBUG", false, false, false},
{Debug, logFeedbackInfo, "INFO", false, false, false},
{Debug, logFeedbackWarn, "WARN", false, false, false},
{Debug, logFeedbackError, "ERROR", false, false, false},
{Debug, logFeedbackError, "ERROR", true, true, false},
{Debug, logFeedbackErrorE, "ERROR", false, false, false},
{Debug, logFeedbackErrorE, "ERROR", true, true, false},
{Info, logFeedbackDebug, "", false, false, false},
{Info, logFeedbackInfo, "INFO", false, false, true},
{Info, logFeedbackInfo, "INFO", false, false, false},
{Info, logFeedbackWarn, "WARN", false, false, false},
{Info, logFeedbackError, "ERROR", false, false, false},
{Info, logFeedbackError, "ERROR", true, true, false},
{Info, logFeedbackErrorE, "ERROR", false, false, false},
{Info, logFeedbackErrorE, "ERROR", true, true, false},
{Warn, logFeedbackDebug, "", false, false, false},
{Warn, logFeedbackInfo, "", false, false, false},
{Warn, logFeedbackWarn, "WARN", false, false, true},
{Warn, logFeedbackWarn, "WARN", false, false, false},
{Warn, logFeedbackError, "ERROR", false, false, false},
{Warn, logFeedbackError, "ERROR", true, true, false},
{Warn, logFeedbackErrorE, "ERROR", false, false, false},
{Warn, logFeedbackErrorE, "ERROR", true, true, false},
{Error, logFeedbackDebug, "", false, false, false},
{Error, logFeedbackInfo, "", false, false, false},
{Error, logFeedbackWarn, "", false, false, false},
{Error, logFeedbackError, "ERROR", false, false, true},
{Error, logFeedbackError, "ERROR", false, false, false},
{Error, logFeedbackError, "ERROR", true, true, false},
{Error, logFeedbackErrorE, "ERROR", false, false, false},
{Error, logFeedbackErrorE, "ERROR", true, true, false},
{Fatal, logFeedbackDebug, "", false, false, true},
{Fatal, logFeedbackDebug, "", false, false, false},
{Fatal, logFeedbackInfo, "", false, false, false},
{Fatal, logFeedbackWarn, "", false, false, false},
{Fatal, logFeedbackError, "", false, false, false},
{Fatal, logFeedbackErrorE, "", false, false, false},
}
Expand Down Expand Up @@ -634,7 +612,7 @@ func TestLogWritesMessagesToLogGivenPipeWithValidPath(t *testing.T) {
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -647,7 +625,7 @@ func TestLogWritesMessagesToLogGivenPipeWithValidPath(t *testing.T) {
}

assert.Equal(t, logMessage, logLines[0]["msg"])
assert.Equal(t, "WARN", logLines[0]["level"])
assert.Equal(t, "INFO", logLines[0]["level"])
assert.Equal(t, "TestLogName", logLines[0]["logger"])
// caller is disabled by default
assert.NotContains(t, logLines[0], "logging_test.go")
Expand All @@ -665,7 +643,7 @@ func TestLogDoesNotWriteMessagesToLogGivenOverrideForAnotherLoggerReducingLogLev
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -688,7 +666,7 @@ func TestLogWritesMessagesToLogGivenOverrideForLoggerReducingLogLevel(t *testing
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -701,7 +679,7 @@ func TestLogWritesMessagesToLogGivenOverrideForLoggerReducingLogLevel(t *testing
}

assert.Equal(t, logMessage, logLines[0]["msg"])
assert.Equal(t, "WARN", logLines[0]["level"])
assert.Equal(t, "INFO", logLines[0]["level"])
assert.Equal(t, "TestLogName", logLines[0]["logger"])
// caller is disabled by default
assert.NotContains(t, logLines[0], "logging_test.go")
Expand All @@ -719,7 +697,7 @@ func TestLogWritesMessagesToLogGivenOverrideForLoggerRaisingLogLevel(t *testing.
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -732,7 +710,7 @@ func TestLogWritesMessagesToLogGivenOverrideForLoggerRaisingLogLevel(t *testing.
}

assert.Equal(t, logMessage, logLines[0]["msg"])
assert.Equal(t, "WARN", logLines[0]["level"])
assert.Equal(t, "INFO", logLines[0]["level"])
assert.Equal(t, "TestLogName", logLines[0]["logger"])
// caller is disabled by default
assert.NotContains(t, logLines[0], "logging_test.go")
Expand All @@ -750,7 +728,7 @@ func TestLogDoesNotWriteMessagesToLogGivenOverrideForLoggerRaisingLogLevel(t *te
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -775,7 +753,7 @@ func TestLogDoesNotWriteMessagesToLogGivenOverrideUpdatedForAnotherLoggerReducin
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -800,7 +778,7 @@ func TestLogWritesMessagesToLogGivenOverrideUpdatedForLoggerReducingLogLevel(t *
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -813,7 +791,7 @@ func TestLogWritesMessagesToLogGivenOverrideUpdatedForLoggerReducingLogLevel(t *
}

assert.Equal(t, logMessage, logLines[0]["msg"])
assert.Equal(t, "WARN", logLines[0]["level"])
assert.Equal(t, "INFO", logLines[0]["level"])
assert.Equal(t, "TestLogName", logLines[0]["logger"])
// caller is disabled by default
assert.NotContains(t, logLines[0], "logging_test.go")
Expand All @@ -833,7 +811,7 @@ func TestLogWritesMessagesToLogGivenOverrideUpdatedForAnotherLoggerRaisingLogLev
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand All @@ -846,7 +824,7 @@ func TestLogWritesMessagesToLogGivenOverrideUpdatedForAnotherLoggerRaisingLogLev
}

assert.Equal(t, logMessage, logLines[0]["msg"])
assert.Equal(t, "WARN", logLines[0]["level"])
assert.Equal(t, "INFO", logLines[0]["level"])
assert.Equal(t, "TestLogName", logLines[0]["logger"])
// caller is disabled by default
assert.NotContains(t, logLines[0], "logging_test.go")
Expand All @@ -866,7 +844,7 @@ func TestLogDoesNotWriteMessagesToLogGivenOverrideUpdatedForLoggerRaisingLogLeve
})
logMessage := "test log message"

logger.Warn(ctx, logMessage)
logger.Info(ctx, logMessage)
logger.Flush()

logLines, err := getLogLines(t, logPath)
Expand Down
6 changes: 3 additions & 3 deletions net/peer.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func (p *Peer) Close() error {
// from the internal broadcaster to the external pubsub network
func (p *Peer) handleBroadcastLoop() {
if p.bus == nil {
log.Warn(p.ctx, "Tried to start internal broadcaster with none defined")
log.Info(p.ctx, "Tried to start internal broadcaster with none defined")
return
}

Expand All @@ -187,7 +187,7 @@ func (p *Peer) handleBroadcastLoop() {
} else if msg.Priority > 1 {
err = p.handleDocUpdateLog(msg)
} else {
log.Warn(p.ctx, "Skipping log with invalid priority of 0", logging.NewKV("CID", msg.Cid))
log.Info(p.ctx, "Skipping log with invalid priority of 0", logging.NewKV("CID", msg.Cid))
}

if err != nil {
Expand Down Expand Up @@ -462,7 +462,7 @@ func stopGRPCServer(ctx context.Context, server *grpc.Server) {
select {
case <-timer.C:
server.Stop()
log.Warn(ctx, "Peer gRPC server was shutdown ungracefully")
log.Info(ctx, "Peer gRPC server was shutdown ungracefully")
case <-stopped:
timer.Stop()
}
Expand Down
6 changes: 2 additions & 4 deletions tests/integration/utils.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,8 +294,7 @@ func ExecuteQueryTestCase(
assert.NotEmpty(t, dbs)

for _, dbi := range dbs {
// We log with level warn to highlight this item
log.Warn(ctx, test.Description, logging.NewKV("Database", dbi.name))
log.Info(ctx, test.Description, logging.NewKV("Database", dbi.name))

if detectDbChanges {
if setupOnly {
Expand Down Expand Up @@ -639,8 +638,7 @@ func assertQueryResults(
}
resultantData := result.Data.([]map[string]interface{})

// We log with level warn to highlight this item
log.Warn(ctx, "", logging.NewKV("QueryResults", result.Data))
log.Info(ctx, "", logging.NewKV("QueryResults", result.Data))

// compare results
assert.Equal(t, len(expectedResults), len(resultantData), description)
Expand Down

0 comments on commit c3405b9

Please sign in to comment.