Fix common logging #316
Fix common logging #316
Conversation
| 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.
@julio-lopez how are we planning to propagate the error from here?
There was a problem hiding this comment.
Good catch. Let me fix it and add tests.
There was a problem hiding this comment.
@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.
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.
@SupriyaKasten Expanded the tests, it should cover the basic functionality that is currently implemented. PTAL and LMK if I missed anything. Thanks.
julio-lopez
force-pushed
the
fix-common-logging
branch
from
aa820c1
to
2b9376f
Compare
Leverage Log(level, args ...) function
Covers basic functionality. The tests are dependent on the implementation since the output of logging is not directly "visible" to the caller and instead sent to a specific logging sink. The tests in this PR leverage implementation-specific hooks to check the expected behavior.
julio-lopez
force-pushed
the
fix-common-logging
branch
from
693f718
to
e619da3
Compare
|
@SupriyaKasten Expanded test coverage. PTAL. |
| } | ||
|
|
||
| func (l *logger) WithContext(ctx context.Context) Logger { | ||
| l.ctx = ctx | ||
| l.entry = l.entry.WithContext(ctx) |
There was a problem hiding this comment.
@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.
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, since logrus 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.
|
PR looks great! Thanks @julio-lopez 🚀 |
|
Thanks @SupriyaKasten |
Change Overview
Send log messages to a common logger instead of creating a new logger for every message.
In the future, this will allow implementing additional functionality such as:
Pull request type
Please check the type of change your PR introduces: