-
Notifications
You must be signed in to change notification settings - Fork 154
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.
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
Fix common logging #316
Fix common logging #316
Changes from all commits
5c3fc8a
79a291b
baab22d
fecffde
56fa442
e619da3
b42f30b
fcc3777
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -22,30 +22,28 @@ const ( | |
|
||
type logger struct { | ||
level Level | ||
entry *logrus.Entry | ||
ctx context.Context | ||
err error | ||
} | ||
|
||
func Info() Logger { | ||
// common logger implementation used in the library | ||
var log = logrus.New() | ||
|
||
func Info() Logger { | ||
return &logger{ | ||
level: InfoLevel, | ||
entry: logrus.NewEntry(logrus.New()), | ||
} | ||
} | ||
|
||
func Error() Logger { | ||
return &logger{ | ||
level: ErrorLevel, | ||
entry: logrus.NewEntry(logrus.New()), | ||
} | ||
} | ||
|
||
func Debug() Logger { | ||
return &logger{ | ||
level: DebugLevel, | ||
entry: logrus.NewEntry(logrus.New()), | ||
} | ||
} | ||
|
||
|
@@ -54,12 +52,12 @@ func Print(msg string) { | |
Info().Print(msg) | ||
} | ||
|
||
func WithContext(ctx context.Context) { | ||
Info().WithContext(ctx) | ||
func WithContext(ctx context.Context) Logger { | ||
return Info().WithContext(ctx) | ||
} | ||
|
||
func WithError(err error) { | ||
Info().WithError(err) | ||
func WithError(err error) Logger { | ||
return Info().WithError(err) | ||
} | ||
|
||
func (l *logger) Print(msg string) { | ||
|
@@ -69,27 +67,19 @@ func (l *logger) Print(msg string) { | |
logFields[cf.Key()] = cf.Value() | ||
} | ||
} | ||
|
||
l.entry = l.entry.WithFields(logFields) | ||
|
||
switch l.level { | ||
case InfoLevel: | ||
l.entry.Info(msg) | ||
case ErrorLevel: | ||
l.entry.Error(msg) | ||
case DebugLevel: | ||
l.entry.Debug(msg) | ||
entry := log.WithFields(logFields) | ||
if l.err != nil { | ||
entry = entry.WithError(l.err) | ||
} | ||
entry.Log(logrus.Level(l.level), msg) | ||
} | ||
|
||
func (l *logger) WithContext(ctx context.Context) Logger { | ||
l.ctx = ctx | ||
l.entry = l.entry.WithContext(ctx) | ||
return l | ||
} | ||
|
||
func (l *logger) WithError(err error) Logger { | ||
l.err = err | ||
l.entry = l.entry.WithError(err) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @julio-lopez how are we planning to propagate the error from here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch. Let me fix it and add tests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SupriyaKasten Addressed the issue and started adding a minimal set of tests. This particular issue is not covered by the current set of tests. I'll add more. Feel free to block the PR until enough test coverage is included. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @julio-lopez, will block it, so that mergify does not merges it automatically before it is ready. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @SupriyaKasten Expanded the tests, it should cover the basic functionality that is currently implemented. PTAL and LMK if I missed anything. Thanks. |
||
return l | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,98 @@ | ||
package log | ||
|
||
import ( | ||
"bytes" | ||
"context" | ||
"encoding/json" | ||
"errors" | ||
"testing" | ||
"time" | ||
|
||
"github.com/kanisterio/kanister/pkg/field" | ||
"github.com/sirupsen/logrus" | ||
. "gopkg.in/check.v1" | ||
) | ||
|
||
const ( | ||
infoLevelStr = "info" | ||
errorLevelStr = "error" | ||
debugLevelStr = "debug" | ||
) | ||
|
||
type LogSuite struct{} | ||
|
||
var _ = Suite(&LogSuite{}) | ||
|
||
func Test(t *testing.T) { | ||
TestingT(t) | ||
} | ||
|
||
func (s *LogSuite) TestWithNilError(c *C) { | ||
log.SetFormatter(&logrus.JSONFormatter{TimestampFormat: time.RFC3339Nano}) | ||
// Should not panic | ||
WithError(nil).Print("Message") | ||
} | ||
|
||
func (s *LogSuite) TestWithNilContext(c *C) { | ||
log.SetFormatter(&logrus.JSONFormatter{TimestampFormat: time.RFC3339Nano}) | ||
// Should not panic | ||
WithContext(nil).Print("Message") | ||
} | ||
|
||
func (s *LogSuite) TestLogMessage(c *C) { | ||
const text = "Some useful text." | ||
testLogMessage(c, text, Print) | ||
} | ||
|
||
func (s *LogSuite) TestLogWithError(c *C) { | ||
const text = "My error message" | ||
err := errors.New("test error") | ||
entry := testLogMessage(c, text, WithError(err).Print) | ||
c.Assert(entry["error"], Equals, err.Error()) | ||
c.Assert(entry["level"], Equals, infoLevelStr) | ||
} | ||
|
||
func (s *LogSuite) TestLogWithContext(c *C) { | ||
const text = "My error message" | ||
ctx := context.Background() | ||
entry := testLogMessage(c, text, WithContext(ctx).Print) | ||
c.Assert(entry["level"], Equals, infoLevelStr) | ||
// Error should not be set in the log entry | ||
c.Assert(entry["error"], Equals, nil) | ||
} | ||
|
||
func (s *LogSuite) TestLogWithContextFields(c *C) { | ||
const text = "My error message" | ||
ctx := field.Context(context.Background(), "key", "value") | ||
entry := testLogMessage(c, text, WithContext(ctx).Print) | ||
c.Assert(entry["level"], Equals, infoLevelStr) | ||
// Error should not be set in the log entry | ||
c.Assert(entry["error"], Equals, nil) | ||
// A field with "key" should be set in the log entry | ||
c.Assert(entry["key"], Equals, "value") | ||
} | ||
|
||
func (s *LogSuite) TestLogWithContextFieldsAndError(c *C) { | ||
const text = "My error message" | ||
ctx := field.Context(context.Background(), "key", "value") | ||
err := errors.New("test error") | ||
entry := testLogMessage(c, text, WithError(err).WithContext(ctx).Print) | ||
c.Assert(entry["level"], Equals, infoLevelStr) | ||
// Error should be included in the log entry | ||
c.Assert(entry["error"], Equals, err.Error()) | ||
// A field with "key" should be set in the log entry | ||
c.Assert(entry["key"], Equals, "value") | ||
} | ||
|
||
func testLogMessage(c *C, msg string, print func(string)) map[string]interface{} { | ||
log.SetFormatter(&logrus.JSONFormatter{TimestampFormat: time.RFC3339Nano}) | ||
var memLog bytes.Buffer | ||
log.SetOutput(&memLog) | ||
print(msg) | ||
var entry map[string]interface{} | ||
err := json.Unmarshal(memLog.Bytes(), &entry) | ||
c.Assert(err, IsNil) | ||
c.Assert(entry, NotNil) | ||
c.Assert(entry["msg"], Equals, msg) | ||
return entry | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@julio-lopez What do you think about setting the logrus.entry.Context using:
entry =entry.WithContext(l.ctx)
in the func(l *logger) Print(msg string){}
? We do extract the context fields though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issues is that it would require keeping the entry around, instead of the context.
We may want to change the approach in the future and set the context in the entry, as you suggest, and then get the fields from the context at a much later stage, in the "hooks" or the "formatters".
There is no need to set the context in the entry and keep it around, given the current approach where the fields are extracted and added to the entry before calling
Log(level, ...)
, as you point out. The fields need to be explicitly extracted, sincelogrus
does not do anything special with the context itself. In particular, there is no context specific information in the log messages sent by default to stderr. This is because there is no general way to discover what values have been set in a context.