-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Support slog for internal logs, beside of logr #4658
Comments
If you don't mind, I can make a pull request for it. |
Why not just wrap the This seems like it will bloat the API. Given we still need to support Go 1.20 I would like to think this through before we add and deprecate something in a stable API. |
It would make more sense for OTel to define its own logger interface that it uses (that can match the |
See https://github.com/go-logr/logr/blob/master/slogr/example/main.go to check how to plug a slog handler as logr logger. |
Yeah, but this is forcing anyone using the opentelemetry package into taking a dependency on |
Correct, we decided to use
More: https://github.com/go-logr/logr#background
|
Ok, that makes sense - I was not particularly interested in |
Using slog as a logr backend (logr.LogSink) is a good tradeoff nowadays. For long term, the app and library developers must choose between the 2 logging interfaces: logr vs slog. I cannot predict which will be the winner after 2 years. I'm using Go for 6 years and the famous log library was changed in each 2 years. I accept @MrAlias opinion, the API should not deprecate a call, because it introduces a long-term confusing situation. So, if the app which is using an slog-based logging solution (the backend of slog can be zerolog or logrus), the https://pkg.go.dev/github.com/go-logr/logr/slogr can be used to set custom logr logger as internal logger of OpenTelemetry library. So the call chain can be, for example: OpenTelemetry --> logr --> slog --> logrus. I don't know exactly, what is the learning lesson of this story. Maybe if the Go std lib developers had introduced the slog earlier (or they had integrated logr), we wouldn't be in this situation now. |
This is correct. We added the internal logger in https://github.com/open-telemetry/opentelemetry-go/releases/tag/v1.3.0 (released in Dec 10, 2021). The slog proposal golang/go#56345 was created in Oct 20, 2022. Side note: OTel was driving one of the slog design decisions: https://go.googlesource.com/proposal/+/master/design/56345-structured-logging.md#levels |
I have an issue with using logr. Using As a result, I have to have otel -> logr -> zapr -> custom zap.Core -> zap, or otel -> custom logr -> zap. If this used an interface with the appropriate methods, I could have a simple shim without needing zapr anywhere and leave the leveling up to the given users and their impls. |
This is a known thing. See: https://github.com/go-logr/zapr#increasing-verbosity
You can also write a custom Thanks to |
Having interfaces, one does not have to deal with translating levels. Instead, it is more obvious what each function does, and keeps the deps cleaner. I ended up writing a custom |
I think we should document the logging levels in use in https://pkg.go.dev/go.opentelemetry.io/otel#SetLogger |
Problem Statement
The
log/slog
package already released in Go 1.21. But it's unable to set forotel.SetLogger
, because it expectslogr.Logger
.Proposed Solution
OTEL uses only a few log functions (
Error
,Warn
,Info
,Debug
), so a new interface can be defined, for example:In order to keep backward compatibility, a new function should be introduced, which sets this kind of logger, for example:
The original
otel.SetLogger
is kept for backward compatibility (but marked as deprecated), which callsotel.SetLoggerAdapter
with the exportedlogr
adapter.The
Error
,Warn
,Info
andDebug
functions are kept ininternal/global
package, but it calls the configured logger adapter, so other code part should not be changed.A new adapter should be created for
log/slog
package, which can be passed tootel.SetLoggerAdapter
.Alternatives
Another alternatives don't keep backward compatibility.
Prior Art
N/A
Additional Context
N/A
The text was updated successfully, but these errors were encountered: