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

feat: WithLogger Provider Option #833

Merged
merged 1 commit into from
Oct 19, 2024

Conversation

vfoucault
Copy link
Contributor

  • ability to respect the current behaviour of SetLogger for Goose Providers

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)"}

* ability to respect the current behavior of `SetLogger` for Goose Providers
@mfridman
Copy link
Collaborator

mfridman commented Oct 8, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

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
Copy link
Collaborator

mfridman commented Oct 19, 2024

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.

@mfridman mfridman merged commit 1e26652 into pressly:master Oct 19, 2024
4 checks passed
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