Add definition to log.Print() func #293
Conversation
SupriyaKasten
force-pushed
the
log-print-func
branch
from
0622ef0
to
29478b3
Compare
pkg/log/log.go
Outdated
| func (l *logger) Print(msg string, fields field.Fields) { | ||
| logFields := make(logrus.Fields) | ||
| if l.ctx != nil { | ||
| ctxFields := field.FromContext(l.ctx) |
There was a problem hiding this comment.
does this handle nil? Can we make it handle nil?
pkg/log/log.go
Outdated
| @@ -46,23 +48,62 @@ func WithContext(ctx context.Context) { | |||
| Info().WithContext(ctx) | |||
| } | |||
|
|
|||
| func (l *logger) Print(msg string) { | |||
| func (l *logger) Print(msg string, fields field.Fields) { | |||
There was a problem hiding this comment.
Please decouple accepting fields as a param and extracting the fields from the context.
There was a problem hiding this comment.
For now, let's have the Print signature be:
| func (l *logger) Print(msg string, fields field.Fields) { | |
| func (l *logger) Print(msg string) { |
The fields argument is not immediately needed, right? They will be extracted from the context, no?
We can easily extend the Print function later to accept other arguments. However, we have not gone through the exercise of evaluating the tradeoffs of different alternatives.
We decided to use WithContext() and WithError() for now as a mean to achieve the immediate desired functionality.
| func (l *logger) Print(msg string) { | ||
| logFields := make(logrus.Fields) | ||
| if ctxFields := field.FromContext(l.ctx); ctxFields != nil { |
There was a problem hiding this comment.
| if ctxFields := field.FromContext(l.ctx); ctxFields != nil { | |
| if ctxFields := field.FromContext(l.ctx); len(ctxFields) != 0 { |
There was a problem hiding this comment.
@tdmanv ctxFields is of type field.Fieldser interface. I think len(ctxFields) will fail.
There was a problem hiding this comment.
you're right! please ignore this comment
| @@ -46,23 +48,50 @@ func WithContext(ctx context.Context) { | |||
| Info().WithContext(ctx) | |||
| } | |||
|
|
|||
| func WithError(err error) { | |||
| Error().WithError(err) | |||
There was a problem hiding this comment.
Default to INFO for now.
We should have a discussion about whether the ERROR level is even needed in the context of (micro-)services.
The general guideline is that we should not use the ERROR level and the package implementation should discourage it and eventually not allow it.
| } | ||
| } | ||
|
|
||
| func (l *logger) WithContext(ctx context.Context) Logger { | ||
| l.ctx = ctx | ||
| if l.entry != nil { |
There was a problem hiding this comment.
Later we can figure out an idiomatic way of avoiding having to always check whether entry is nil or not.
And in particular, it is desirable to make "logger" immutable. That's mostly implementation and would be compatible with the API currently exposed.
Change Overview
Function definition for log.Print() func
Pull request type
Please check the type of change your PR introduces:
Test Plan