pressly / goose

A database migration tool. Supports SQL migrations and Go functions.
http://pressly.github.io/goose/
Other
7.16k stars 523 forks source link

feat: WithLogger Provider Option #833

Closed vfoucault closed 1 month ago

vfoucault commented 1 month ago

Currently goose provides a SetLogger method to overload the goose logger. Goose providers doesn't provide such mechanism, leading to a painful use to do full structured logging when embedding goose into a custom package.

how to use (with zerolog)

type MyLogger struct {
    l *zerolog.Logger
}

func NewMyLogger(l *zerolog.Logger) *MyLogger {
    return &MyLogger{l: l}
}

func (m MyLogger) Fatalf(format string, v ...interface{}) {
    m.l.Fatal().Msgf(format, v...)
}

func (m MyLogger) Printf(format string, v ...interface{}) {
    m.l.Info().Msgf(format, v...)
}

func RunStuff() {
        log.Info().Msgf("Starting goose stage: creating custom logger")
        l := NewMyLogger(&log.Logger)
        fsObj := os.DirFS("./localtest2/migrations")
    p, err := goose.NewProvider("clickhouse", dbConn, fsObj, goose.WithVerbose(true), goose.WithLogger(l))
    if err != nil {
        log.Fatal().Err(err).Msg("Failed to create provider")
    }
    res, err := p.Up(context.TODO())
    if err != nil {
        log.Fatal().Err(err).Msg("Failed to up")
    }
}

which outputs:

{"level":"info","time":"2024-10-08T09:15:54+02:00","message":"Starting goose stage: creating custom logger"}
{"level":"info","time":"2024-10-08T09:15:54+02:00","message":"goose: OK    up 00005_testme.sql (73.44ms)"}
{"level":"info","time":"2024-10-08T09:15:54+02:00","message":"goose: successfully migrated database, current version: 5"}
{"level":"info","time":"2024-10-08T09:15:54+02:00","message":"Migration applied: OK    up 00005_testme.sql (73.44ms)"}
mfridman commented 1 month ago

Ah yes, we'll for sure add this.

The only reason I was delaying this as much as possible was to figure out the UX around it. Should we continue to use the Logger interface, or use *slog.Logger or maybe use a Logger interface and expose a helper to convert an *slog.Logger.

vfoucault commented 1 month ago

Ah yes, we'll for sure add this.

The only reason I was delaying this as much as possible was to figure out the UX around it. Should we continue to use the Logger interface, or use *slog.Logger or maybe use a Logger interface and expose a helper to convert an *slog.Logger.

Indeed, a slog.Logger implemented could be a good thing, though it requires us (everybody) to have a structured logging system that is compliant with slog.logger. I must admit that I don't know much about it. I maybe push things to another brighter day 😅

mfridman commented 1 month ago

I took a stab at exposing an *slog.Logger instead of the interface (which predates slog by many years).

Do you think #836 would be sufficient?

It also means you have a bit more control over the output, although i doubt most applications really care.

vfoucault commented 1 month ago

I took a stab at exposing an *slog.Logger instead of the interface (which predates slog by many years).

Do you think #836 would be sufficient?

It also means you have a bit more control over the output, although i doubt most applications really care.

Hi there, honestly this looks the right approach to tend towards a standard logging api, but it would require all users (including me) to move their logging towards a slog compliant approach. This would be a switch the hard way. Having maybe both option would lean towards a easier a smoother switch.

I don't know yet much about slogger, I saw that there are some bridges between zerolog and slog. The thing that I wish not to re implement would be logging company wide.

kr.

mfridman commented 1 month ago

Thanks for the feedback. I'm okay adding the interface-based Logger in the /v3 of goose.

In the future I'll need to revisit logging, either by extending the package with *slog.Logger or cutting a major version which moves forward with *slog.Logger and drops the Logger interface entirely.