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.

Sign up for GitHub

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

refactor: Remove extra log levels #634

Merged
merged 2 commits into from
Jul 14, 2022
Merged

refactor: Remove extra log levels #634

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eab50ad
remove extra log levels
fredcarle Jul 13, 2022
0ccf822
remove leftover warn related code
fredcarle Jul 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions logging/logger.go
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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")
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like an Error

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

To me this is not an error. An error is something that needs to be handled by the caller. This is just a message sent back to the console/logs to let the user know that something happened. Hence it's just Information.

Copy link
Contributor

@orpheuslummis orpheuslummis Jul 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Do we want to prepend something like "Warning" on some of the Info messages? I'm towards yes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to prepend something like "Warning" on some of the Info messages? I'm towards yes.

🤣 Please dont do this lol. If someone really wants it to be a warning then they should use/re-introduce it as a log level (I dont think they should lol, but it is better than doing this)

case <-stopped:
timer.Stop()
}
Expand Down
6 changes: 2 additions & 4 deletions tests/integration/utils.go
View file
Open in desktop
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