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

Use slog in a backward compatible way #442

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Use slog in a backward compatible way #442

wants to merge 1 commit into from

Conversation

dennwc
Copy link
Contributor

@dennwc dennwc commented Apr 10, 2024

Go SDK will be used in other projects, thus it makes sense to use standard structured logging instead of our own one.

livekit/protocol#668 added support for slog to the protocol's logger package. This change propagates it to the SDK.

For our own projects, calling logger.SetLogger will automatically set slog logger as well, making this change backward-compatible.

Also, add SetLogger to the most important structs of the package, allowing to associate additional metadata with RTC logs.

Copy link
Member

@davidzhao davidzhao left a comment

Choose a reason for hiding this comment

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

Did we have to change all of the logger usage internally to slog?

IMO it makes sense to use it as the interface, and when interfacing with external clients. But using slog internally in the code base seems to be a step backwards for the following reasons:

  • It breaks a certain consistency within our Go codebase. Everywhere else we are using protocol logger
  • We would lose features, particularly with WithValues, WithComponent, etc

@dennwc
Copy link
Contributor Author

dennwc commented Apr 10, 2024

Did we have to change all of the logger usage internally to slog?

Only in the SDK, yes.

To be clear, using logger still works as before for LK projects (it sets both logger and slog). But for external users of SDK it still requires passing an implementation of logger.Logger, which this PR tries to address.

Conversion is currently only one way slog -> logger, not the opposite. This is because both Zap and Logr already provide slog adapters. When SDK logs something in LK projects it goes directly to (LK-configured) Zap.

If we want to allow passing custom slog implementations to our logger that will need additional adapters in protocol.

It breaks a certain consistency within our Go codebase. Everywhere else we are using protocol logger

This depends on if we will adopt it long term. It could be just a temporary inconsistency.

In practice (if I understand correctly), we mostly use Zap under the hood, which has slog adapter as well. So in general we could decide to move to slog as a main log interface eventually. That would remove the need to maintain our own logger interface. There are too many loggers in the wild already 😅

But that's just as an option, it's not directly related to the PR.

We would lose features, particularly with WithValues, WithComponent, etc

WithValues is supported: slog.Logger.With. WithComponent could be a wrapper for With("some-predefined-key", name+component) in our logger package. I will make sure it plays nicely with Zap if we decide to switch.

We do lose Zap-specific things like WithItemSampler. But I think it could also be a wrapper in logger package that asserts that the slog handler is Zap and reconfigures it accordingly. Something along the lines of:

package logger

func WithItemSampler(log *slog.Logger) *slog.Logger

If the underlying logger is not Zap it will just return the same logger with no changes. This will work for Logr, for example, but also for any other log package out there.

So it's definitely worth a separate discussion about the long term plans. I could make a separate DD/proposal with all the details that we can discuss.

But for now this PR only changes client-facing SDK to use the logger from Go stdlib. I think eventually most libraries will converge to using that interface anyway, so I wanted to see how invasive it would be to switch. Turns out it's not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants