SNOW-625282: Make logrus dependency optional or drop it all together #1286
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
log.go
Outdated
| func (log *defaultLogger) TraceFn(fn rlog.LogFunction) { | ||
| if log.enabled { | ||
| log.inner.TraceFn(fn) | ||
| } | ||
| } |
There was a problem hiding this comment.
all Fn variants removed; again unused in prod code.
log.go
Outdated
| // AddHook adds a hook to the logger hooks. | ||
| func (log *defaultLogger) AddHook(hook rlog.Hook) { | ||
| log.inner.AddHook(hook) | ||
|
|
||
| } | ||
|
|
||
| // IsLevelEnabled checks if the log level of the logger is greater than the level param | ||
| func (log *defaultLogger) IsLevelEnabled(level rlog.Level) bool { | ||
| return log.inner.IsLevelEnabled(level) | ||
| } |
log.go
Outdated
| func (log *defaultLogger) SetFormatter(formatter rlog.Formatter) { | ||
| log.inner.SetFormatter(formatter) | ||
| } | ||
|
|
||
| // SetOutput sets the logger output. | ||
| func (log *defaultLogger) SetOutput(output io.Writer) { | ||
| log.inner.SetOutput(output) | ||
| } | ||
|
|
||
| func (log *defaultLogger) SetReportCaller(reportCaller bool) { | ||
| log.inner.SetReportCaller(reportCaller) | ||
| } | ||
|
|
There was a problem hiding this comment.
SetFormatter / SetReportCaller only have 1 internal usage and do not need to be part of the interface.
|
I have read the CLA Document and I hereby sign the CLA |
| SetLogLevel(level string) error | ||
| GetLogLevel() string | ||
| WithContext(ctx context.Context) *rlog.Entry | ||
| WithContext(ctx ...context.Context) LogEntry |
There was a problem hiding this comment.
It is backward compatible, isn't it? Previously this function returned a struct, now it returns the interface which is implemented by logrus struct, right?
There was a problem hiding this comment.
This is probably the biggest change. Every field in the rlog.Entry struct is exported and modifiable. With a LogEntry you can only emit a log line via trace/debug/info/... .
The rationale behind this change is that ultimately we want to be able to have callers of the driver be able to have full control over the logger and this is only possible when interfaces are exposed, or at the very least its a lot easier.
If we're still looking for a means of exposing the underlying logrus Entry we could ensure that a cast from LogEntry works. I'll wire up a unit test to make sure this is straightforward enough. It doesn't provide 100% code compatibility, but at least it provides an out for anyone that has directly manipulated the logrus Entries.
There was a problem hiding this comment.
Added ConvertibleEntry interface so that callers can convert the underlying LogEntry into a logrus Entry struct. Test case TestLogEntryConversion() shows how to do this.
|
@sfc-gh-pfus please have another look, as this version now maximizes backwards compatibility. The breaking change is now narrowed down to WithContext/WithField/WithFields/WithError which now return LogEntry rather than the underlying logrus Entry struct. The function ToEntry() on the ConvertibleEntry interface can be used to obtain the wrapped logrus Entry struct from a LogEntry but it requires a change on the caller side (i.e. 1 additional method call). See TestLogEntryConversion for an example. |
| @@ -40,12 +40,59 @@ func RegisterLogContextHook(contextKey string, ctxExtractor ClientLogContextHook | |||
| // logger.WithContext is used | |||
| var LogKeys = [...]contextKey{SFSessionIDKey, SFSessionUserKey} | |||
|
|
|||
| // SFLogger Snowflake logger interface to expose FieldLogger defined in logrus | |||
| // Fields | |||
There was a problem hiding this comment.
Any comment? Why do we need another public var?
There was a problem hiding this comment.
LogKeys ? It was added in 22Dec20 looks like a convenience for the iteration. At the very least we could make it package local or even fold it into the context2Fields() func, which contains the only use.
There was a problem hiding this comment.
No, I meant Fields :)
There was a problem hiding this comment.
the Fields type matches the definition of logrus Fields and exists for backward compatibility. I.e. the existing withXXX methods on FieldLogger are;
WithField(key string, value interface{}) *Entry
WithFields(fields Fields) *Entry
WithError(err error) *Entry
so keeping the interfaces as close as possible we have SFLogger containing;
WithField(key string, value interface{}) LogEntry
WithFields(fields Fields) LogEntry
WithError(err error) LogEntry
There was a problem hiding this comment.
Ok, so just leave a comment above the var :) Also, if we want to release it as a non compatible change, we don't have to write in a comment that it is a backward compatibility to logrus.
Regardless the variable itself, I see two use cases.
- Global map with fields actually global, like instance name/host, OS or app version - that is fine.
WithFieldsused to propagate fields to the actual log entry. I can't see a good examples, but my intuition says that in some cases it may be helpful, so probably also good to have it.
Description
The following changes decouple logrs from SFLogger, with the eventual goal to allow per context logging control (in a followup PR) by a user provided SFLogger.
This PR is not (yet) complete but is presented as a proof-of-concept. Further cleanup will be performed if this approach is deemed acceptable. One thing to be aware of is that certain interfaces are no longer supported in particular the functional variants of logging, and the WithTime, WithFields, WithError, Panic and Fatal which were all ONLY being exercised in unit tests.
Checklist