draft: logging implementation design #2010
Conversation
bhautikpip
requested review from
Aneurysm9,
dashpole,
evantorrie,
jmacd,
MadVikingGod,
MrAlias,
paivagustavo and
XSAM
as code owners
…y-go into log-implementation
…entelemetry-go into log-implementation
|
@Aneurysm9 @MrAlias Feel free to provide feedback. |
| ) | ||
|
|
||
| type Logger interface { | ||
| Log(level LogLevel, msg fmt.Stringer) |
There was a problem hiding this comment.
| 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.
There was a problem hiding this comment.
Yes. I think we can add context and attribute to the interface method.
There was a problem hiding this comment.
@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?
otellog/otel_log.go
Outdated
|
|
||
| const ( | ||
| // LogLevelDebug is usually only enabled when debugging. | ||
| LogLevelDebug LogLevel = iota + 1 |
There was a problem hiding this comment.
Should we try to align these with the levels currently defined in the logging spec draft?
otellog/otel_log.go
Outdated
|
|
||
| l.mu.Lock() | ||
| defer l.mu.Unlock() | ||
| _, _ = fmt.Fprintf(l.w, "%s [%s] %s\n", time.Now().Format(time.RFC3339), ll, msg) |
There was a problem hiding this comment.
| _, _ = fmt.Fprintf(l.w, "%s [%s] %s\n", time.Now().Format(time.RFC3339), ll, msg) | |
| _, _ = fmt.Fprintf(l.w, "%s\t[%s]\t%s\n", time.Now().Format(time.RFC3339), ll, msg) |
Tab-delimited records are easier to parse.
| "go.opentelemetry.io/otel/otellog" | ||
| ) | ||
|
|
||
| var Logger otellog.Logger = otellog.NewDefaultLogger(os.Stdout, otellog.LogLevelInfo) |
There was a problem hiding this comment.
I think we need to provide a mechanism somewhere for the application author to provide an alternate implementation.
There was a problem hiding this comment.
Agreed. I would like it if this closer followed what we do with the ErrorHandler interface.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| if ll < l.minLevel { | ||
| return | ||
| } |
There was a problem hiding this comment.
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.
|
I'm working on a very similar thing. And I would appreciate your feedback on: https://github.com/6543/log/tree/observability_api_draft . And it would be really great to unite forces if possible. Specifically for example my understanding of OpenTelemetry may be wrong :) |
| ) | ||
|
|
||
| type Logger interface { | ||
| Log(level LogLevel, msg fmt.Stringer) |
There was a problem hiding this comment.
| 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.
|
Closing in favor of #2343 |
Started with initial implementation design based on this doc: https://docs.google.com/document/d/1WIHyMyA8ZnKaobJlqvvfzyVBD3HHfU0OC3elOrWKUJs/edit
Feel free to provide feedback/suggestions on what type of interface or methods we should add and if the current directory structure would be okay for SDK.