log/slog: change constructors to NewXXXHandler(io.Writer, *HandlerOptions)

[ianthehat]

There are two ways to constructors

slog.NewJSONHandler(os.Stdout)
slog.HandlerOptions{}.NewJSONHandler(os.Stdout)

The former cannot set any options, the latter is a method on the options.
I believe this is a workaround for not having function overloads, if we had them it would have been written

slog.NewJSONHandler(os.Stdout)
slog.NewJSONHandler(os.Stdout, slog.HandlerOptions{})

The trouble is, the method construction form is not discoverable. Our documentation tools do a good job of recognizing that free functions are constructors and showing them with the type the construct, but the method form is not that, it is a method on what is an unrelated type. If you go to pkgsite trying to work out how to make a slog.JSONHandler, it is not easy to even know that it is possible to set options on it.
It also blesses the two handlers in that package as special, when the HandlerOptions is a generally useful struct that can be accepted by many external handler implementations.
The downside of the transformed form is that you always have to have slog.HandlerOptions{} even to get the defaults.
Maybe that is okay, we don't expect that new handler construction will be very common, if it is not we could use the pointer form, so that it is

slog.NewJSONHandler(os.Stdout, nil) // for nil
slog.NewJSONHandler(os.Stdout, &slog.HandlerOptions{Level:slog.LevelInfo}) // with options

Either way I think the methods on slog.HandlerOptions should be removed in favor of free functions.

[jba]

Link to original slog proposal: #56345.

[jba]

@dsnet, I'd love your thoughts. I was partly inspired by your design of encoding/json v2.

[dsnet]

The way the v2 "encodingjson" prototype handles options is not something we particularly like and is subject to potential change.

In terms of API aesthetics, I personally like variadic options similar to how cmp.Equal operates.

The main reason why we didn't use variadic options for json.Marshal was for performance. It is occasionally the case that someone marshals incredibly tiny values (e.g., just a Go bool), where the amount of time spent iterating over a variadic list of options is a significant fraction of CPU time. It's on my TODO list to re-explore variadic options again to see if we can make it sufficiently performant, and we might switch to that (or some other options API).

Given that slog.NewJSONHandler is only called occasionally where you're not squeezing every nanosecond of performance out of the constructor, I don't think the concerns we were running into is applicable to your situation.

[dsnet]

slog.NewJSONHandler(os.Stdout, nil) // for nil

One objection I've often heard with this approach is that the presence of nil is often a code-smell when calling APIs. It legitimately makes one ask themselves: "Is it okay to pass nil here? How do I know that it doesn't lead to a nil-pointer panic later?". Even if the documentation of NewJSONHandler says it's okay, it's still a mental barrier that needs to be overcome.

[AndrewHarrisSPU]

The trouble is, the method construction form is not discoverable.

To this point, I feel like chainable methods on a configuration struct flow well into go doc or go.dev pages. I did like this approach when experimenting with slog. This is definitely a subjective conclusion (I think this is technically sound, but it's not the only way to do things) - I tried approaches more oriented around structs or variadic options, and I found chaining comfortable and capable, and the best for documenting.

To very quickly sketch that style of API, limited to slog:

// Opening a new logger configuration
func New() *Config

// Discrete configuration tasks
func (*Config) AddSource(bool) *Config
func (*Config) Ref(Leveler) *Config
func (*Config) Replace(func([]string, Attr) Attr) *Config
func (*Config) Writer(io.Writer) *Config

// Closing a configuration and producing a Logger
func (*Config) JSON() Logger
func (*Config) Text() Logger

Building a logger might look like:

// very fast to get up and running (if a default writer / ref of os.Stderr, INFO is sane enough)
log := slog.New().JSON()

// more detailed configuration, the per-line method calling survives "go fmt'ing"
log := slog.New().
	Replace(quirkyTimestamp).
	Ref(slog.DEBUG).
	Writer(os.Stdout).
	JSON()

[travbale]

Another solution could be to just name the constructors different things.

func NewJSONHandler(w io.Writer) *JSONHandler
func NewJSONHandlerWithOptions(w io.Writer, opts HandlerOptions) *JSONHandler

My thought is the current model is effectively two constructors already.

You just replace:

slog.HandlerOptions{}.NewJSONHandler(os.Stdout)

With:

slog.NewJSONHandlerWithOptions(os.Stdout, HandlerOptions{})

[dsnet]

@travbale, to be precise, you'll actually be replacing:

slog.HandlerOptions{}.NewJSONHandler(os.Stdout)

with

slog.NewJSONHandlerWithOptions(os.Stdout, slog.HandlerOptions{})

which is 17 characters longer unless we have #12854, in which case, it reduces down to:

slog.NewJSONHandlerWithOptions(os.Stdout, {})

which is 2 characters shorter.

[travbale]

@dsnet, Yep it would certainly is a bit more verbose so its not one to one. I know functional options could be done which is a pattern I like as well. Any implementation using functional options would increase the API surface area and would probably add more characters as well given each would be prefixed with slog so that would probably need to be considered if the constructors were to go that route. The benefit would be a single constructor.

[jba]

There is one example of XXXWithOptions in the standard library: Verify in https://pkg.go.dev/crypto/ed25519.

I prefer the terser "Opt" (in other words, NewJSONHandler(io.Writer) and NewJSONHandlerOpt(io.Writer, HandlerOptions). There are no public examples of that in the standard library, but a few private ones.

[jba]

Here's a example of the Opt form: https://pkg.go.dev/golang.org/x/net/http2#Transport.RoundTripOpt.

[travbale]

I personally think NewJSONHandlerOpt reads like you are making a new value for a type called JSONHandlerOpt rather than a new JSONHandler.

[jba]

Here are some reasons why I think that functional options are inferior to option structs:

1. It isn't clear what happens when there are duplicates.

2. They are wordy, because you have to type the package prefix before each one.

3. (minor) They clutter the godoc: it has a separate function for each option, instead of a single struct entry.

4. It's clumsier to combine sets of options. If you have a group of options as a slice and you want to add some for a call, you have to append, and if you don't clip the original you could have aliasing problems:

   common := []Option{a, b, c}
   f1 := newFoo(append(common, d)) 
   f2 := newFoo(append(common, e)) // could overwrite the location containing d
   

5. They have to share a namespace with the other top-level declarations. For example, what would we call the function that is equivalent to HandlerOptions.Level? We can't use Level or Leveler.

Their advantages are:

1. The no-option case is easier to use (or, you only need one function instead of two).
2. You don't have to figure out how to handle cases where the default option is not the zero value. That is, you can distinguish a missing option from one set to its default value.

[jba]

Here's another idea: keep the HandlerOptions struct but have the arg be ...HandlerOptions. Fail if there is more than one.

Problems:

1. No one ever does this.
2. Right now, NewJSONHandler and NewTextHandler have no error return, so you can put them directly inside slog.New. The new versions would either return an error, making them clumsier to use, or panic, which some might find too severe (although I think it's fine).

I sometimes wish that Go had a two-dot variadic argument that meant "zero or one".

[jba]

@AndrewHarrisSPU, I think your fluent-config design is elegant. But it isn't idiomatic Go. In particular, people would expect slog.NewHandler to return a Handler, but it would actually return a HandlerConfig that only turns into a Handler when you call JSON or Text.

It also has one of the problems of the current design: the built-in handlers have special status with respect to the config/option type, because they're the only ones with constructors that are methods. That seems mildly unfair to third-party handlers.

[ianthehat]

How about

func NewJSONHandler(w io.Writer, opts HandlerOptions) *JSONHandler
func NewDefaultJSONHandler(w io.Writer) *JSONHandler

and then if we ever get the improved type inference rules we can deprecate NewDefaultJSONHandler in favour of using NewJSONHandler(w, {})

[rsc]

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

[twmb]

I'd be +1 on a variadic approach, similar to cmp. A variadic options approach makes it easier to have defaults where the zero value could theoretically be significant.

[mvrhov]

Passing slog.HandlerOptions via *New* method would also allow to wrap both default handlers and override some methods.
ATM I have to copy full slog.JSONHandler just to add two additional fields to the Record when output is set to send data to logstash. Those 2 fields are totally irrelevant when logging to StdErr or StdOut

[jba]

@mrhov, can you elaborate on that? Why can't you wrap the handlers now?

[jba]

Here's another idea, mentioned at the end of the top post:

func NewJSONHandler(w io.Writer, opts *HandlerOptions) *JSONHandler

Yes, the tried-and-true, old-fashioned way. Its great advantages are that there is only one function, and it has precedent on its side. Almost anywhere you find an Options struct in the standard library, you will find a constructor that takes a pointer to it:

• https://pkg.go.dev/database/sql#Conn.BeginTx
• https://pkg.go.dev/image/jpeg#Encode
• https://pkg.go.dev/image/gif#Encode
• https://pkg.go.dev/net/http#PushOptions
• https://pkg.go.dev/net/http/cookiejar#New
• https://pkg.go.dev/crypto#Signer (the option type is an interface, so it can be nil)
• https://pkg.go.dev/crypto#Decrypter (ditto)

I personally have no problem with passing nil to get the defaults, although others feel differently. I think that when the reader sees nil with a New function, they can make a pretty good guess that default options are being passed. It also alerts them to the fact that options are available, which in most other designs is hard to discover from reading a call.

[08d2]

func NewJSONHandler(w io.Writer, opts HandlerOptions) *JSONHandler

seems preferable. Passing slog.HandlerOptions{} more clearly communicates what's going on versus nil. If a shorter-form constructor is desired, NewJSONHandler could take io.Writer and no options (defaulting to the default values of each field) and a separate e.g. NewJSONHandlerOpts constructor could take the opts field.

This is roughly equivalent to @ianthehat suggestion modulo constructor names.

[jba]

@08d2, one problem with your idea is that people interpret NewX as "make me an X", unless certain words like "With" or "From" appear in "X". So NewJSONHandlerOpts would suggest that it creates JSONHandlerOpts. This is the point that @travbale made above.

[jba]

I said above that no one ever uses a variadic argument for a single optional value. Not true; here's an example from the standard library: https://pkg.go.dev/go/doc#NewFromFiles.

[jba]

On balance, I think the good old-fashioned way is the best choice. I'll update the proposal.

[mvrhov]

@mrhov, can you elaborate on that? Why can't you wrap the handlers now?

I had a brainfart. I can also wrap it now but I have to proxy all methods. Embedding it and just overwriting handle method was what I had in mind and that also won't work with new New* method. Sorry for noise.

[08d2]

@08d2, one problem with your idea is that people interpret NewX as "make me an X", unless certain words like "With" or "From" appear in "X". So NewJSONHandlerOpts would suggest that it creates JSONHandlerOpts. This is the point that @travbale made above.

I don't think that NewJSONHandlerOpts implies that it creates a value of type JSONHandlerOpts. That seems like an overly-strict assumption about constructor naming conventions.

But I think it is reasonable to accept a *HandlerOpts as a final parameter to a constructor, so 👍 in the end.

[rsc]

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

[gopherbot]

Change https://go.dev/cl/486415 mentions this issue: log/slog: built-in handler constructors take options as a second arg

[rsc]

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group