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.

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

draft: logging implementation design #2010

Closed
wants to merge 6 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
84 changes: 84 additions & 0 deletions internal/logger/logger.go
@@ -0,0 +1,84 @@
package logger

import (
"fmt"
"os"

"go.opentelemetry.io/otel/otellog"
)

var Logger otellog.Logger = otellog.NewDefaultLogger(os.Stdout, otellog.LogLevelInfo)
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to provide a mechanism somewhere for the application author to provide an alternate implementation.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I would like it if this closer followed what we do with the ErrorHandler interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes I was thinking to add SetLogger API to add custom logging implementation but I will try to follow @MrAlias's suggestion and lookup ErrorHandler interface to maintain implementation parity within SDK.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I was suggesting, in a very terse manner, was that we include a SetLogger and GetLogger function in the otel package and move the Logger interface to the otel package as well. This would mirror what is done with the ErrorHandler. From there, the default implementation could be abstracted away into an internal package.


func Tracef(format string, args ...interface{}) {
Logger.Log(otellog.LogLevelTrace, printfArgs{format, args})
}

func Trace(args ...interface{}) {
Logger.Log(otellog.LogLevelTrace, printArgs(args))
}

func Debugf(format string, args ...interface{}) {
Logger.Log(otellog.LogLevelDebug, printfArgs{format, args})
}

func Debug(args ...interface{}) {
Logger.Log(otellog.LogLevelDebug, printArgs(args))
}

func DebugDeferred(fn func() string) {
Logger.Log(otellog.LogLevelDebug, stringerFunc(fn))
}

func Infof(format string, args ...interface{}) {
Logger.Log(otellog.LogLevelInfo, printfArgs{format, args})
}

func Info(args ...interface{}) {
Logger.Log(otellog.LogLevelInfo, printArgs(args))
}

func Warnf(format string, args ...interface{}) {
Logger.Log(otellog.LogLevelWarn, printfArgs{format, args})
}

func Warn(args ...interface{}) {
Logger.Log(otellog.LogLevelWarn, printArgs(args))
}

func Errorf(format string, args ...interface{}) {
Logger.Log(otellog.LogLevelError, printfArgs{format, args})
}

func Error(args ...interface{}) {
Logger.Log(otellog.LogLevelError, printArgs(args))
}

func Fatalf(format string, args ...interface{}) {
Logger.Log(otellog.LogLevelFatal, printfArgs{format, args})
}

func Fatal(args ...interface{}) {
Logger.Log(otellog.LogLevelFatal, printArgs(args))
}

type printfArgs struct {
format string
args []interface{}
}

func (p printfArgs) String() string {
return fmt.Sprintf(p.format, p.args...)
}

type printArgs []interface{}

func (p printArgs) String() string {
return fmt.Sprint([]interface{}(p)...)
}

type stringerFunc func() string

func (sf stringerFunc) String() string {
return sf()
}

80 changes: 80 additions & 0 deletions otellog/otel_log.go
@@ -0,0 +1,80 @@
package otellog

import (
"fmt"
"io"
"sync"
"time"
)

type Logger interface {
Log(level LogLevel, msg fmt.Stringer)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Log(level LogLevel, msg fmt.Stringer)
Log(ctx context.Context, level LogLevel, msg fmt.Stringer, attrs ...attribute.KeyValue)

What about this interface? Including the Context enables the logger to correlate entries with active spans or other context data. Including attributes allows for something more like structured logging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. I think we can add context and attribute to the interface method.

Copy link

@xaionaro xaionaro Sep 3, 2021

Choose a reason for hiding this comment

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

@Aneurysm9 , @bhautikpip : In structured logging I used to see an hierarchy of values. Like WithValue in logrus and With in zap: in some function we added a value and called another function, then in that function we added a one value more and called a logger.

And I do not understand how it supposed to work in this interface. Is the hierarchy of values stored in the context?

Copy link

Choose a reason for hiding this comment

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

Suggested change
Log(level LogLevel, msg fmt.Stringer)
Log(ctx context.Context, level LogLevel, msg fmt.Stringer, attrs ...attribute.KeyValue)
ContextDecoder(ctx context.Context) []attribute.KeyValue

What about this interface? I think just including the Context is not enough. It should provide an way to "decode" the Context, which means getting values from the Context and logging these values that we exactly need.
For example, In a http server, the Context invocation chain always begins from the Request.Context(). Therefore, there are many redundant values in the passed Context. It is expensive to log all these values. What we need in such a Context may just the tracing related values. In such a situation, including a ContextDecoder can solve it easily, remaining scalability and flexibility.
In default case, it is enough maintaining a NoopDecoder that ignoring the Context and returning nil.

}

type LogLevel int

const (
// LogLevelTrace is used to for fine-grained debugging event and disabled in default configurations.
LogLevelTrace LogLevel = iota + 1

// LogLevelDebug is usually only enabled when debugging.
LogLevelDebug

// LogLevelInfo is used only for informal event indicates that event has happened.
LogLevelInfo

// LogLevelWarn is non-critical entries that deserve eyes.
LogLevelWarn

// LogLevelError is used for errors that should definitely be noted.
LogLevelError

// LogLevelError is used for fatal errors such as application or system crash.
LogLevelFatal
)

func (ll LogLevel) String() string {
switch ll {
case LogLevelTrace:
return "TRACE"
case LogLevelDebug:
return "DEBUG"
case LogLevelInfo:
return "INFO"
case LogLevelWarn:
return "WARN"
case LogLevelError:
return "ERROR"
case LogLevelFatal:
return "FATAL"
default:
return fmt.Sprintf("UNKNOWNLOGLEVEL<%d>", ll)
}
}

func NewDefaultLogger(w io.Writer, minLogLevel LogLevel) Logger {
return &defaultLogger{w: w, minLevel: minLogLevel}
}

type defaultLogger struct {
mu sync.Mutex
w io.Writer
minLevel LogLevel
}

func (l *defaultLogger) Log(ll LogLevel, msg fmt.Stringer) {
if ll < l.minLevel {
return
}
Comment on lines +66 to +68
Copy link
Member

@pellared pellared Jun 29, 2021

Choose a reason for hiding this comment

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

nit but I think that important:
we need some API to check the current log level so that we can prevent unnecessary printf when big objects are being formated

at this point, it is a little too late: if one does logger.Tracef("dump my big object %v", iAmHuge) then iAmHuge would be formatted in the memory

I suggest adding SetLogLevel function to the logger package (side-by-side to the SetLogger function). So that the functions like Trace and Tracef in this package could take advantage of the global log level.


l.mu.Lock()
defer l.mu.Unlock()
_, _ = fmt.Fprintf(l.w, "%s\t[%s]\t%s\n", time.Now().Format(time.RFC3339), ll, msg)
}

var NullLogger = nullLogger{}

type nullLogger struct{}

func (nl nullLogger) Log(ll LogLevel, msg fmt.Stringer) {
}