Skip to content
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.

Sign up for GitHub

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

SNOW-625282: Make logrus dependency optional or drop it all together #1286

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

SNOW-625282: Make logrus dependency optional or drop it all together #1286

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
0bc7382
SNOW-625282: Make logrus dependency optional or drop it all together
rickr-sigma Jan 6, 2025
9501757
minor cleanup
rickr-sigma Jan 7, 2025
d6562e6
add some rm methods back
rickr-sigma Jan 13, 2025
1670ee4
fixups to make diff cleaner
rickr-sigma Jan 13, 2025
87f99ed
undo
rickr-sigma Jan 13, 2025
10f8419
undo
rickr-sigma Jan 13, 2025
d18ddb2
undo
rickr-sigma Jan 13, 2025
f91df81
undo
rickr-sigma Jan 13, 2025
6df823d
fix test
rickr-sigma Jan 17, 2025
f179022
Merge remote-tracking branch 'origin/master' into rr/logif
rickr-sigma Jan 17, 2025
8f24481
higher level of backwards compat
rickr-sigma Jan 17, 2025
5f8b1fd
even less changes
rickr-sigma Jan 17, 2025
cbd4cd7
more cleanup
rickr-sigma Jan 17, 2025
ee9f8f3
undo
rickr-sigma Jan 17, 2025
3975959
clean diff
rickr-sigma Jan 17, 2025
2dcc0ac
more cleanup
rickr-sigma Jan 17, 2025
45f7c85
put function logging back in
rickr-sigma Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion auth.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -353,7 +353,7 @@ func authenticate(
params.Add("roleName", sc.cfg.Role)
}

logger.WithContext(ctx).WithContext(sc.ctx).Infof("PARAMS for Auth: %v, %v, %v, %v, %v, %v",
logger.WithContext(ctx, sc.ctx).Infof("PARAMS for Auth: %v, %v, %v, %v, %v, %v",
params, sc.rest.Protocol, sc.rest.Host, sc.rest.Port, sc.rest.LoginTimeout, sc.cfg.Authenticator.String())

respd, err := sc.rest.FuncPostAuth(ctx, sc.rest, sc.rest.getClientFor(sc.cfg.Authenticator), params, headers, bodyCreator, sc.rest.LoginTimeout)
Expand Down
12 changes: 5 additions & 7 deletions ctx_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,15 +38,13 @@ func TestCtxVal(t *testing.T) {
}
}

func TestLogEntryCtx(t *testing.T) {
func TestLogCtx(t *testing.T) {
var log = logger
var ctx1 = context.WithValue(context.Background(), SFSessionIDKey, "sessID1")
var ctx2 = context.WithValue(context.Background(), SFSessionUserKey, "admin")

fs1 := context2Fields(ctx1)
fs2 := context2Fields(ctx2)
l1 := log.WithFields(*fs1)
l2 := log.WithFields(*fs2)
l1.Info("Hello 1")
l2.Warning("Hello 2")
l := log.WithContext(ctx1, ctx2)
l.Info("Hello 1")
l.Warn("Hello 2")
// what purpose does this test serve? ... nothing is being validated except that it compiles and runs.
}
202 changes: 75 additions & 127 deletions log.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,12 +40,52 @@ func RegisterLogContextHook(contextKey string, ctxExtractor ClientLogContextHook
// logger.WithContext is used
var LogKeys = [...]contextKey{SFSessionIDKey, SFSessionUserKey}

// Fields
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any comment? Why do we need another public var?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I meant Fields :)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

  1. Global map with fields actually global, like instance name/host, OS or app version - that is fine.
  2. WithFields used 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.

type Fields map[string]any

type LogEntry interface {
Tracef(format string, args ...interface{})
Debugf(format string, args ...interface{})
Infof(format string, args ...interface{})
Printf(format string, args ...interface{})
Warnf(format string, args ...interface{})
Warningf(format string, args ...interface{})
Errorf(format string, args ...interface{})
Fatalf(format string, args ...interface{})
Panicf(format string, args ...interface{})

Trace(args ...interface{})
Debug(args ...interface{})
Info(args ...interface{})
Print(args ...interface{})
Warn(args ...interface{})
Warning(args ...interface{})
Error(args ...interface{})
Fatal(args ...interface{})
Panic(args ...interface{})

Traceln(args ...interface{})
Debugln(args ...interface{})
Infoln(args ...interface{})
Println(args ...interface{})
Warnln(args ...interface{})
Warningln(args ...interface{})
Errorln(args ...interface{})
Fatalln(args ...interface{})
Panicln(args ...interface{})
}

// SFLogger Snowflake logger interface to expose FieldLogger defined in logrus
type SFLogger interface {
rlog.Ext1FieldLogger
LogEntry
WithField(key string, value interface{}) LogEntry
WithFields(fields Fields) LogEntry
WithError(err error) LogEntry
WithTime(t time.Time) LogEntry

SetLogLevel(level string) error
GetLogLevel() string
WithContext(ctx context.Context) *rlog.Entry
WithContext(ctx ...context.Context) LogEntry
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added ConvertibleEntry interface so that callers can convert the underlying LogEntry into a logrus Entry struct. Test case TestLogEntryConversion() shows how to do this.

SetOutput(output io.Writer)
CloseFileOnLoggerReplace(file *os.File) error
Replace(newLogger *SFLogger)
Expand All @@ -56,6 +96,8 @@ func SFCallerPrettyfier(frame *runtime.Frame) (string, string) {
return path.Base(frame.Function), fmt.Sprintf("%s:%d", path.Base(frame.File), frame.Line)
}

var _ SFLogger = &defaultLogger{} // ensure defaultLogger isa SFLogger.

type defaultLogger struct {
inner *rlog.Logger
enabled bool
Expand All @@ -81,7 +123,7 @@ func (log *defaultLogger) SetLogLevel(level string) error {
if err != nil {
return err
}
log.inner.SetLevel(actualLevel)
log.inner.Level = actualLevel
}
return nil
}
Expand All @@ -91,7 +133,7 @@ func (log *defaultLogger) GetLogLevel() string {
if !log.enabled {
return "OFF"
}
return log.inner.GetLevel().String()
return log.inner.Level.String()
}

// CloseFileOnLoggerReplace set a file to be closed when releasing resources occupied by the logger
Expand Down Expand Up @@ -119,9 +161,9 @@ func closeLogFile(file *os.File) {
}

// WithContext return Entry to include fields in context
func (log *defaultLogger) WithContext(ctx context.Context) *rlog.Entry {
fields := context2Fields(ctx)
return log.inner.WithFields(*fields)
func (log *defaultLogger) WithContext(ctxs ...context.Context) LogEntry {
fields := context2Fields(ctxs...)
return log.WithFields(*fields)
}

// CreateDefaultLogger return a new instance of SFLogger with default config
Expand All @@ -132,37 +174,39 @@ func CreateDefaultLogger() SFLogger {
rLogger.SetFormatter(formatter)
rLogger.SetReportCaller(true)
var ret = defaultLogger{inner: rLogger, enabled: true}
return &ret //(&ret).(*SFLogger)
return &ret
}

// WithField allocates a new entry and adds a field to it.
// Debug, Print, Info, Warn, Error, Fatal or Panic must be then applied to
// this new returned entry.
// If you want multiple fields, use `WithFields`.
func (log *defaultLogger) WithField(key string, value interface{}) *rlog.Entry {
return log.inner.WithField(key, value)

func (log *defaultLogger) WithField(key string, value interface{}) LogEntry {
return &entryBridge{log.inner.WithField(key, value)}
}

// Adds a struct of fields to the log entry. All it does is call `WithField` for
// each `Field`.
func (log *defaultLogger) WithFields(fields rlog.Fields) *rlog.Entry {
return log.inner.WithFields(fields)
func (log *defaultLogger) WithFields(fields Fields) LogEntry {
m := map[string]any(fields)
return &entryBridge{log.inner.WithFields(m)}
}

// Add an error as single field to the log entry. All it does is call
// `WithError` for the given `error`.
func (log *defaultLogger) WithError(err error) *rlog.Entry {
return log.inner.WithError(err)
func (log *defaultLogger) WithError(err error) LogEntry {
return &entryBridge{log.inner.WithError(err)}
}

// Overrides the time of the log entry.
func (log *defaultLogger) WithTime(t time.Time) *rlog.Entry {
return log.inner.WithTime(t)
// WithTime overrides the time of the log entry.
func (log *defaultLogger) WithTime(t time.Time) LogEntry {
return &entryBridge{log.inner.WithTime(t)}
}

func (log *defaultLogger) Logf(level rlog.Level, format string, args ...interface{}) {
log.inner.Logf(level, format, args...)
var _ LogEntry = &entryBridge{} // ensure entryBridge isa LogEntry.

type entryBridge struct {
*rlog.Entry
}

func (log *defaultLogger) Tracef(format string, args ...interface{}) {
Expand Down Expand Up @@ -219,14 +263,6 @@ func (log *defaultLogger) Panicf(format string, args ...interface{}) {
}
}

func (log *defaultLogger) Log(level rlog.Level, args ...interface{}) {
log.inner.Log(level, args...)
}

func (log *defaultLogger) LogFn(level rlog.Level, fn rlog.LogFunction) {
log.inner.LogFn(level, fn)
}

func (log *defaultLogger) Trace(args ...interface{}) {
if log.enabled {
log.inner.Trace(args...)
Expand Down Expand Up @@ -281,64 +317,6 @@ func (log *defaultLogger) Panic(args ...interface{}) {
}
}

func (log *defaultLogger) TraceFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.TraceFn(fn)
}
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all Fn variants removed; again unused in prod code.


func (log *defaultLogger) DebugFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.DebugFn(fn)
}
}

func (log *defaultLogger) InfoFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.InfoFn(fn)
}
}

func (log *defaultLogger) PrintFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.PrintFn(fn)
}
}

func (log *defaultLogger) WarnFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.PrintFn(fn)
}
}

func (log *defaultLogger) WarningFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.WarningFn(fn)
}
}

func (log *defaultLogger) ErrorFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.ErrorFn(fn)
}
}

func (log *defaultLogger) FatalFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.FatalFn(fn)
}
}

func (log *defaultLogger) PanicFn(fn rlog.LogFunction) {
if log.enabled {
log.inner.PanicFn(fn)
}
}

func (log *defaultLogger) Logln(level rlog.Level, args ...interface{}) {
log.inner.Logln(level, args...)
}

func (log *defaultLogger) Traceln(args ...interface{}) {
if log.enabled {
log.inner.Traceln(args...)
Expand Down Expand Up @@ -393,45 +371,11 @@ func (log *defaultLogger) Panicln(args ...interface{}) {
}
}

func (log *defaultLogger) Exit(code int) {
log.inner.Exit(code)
}

// SetLevel sets the logger level.
func (log *defaultLogger) SetLevel(level rlog.Level) {
log.inner.SetLevel(level)
}

// GetLevel returns the logger level.
func (log *defaultLogger) GetLevel() rlog.Level {
return log.inner.GetLevel()
}

// 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)
}
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused.


// SetFormatter sets the logger formatter.
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)
}

Copy link
Author

@rickr-sigma rickr-sigma Jan 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SetFormatter / SetReportCaller only have 1 internal usage and do not need to be part of the interface.

// SetLogger set a new logger of SFLogger interface for gosnowflake
func SetLogger(inLogger *SFLogger) {
logger = *inLogger //.(*defaultLogger)
Expand All @@ -442,21 +386,25 @@ func GetLogger() SFLogger {
return logger
}

func context2Fields(ctx context.Context) *rlog.Fields {
var fields = rlog.Fields{}
if ctx == nil {
func context2Fields(ctxs ...context.Context) *Fields {
var fields = Fields{}
if len(ctxs) <= 0 {
return &fields
}

for i := 0; i < len(LogKeys); i++ {
if ctx.Value(LogKeys[i]) != nil {
fields[string(LogKeys[i])] = ctx.Value(LogKeys[i])
for _, ctx := range ctxs {
if ctx.Value(LogKeys[i]) != nil {
fields[string(LogKeys[i])] = ctx.Value(LogKeys[i])
}
}
}

for key, hook := range clientLogContextHooks {
if value := hook(ctx); value != "" {
fields[key] = value
for _, ctx := range ctxs {
if value := hook(ctx); value != "" {
fields[key] = value
}
}
}

Expand Down
Loading
Loading