We should also set the logger for SQL?

Added

To reduce duplication what about having something like ?

// internal/logging/logger.go
package logging

import "log/slog"

func GetLogger(logger *slog.Logger) *slog.Logger {
[...]
}

func SetLogger(newLogger *slog.Logger) *slog.Logger {
[...]
}

or maybe go even further if we accept this (also with the nil as slog.Default()).

type Holder struct {
    logger *slog.Logger
}

func (h *Holder) Logger() *slog.Logger {
    if h.logger != nil {
        return h.logger
    }
    return slog.Default()
}

func (h *Holder) SetLogger(newLogger *slog.Logger) {
    if newLogger == nil {
        h.logger = slog.Default()
        return
    }
    h.logger = newLogger
}

Good suggestion. I assume you are suggesting using Holder as a struct embedding, right? Like so

type MyThing struct {
    Holder
}

func (m *MyThing) Do() {
    m.Logger().Info("Doing something")
}

Same comment as before. What about using slog.Default() when the logger is nil, and requiring users to explicitly pass something like slog.New(slog.NewJSONHandler(io.Discard, nil)) if they want to discard logs?

I had that in the begin then I changed my mind. When consuming an API like this, I find passing nil i.e the default value of a pointer is synonymous to "unset my value". The effect of "unsetting" a logger is not to log anything.

I'm curious, why are we not using a *slog.Logger directly here? If I use the SDK example and adapt it with your code, I don't see the need to use and atomic pointer.

func runUpgrade(....) error {
    upgradeLogger := slog.New(slog.NewJSONHandler(os.Stdout, nil)).With(
        slog.String("request_id", "abc-123"), 
    )
    actionConfig := action.NewConfiguration()
    actionConfig.SetLogger(upgradeLogger)
    // [...]
    upgradeClient := action.NewUpgrade(actionConfig) // then I have my logger per action 🤔 

Maybe I’m missing some scope detail or other considerations you had in mind?

This is to make sure SetLogger and Logger are thread safe

I’m wondering in which scenario someone would actually modify the logger during execution. 🤔
If we typically set it once in Configuration (for example, at action initialization), I don’t see a case where concurrent reads and writes would happen:

cfg := action.NewConfiguration()

// Goroutine 1: Constantly changing logger
go func() {
    for {
        cfg.SetLogger(logger1)  // Writing
    }
}()

// Goroutine 2: Reading logger
go func() {
    for {
        cfg.Logger().Info("test")  // Reading
    }
}()

Wouldn’t this kind of pattern be unusual in practice? It feels like we’d only set the logger once and reuse it across actions.

Wouldn’t this kind of pattern be unusual in practice? It feels like we’d only set the logger once and reuse it across actions.

Its unusual to do this for sure, but given that the cost of implementing thread safety is low, why not ensure its there? If performance is a concern, using atomic.Pointer ensure we have lock-free thread safety so the cost is negligible.

Option parameters might be better than this SetLogger() pattern?

ie.

type ConfigurationOption func(c *Configuration)

// Override the default logging handler
// If unspecified, the default logger will be used
func ConfigurationSetLogger(h *slog.Handler) ConfigurationOption {
    return func(c *Configuration) {
       c.logger = slog.New(h)
    }
}

func NewConfiguration(options...ConfigurationOption) {
    c := &Configuration{}
    for _, o := options {
       o(c)
    }
    
     if o.logger == nil {
        o.logger = slog.Default()
     }

this would assume that the user can set the logger when creating the action Configuration

(and doesn't want to change it later; but would be a somewhat "more idiomatic" or "cleaner" approach, perhaps)

Done. It allows us to add more options in future

I think we need the if c.logger == nil { c.SetLogger(slog.Default()) } here?

Otherwise nothing will se tthe logger, and Logger() will default to the discard handler.

Oh, yes. Good catch.

Hmm. logger would never be nil since it's not a pointer type

type LogHolder struct {
	logger atomic.Pointer[slog.Logger]
}

The atomic value/pointer is nil:

if c.logger.Load() == nil { c.SetLogger(slog.Default()) }

It would be nice if this would default to slog.Default().Handler() so this doesn't have to call SetLogger all over the place. There could be a follow up to do this if anyone else thinks this is a good idea.

this does sound useful as a follow-up IMO

@banjoh There also used to be a method for passing a logger to the pkg/storage.Storage. Now the way you can do it is by passing a driver.Driver to pkg/storage.Init() that implements this LoggerSetterGetter interface. Can this interface be exposed, please? 🙏

Sorry, a bit too late, but there's a bug here. Line 344 should be s.SetLogger()