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

Add logger to bbolt #509

Closed
3 tasks done
ahrtr opened this issue May 22, 2023 · 11 comments · Fixed by #624
Closed
3 tasks done

Add logger to bbolt #509

ahrtr opened this issue May 22, 2023 · 11 comments · Fixed by #624
Assignees
Milestone

Comments

@ahrtr
Copy link
Member

ahrtr commented May 22, 2023

Currently bbolt only returns error to caller. It should support printing log message as well.

We can follow the same way as raft does,

  • Define a Logger interface, and the caller can pass into any implementation they like.
  • bbolt implements a default implementation if users do not pass in any logger implementation.
  • Add log messages. e.g. all error cases should have an error message.
@serathius
Copy link
Member

I'm a little out of touch with golang slog, but it's better to take an existing logging interface then rollout your own. For picking:

@CaojiamingAlan
Copy link
Contributor

CaojiamingAlan commented May 22, 2023

Please assign it to me. I assume the first step is to add logs that are sufficient to trace each error, right?

I'm a little out of touch with golang slog, but it's better to take an existing logging interface then rollout your own. For picking:

I've go over the two links. Seems that slog is not intended to be used as a log interface, and there is an open issue for logr discussing implementing logr using slog, therefore I think logr is a better choice. I think we may use(or copy) https://github.com/go-logr/stdr as our default implementation.

@ahrtr
Copy link
Member Author

ahrtr commented May 23, 2023

Proposal 1:

Just as I mentioned in #509 (comment), we follow the same way as what we does for etcd and raft.

  • Define an simple Logger interface in bbolt.
  • Make very minor change in etcd, so that the existing Logger implementation for raft can be reused for bbolt as well.

The minor concern is there is duplicated Logger interface definition in both bbolt and raft. It should be extracted into a separate common repo.

The other concern is that it isn't structured log. It isn't a big problem for etcd, because the log will be encapsulated into JSON format by etcd.

Proposal 2:

  1. Introduce https://github.com/go-logr/logr into bbolt;
  2. Use the fnlogger as the default implementation if users do not provide a LogSink implementation. See example NewStdoutLogger;
  3. Add an simeple & thin adapter for uber-go/zap in etcd to implement the LogSink interface.

The good side is it's structured log.

The concern of this proposal is logr only has Error and Info, and Verbosity can be used to control the level of info log. It doesn't match uber-go/zap's level definition: debug, info, warn, error, panic and fatal. It means we need to add additional conversion between the two kinds of log Verbosity/level in etcd. For example, we can regard all level > 0 as debug log.

etcd already has a flag --log-level (see below) for uber-go/zap, we definitely don't want to add one more flag something like --log-verbose for go-logr/logr,

  --log-level 'info'
    Configures log level. Only supports debug, info, warn, error, panic, or fatal.

@cenkalti
Copy link
Member

A library should not output any logs by default. That is application's responsibility. We can provide a logger field that users can override with their own implementation.

@ahrtr
Copy link
Member Author

ahrtr commented May 24, 2023

A library should not output any logs by default.

Basically agree with this. Otherwise the bbolt CLI may output some unexpected log messages as well.

We can set the default logger to discardLogger, so that the logger always points to a valid instance, and accordingly we don't need to check nil pointer something like if lg != nil {...}.

@cenkalti
Copy link
Member

Proposal 3:

Use https://pkg.go.dev/golang.org/x/exp/slog as it is more likely to get adopted by the community and become a standard. The user can chose whatever handler he want want with it.

@ahrtr ahrtr added this to the v1.3.8 milestone Jul 24, 2023
@ahrtr ahrtr mentioned this issue Jul 24, 2023
6 tasks
@ahrtr
Copy link
Member Author

ahrtr commented Jul 31, 2023

We do not need fancy features, we just need a simple logger interface, so that users can provide whatever logger implementation they want. Note that etcd already uses go.uber.org/zap, and proposal 1 has already been proved that it works well with uber.

@ahrtr ahrtr modified the milestones: v1.3.8, v1.4.0 Aug 11, 2023
@Elbehery Elbehery mentioned this issue Nov 27, 2023
@ahrtr ahrtr closed this as completed in #624 Dec 1, 2023
@ahrtr ahrtr reopened this Dec 7, 2023
@Elbehery
Copy link
Contributor

Elbehery commented Dec 7, 2023

what happened ?

@ahrtr
Copy link
Member Author

ahrtr commented Dec 7, 2023

what happened ?

We also need to add log messages. I am doing it. Also read #509 (comment)

@Elbehery
Copy link
Contributor

@ahrtr hello ✋🏽

can u push the changes you made to log messages, and i will continue ?

@ahrtr ahrtr mentioned this issue Dec 19, 2023
@ahrtr
Copy link
Member Author

ahrtr commented Dec 29, 2023

Done.

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

Successfully merging a pull request may close this issue.

5 participants