Connect the Zap logger w/ the otel-go SDK error handler and internal logger #6717
Conversation
| otel.SetErrorHandler(otel.ErrorHandlerFunc(func(err error) { | ||
| logger.Error("OTel SDK error", zap.Error(err)) | ||
| })) | ||
| otel.SetLogger(zapr.NewLogger(logger)) |
There was a problem hiding this comment.
Very interesting choice of the logger implementation... I am a bit nervous about this extra dependency, but I think there is no other option...
question: If we set the logger, does otel not log the errors?
There was a problem hiding this comment.
By default, the built-in internal logger goes to the console. I don't have any examples lying around, and I would be OK not adopting this line as the error handler is more important.
There was a problem hiding this comment.
(I've wanted to catch up with https://go.googlesource.com/proposal/+/master/design/56345-structured-logging.md, regarding logging APIs)
There was a problem hiding this comment.
My question is, when you call otel.ErrorHandlerFunc do you not call the logger in advance? With this setup would errors going to be double logged?
There was a problem hiding this comment.
It looks like this is not the case. The default error handler uses the Go-1.0 log package, i.e., log.New(os.Stderr, "", log.LstdFlags), while the default internal logger uses logr's "stdr" package, i.e., stdr.New(log.New(os.Stderr, "", log.LstdFlags|log.Lshortfile)).
I admit I'm not sure whether it's easy/difficult to double-log, but they appear separate. @MadVikingGod would you comment on whether this PR looks sane for a user of zap, to configure both error handler and logger?
There was a problem hiding this comment.
There is no problem setting both. We prefer to use error handling to enable users to take action in response to errors. But nothing in the SDK should do both at the same time. Libraries outside of the base otel can't access the logging library.
I don't know about zap, but if you are looking for the info or debug logs you might need to increase zapr verbosity.
There was a problem hiding this comment.
My understanding is that with my change, if the collector were configured for verbose logging, then otel-go's debug logging would begin to appear because zapr does the right thing (I haven't verified this). My secondary goal was to ensure consistent formatting w/ the configured logger. Thanks @MadVikingGod
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
This PR was marked stale due to lack of activity. It will be closed in 14 days. |
|
Closed as inactive. Feel free to reopen if this PR is still being worked on. |
Description:
When the OTel-Go metric SDK is configured behind the
obsreportpackage, the logs and errors are not integrated with the collector's Zap logs. This change enables the OTel-Go SDK logs to go to the configured logger destination.Link to tracking Issue:
Testing: Manual testing. I've run the OTel-Go feature-gate with this change.