Proposal: add custom logger support

[icamys]

Hi! First of all, that you for implementing this package!

I've noticed that for logging errors the log package is used, concretely two functions log.Printf and log.Fatalf. There are a couple of issues with this approach:

1. There's no simple way to inject a custom logger and the only way to control the output is to inject the custom logger in the log package itself, which is not a good solution. Ideally, we should be able to use any logger we prefer.
2. log.Fatalf terminates the program, which is not always the preferred behaviour.

I'd propose allowing the caller provide the logger implementation on Server constructor and use the log package by default. Here's an example of what I'd would do.

1. Since both log.Printf and log.Fatalf are used to print errors, we define a Logger interface to replace them both:

   // Logger is the minimal interface SCIM Server needs for logging. Note that
   // log.Logger from the standard library implements this interface, and it is
   // easy to implement by custom loggers, if they don't do so already anyway.
   type Logger interface {
   Println(v ...interface{})
   }

2. Add an ErrorLogger field to the Server:

   type Server struct {
   Config        ServiceProviderConfig
   ResourceTypes []ResourceType
       ErrorLogger Logger // <---
   }

3. When creating the Server, pass any instance of Logger. Here's an example of implementation based on slog logger:

   
   // SlogErrorLogger implements the 'scim.Logger' interface
   type SlogErrorLogger struct {
   Log *slog.Logger
   }

func (l *SlogErrorLogger) Println(v ...interface{}) { for _, msg := range v { l.Log.Error(fmt.Sprint(msg)) } }

func main() { // set up the slog.Logger instance // slogLogger := ...

    l := &SlogErrorLogger{Log: slogLogger}
    // probably it's worth to replace this
    // with a constructor + functional options and make server fields private
    // since we break the server creation contract anyway
    s := Server{  
            ErrorLogger: l,
    }

}

4. Inside the scim package, replace `log.Printf` and `log.Fatalf` calls with the `Logger`.

Any thoughts on this?

[q-uint]

Hi @icamys!

Thanks for opening an issue for this! The server provides no logging interface by design. Of course this is open for discussion.

There are two moments where the server logs:

1. When it was not able to marshal the response, it will log the error. This should not happen, since these are predefined structures, of which most have custom MarshalJSON methods.
2. When the server was not able to Write the response.

The only reason for a logger would be to make sure they do not just go to stout. This can already be done with log.SetOutput.

[icamys]

@q-uint Thanks for the response. I understand the reasoning behind the low logger usage. My proposal is more about providing a choice of the logger that should be used by this package. When the server uses the log package, there's an assumption that a global logger instance inside the log package is already configured somewhere earlier in the application. But that's not always the case. In some applications (like mine) the logger is passed as a dependency to object constructors and doesn't use global variables. If instead of implicitly using the log package the server could use an instance passed via a Logger interface, it would remove an implicit dependency from the log package and provide more freedom to the developers to choose what logging facility they want to use.

Just wanted to make clear, that this approach is not something exotic. It is widely used in the wild:

1. Official elasticsearch client library uses the logger interface pattern.
2. Prometheus client logger interface