Skip to content

feat: WithLogger Provider Option#833

Merged
mfridman merged 1 commit intopressly:masterfrom
vfoucault:feat/new_provider_option_loggeer
Oct 19, 2024
Merged

feat: WithLogger Provider Option#833
mfridman merged 1 commit intopressly:masterfrom
vfoucault:feat/new_provider_option_loggeer

Conversation

@vfoucault
Copy link
Copy Markdown
Contributor

@vfoucault vfoucault commented Oct 8, 2024

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

@vfoucault
feat: WithLogger Provider Option
1aefc18 
* ability to respect the current behavior of `SetLogger` for Goose Providers
@mfridman
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

vfoucault 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.

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

mfridman commented Oct 14, 2024

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

vfoucault commented Oct 14, 2024

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

mfridman commented Oct 19, 2024 •
edited
Loading

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 mentioned this pull request Oct 19, 2024
Closed
@mfridman mfridman merged commit 1e26652 into pressly:master Oct 19, 2024
@vfoucault vfoucault deleted the feat/new_provider_option_loggeer branch November 5, 2024 16:29
@BrewTestBot BrewTestBot mentioned this pull request Nov 21, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mfridman mfridman mfridman approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@vfoucault @mfridman