diff --git a/changelogs/unreleased/1301-skriss b/changelogs/unreleased/1301-skriss new file mode 100644 index 000000000..953af85df --- /dev/null +++ b/changelogs/unreleased/1301-skriss @@ -0,0 +1 @@ +log error locations from plugin logger, and don't overwrite them in the client logger if they exist already diff --git a/pkg/plugin/framework/logger.go b/pkg/plugin/framework/logger.go index 85d24dd4b..c5396856e 100644 --- a/pkg/plugin/framework/logger.go +++ b/pkg/plugin/framework/logger.go @@ -50,6 +50,9 @@ func newLogger() *logrus.Logger { // server logger that the location has been set within a hook. logger.Hooks.Add((&logging.LogLocationHook{}).WithLoggerName("plugin")) + // make sure we attempt to record the error location + logger.Hooks.Add(&logging.ErrorLocationHook{}) + // this hook adjusts the string representation of WarnLevel to "warn" // rather than "warning" to make it parseable by go-plugin within the // Velero server code diff --git a/pkg/plugin/framework/logger_test.go b/pkg/plugin/framework/logger_test.go index bfbdcf3b8..787bf2f88 100644 --- a/pkg/plugin/framework/logger_test.go +++ b/pkg/plugin/framework/logger_test.go @@ -37,6 +37,7 @@ func TestNewLogger(t *testing.T) { expectedHooks := []logrus.Hook{ (&logging.LogLocationHook{}).WithLoggerName("plugin"), + &logging.ErrorLocationHook{}, &logging.HcLogLevelHook{}, } diff --git a/pkg/util/logging/error_location_hook.go b/pkg/util/logging/error_location_hook.go index a00206ceb..861931a8e 100644 --- a/pkg/util/logging/error_location_hook.go +++ b/pkg/util/logging/error_location_hook.go @@ -1,5 +1,5 @@ /* -Copyright 2017 the Velero contributors. +Copyright 2017, 2019 the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -48,6 +48,17 @@ func (h *ErrorLocationHook) Fire(entry *logrus.Entry) error { return nil } + if _, ok := entry.Data[errorFileField]; ok { + // If there is already an error file field, preserve it instead of overwriting it. This field will already exist if + // the log message occurred in the server half of a plugin. + return nil + } + if _, ok := entry.Data[errorFunctionField]; ok { + // If there is already an error function field, preserve it instead of overwriting it. This field will already exist if + // the log message occurred in the server half of a plugin. + return nil + } + err, ok := errObj.(error) if !ok { return errors.New("object logged as error does not satisfy error interface") diff --git a/pkg/util/logging/error_location_hook_test.go b/pkg/util/logging/error_location_hook_test.go index 542d5f065..180680eec 100644 --- a/pkg/util/logging/error_location_hook_test.go +++ b/pkg/util/logging/error_location_hook_test.go @@ -1,5 +1,5 @@ /* -Copyright 2017 the Velero contributors. +Copyright 2017, 2019 the Velero contributors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -58,6 +58,19 @@ func TestFire(t *testing.T) { errorFunctionField: "TestFire", }, }, + { + name: "already have error file and function fields", + preEntryFields: map[string]interface{}{ + logrus.ErrorKey: pkgerrs.New("a pkg/errors error"), + errorFileField: "some_file.go:123", + errorFunctionField: "SomeFunction", + }, + expectedEntryFields: map[string]interface{}{ + logrus.ErrorKey: pkgerrs.New("a pkg/errors error"), + errorFileField: "some_file.go:123", + errorFunctionField: "SomeFunction", + }, + }, } for _, test := range tests {