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

log: add func Default() *Logger #39057

Closed
carnott-snap opened this issue May 13, 2020 · 18 comments
Closed

log: add func Default() *Logger #39057

carnott-snap opened this issue May 13, 2020 · 18 comments

Comments

@carnott-snap
Copy link

Currently many packages contain global vars with a default implementation that is used for package functions, e.g. http.DefaultClient and http.Get. Internally, this is how the log package is structured with calls like log.Print forwarding to log.std, where var std = New(...).

Can we export the log.std symbol? This could be useful for globally configuring a custom logger, or simply passing around the default Logger without needing to call New(os.Stderr, "", LstdFlags) and hoping that still matches up with the stdlib implementation.

Proposed implementation
diff --git a/src/log/log.go b/src/log/log.go
--- a/src/log/log.go
+++ b/src/log/log.go
@@ -73,7 +73,8 @@ func (l *Logger) SetOutput(w io.Writer) {
 	l.out = w
 }
 
-var std = New(os.Stderr, "", LstdFlags)
+// Default is the Logger used by package functions like Fatal, Panic, Print, etc.
+var Default = New(os.Stderr, "", LstdFlags)
 
 // Cheap integer to fixed-width decimal ASCII. Give a negative width to avoid zero-padding.
 func itoa(buf *[]byte, i int, wid int) {
@@ -274,36 +275,36 @@ func (l *Logger) Writer() io.Writer {
 
 // SetOutput sets the output destination for the standard logger.
 func SetOutput(w io.Writer) {
-	std.mu.Lock()
-	defer std.mu.Unlock()
-	std.out = w
+	Default.mu.Lock()
+	defer Default.mu.Unlock()
+	Default.out = w
 }
 
 // Flags returns the output flags for the standard logger.
 // The flag bits are Ldate, Ltime, and so on.
 func Flags() int {
-	return std.Flags()
+	return Default.Flags()
 }
 
 // SetFlags sets the output flags for the standard logger.
 // The flag bits are Ldate, Ltime, and so on.
 func SetFlags(flag int) {
-	std.SetFlags(flag)
+	Default.SetFlags(flag)
 }
 
 // Prefix returns the output prefix for the standard logger.
 func Prefix() string {
-	return std.Prefix()
+	return Default.Prefix()
 }
 
 // SetPrefix sets the output prefix for the standard logger.
 func SetPrefix(prefix string) {
-	std.SetPrefix(prefix)
+	Default.SetPrefix(prefix)
 }
 
 // Writer returns the output destination for the standard logger.
 func Writer() io.Writer {
-	return std.Writer()
+	return Default.Writer()
 }
 
 // These functions write to the standard logger.
@@ -311,57 +312,57 @@ func Writer() io.Writer {
 // Print calls Output to print to the standard logger.
 // Arguments are handled in the manner of fmt.Print.
 func Print(v ...interface{}) {
-	std.Output(2, fmt.Sprint(v...))
+	Default.Output(2, fmt.Sprint(v...))
 }
 
 // Printf calls Output to print to the standard logger.
 // Arguments are handled in the manner of fmt.Printf.
 func Printf(format string, v ...interface{}) {
-	std.Output(2, fmt.Sprintf(format, v...))
+	Default.Output(2, fmt.Sprintf(format, v...))
 }
 
 // Println calls Output to print to the standard logger.
 // Arguments are handled in the manner of fmt.Println.
 func Println(v ...interface{}) {
-	std.Output(2, fmt.Sprintln(v...))
+	Default.Output(2, fmt.Sprintln(v...))
 }
 
 // Fatal is equivalent to Print() followed by a call to os.Exit(1).
 func Fatal(v ...interface{}) {
-	std.Output(2, fmt.Sprint(v...))
+	Default.Output(2, fmt.Sprint(v...))
 	os.Exit(1)
 }
 
 // Fatalf is equivalent to Printf() followed by a call to os.Exit(1).
 func Fatalf(format string, v ...interface{}) {
-	std.Output(2, fmt.Sprintf(format, v...))
+	Default.Output(2, fmt.Sprintf(format, v...))
 	os.Exit(1)
 }
 
 // Fatalln is equivalent to Println() followed by a call to os.Exit(1).
 func Fatalln(v ...interface{}) {
-	std.Output(2, fmt.Sprintln(v...))
+	Default.Output(2, fmt.Sprintln(v...))
 	os.Exit(1)
 }
 
 // Panic is equivalent to Print() followed by a call to panic().
 func Panic(v ...interface{}) {
 	s := fmt.Sprint(v...)
-	std.Output(2, s)
+	Default.Output(2, s)
 	panic(s)
 }
 
 // Panicf is equivalent to Printf() followed by a call to panic().
 func Panicf(format string, v ...interface{}) {
 	s := fmt.Sprintf(format, v...)
-	std.Output(2, s)
+	Default.Output(2, s)
 	panic(s)
 }
 
 // Panicln is equivalent to Println() followed by a call to panic().
 func Panicln(v ...interface{}) {
 	s := fmt.Sprintln(v...)
-	std.Output(2, s)
+	Default.Output(2, s)
 	panic(s)
 }
 
@@ -373,5 +374,5 @@ func Panicln(v ...interface{}) {
 // if Llongfile or Lshortfile is set; a value of 1 will print the details
 // for the caller of Output.
 func Output(calldepth int, s string) error {
-	return std.Output(calldepth+1, s) // +1 for this frame.
+	return Default.Output(calldepth+1, s) // +1 for this frame.
 }
@gopherbot gopherbot added this to the Proposal milestone May 13, 2020
@mvdan
Copy link
Member

mvdan commented May 15, 2020

You're getting a lot of downvotes, so I'll try to explain mine. You can already configure the global logger via SetFlags, SetOutput, and SetPrefix. You can also query some of those with funcs like like Flags, Writer, and Prefix.

This already covers pretty much all reasonable use cases of modifying the global logger. Exporting the variable would have multiple downsides:

  • Duplication of features; for example, log.SetOutput would be equivalent to log.Default.SetOutput
  • Pitfalls; what if I do log.Default = nil?
  • Inviting more data races. Replacing log.Default isn't safe if log messages could already be happening.

@carnott-snap
Copy link
Author

carnott-snap commented May 15, 2020

You implies that we should not have global default implementations, sorry if I am misreading you, but how is this different from net/http.DefaultClient, flag.DefaultUsage, net.DefaultResolver, go/build.Default? This is a pretty common pattern in the stdlib, and if the community does not think it is reasonable, we should deprecate all of these symbols.

The primary reason I am interested in this feature is the ability to (readonly) pass around the default log.Logger implementation for modularity/testing. I get that this opens up unsaftey, but that is due to the lack of const vars.

// in prod
t := thing.New(log.Default)
t.Do()

// in test
l := &mockLogger{}
t := thing.New(l)
t.Do()
l.Assert()

Today you would have to make a dummy implementation that just forwards calls:

type StdLogger struct{}
func (l StdLogger) Fatal(v ...interface{}) { log.Fatal(v) }
func (l StdLogger) Fatalf(format string, v ...interface{}) { log.Fatalf(format, v) }
// ...

Speaking to your concerns specifically:

  • Duplication of features; for example, log.SetOutput would be equivalent to log.Default.SetOutput

Yes, but http.Get is equivalent to http.DefaultClient.Get and nobody is complaining about its existence?

  • Pitfalls; what if I do log.Default = nil?

Do not. You can also modify os.Stdout, it does not mean you should. We can document the var to assist.

  • Inviting more data races. Replacing log.Default isn't safe if log messages could already be happening.

This is more a problem with global mutability, as you see the same issues with http.DefaultClient.

@jimmyfrasche
Copy link
Member

Maybe it would be safer with

package log
func Default() *Logger { return std }

but the other packages that use this pattern seem to be fine.

@carnott-snap
Copy link
Author

I am open to this option as well, but agree consistency is probably more important. Also, especially in testing, there are cases where you want to nop out the actual default Logger. I have mucked around with os.Stdout to know that there are some edge cases that need the sledgehammer.

@jfesler
Copy link

jfesler commented May 16, 2020

I very much understand the desire to pass a *log.Logger; and to use that instead of calling the default log directly. An example, optional logging in a package. Where the caller wants to use the default log. Best they can do is pass a new log object with the same parameters as the default, ie:
log.New(log.Writer(), log.Prefix(), log.Flags()). Unfortunately this does NOT share the private mu sync.Mutex.

@mvdan
Copy link
Member

mvdan commented May 22, 2020

The primary reason I am interested in this feature is the ability to (readonly) pass around the default log.Logger implementation for modularity/testing.

I don't agree that passing around a *log.Logger around is good for modularity. That requires all packages to adhere to the same logger implementation, which is not going to apply to the vast majority of Go software out in the wild.

I think the right solution is an interface, and there is a thread about that in #13182. I think we should focus the discussion and suggestions in that thread; it would make little sense to accept this proposal independently of that earlier one.

Regarding the API similarities with other packages - yes, other packages have similar pitfalls, but that doesn't mean they are OK to inherit :) Remember that a lot of packages in the standard library were designed over ten years ago, and they might still carry bad design decisions due to the Go1 compatibility guarantee. I would definitely say that net/http is not a good example to take ideas from, in general.

@carnott-snap
Copy link
Author

I don't agree that passing around a *log.Logger around is good for modularity. That requires all packages to adhere to the same logger implementation, which is not going to apply to the vast majority of Go software out in the wild.

That is not necessarily the case: you could wrap the default log.Logger into your custom interface. The goal of this change is to expose that reference, currently you just have methods, which have the noted sync.Mutex limitation.

I think the right solution is an interface, and there is a thread about that in #13182. I think we should focus the discussion and suggestions in that thread; it would make little sense to accept this proposal independently of that earlier one.

I am open to adding an interface too, it just seems orthogonal. I can still make my own interface and get the modularity I want with just the default var, but these two symbols seem to play together nicely, so maybe doing them at once is good: (or #13182 first)

var Default Interface = New(os.Stderr, "", LstdFlags)

type Interface interface { // or what ever name #13182 decides
        Fatal(...interface{})
        Fatalf(string, ...interface{})
        Fatalln(...interface{})
        Flags() int
        Output(int, string) error
        Panic(...interface{})
        Panicf(string, ...interface{})
        Panicln(...interface{})
        Prefix() string
        Print(...interface{})
        Printf(string, ...interface{})
        Println(...interface{})
        SetFlags(int) 
        SetOutput(io.Writer)
        SetPrefix(string)
        Writer() io.Writer
}

@rsc
Copy link
Contributor

rsc commented Jun 10, 2020

I'm skeptical that package log should define an interface, especially not the one in the comment above mine. A package that wants a logger should probably define a one-method interface that it expects. The interface above is no different from using log.Logger directly.

@rsc rsc added this to Incoming in Proposals (old) Jun 10, 2020
@carnott-snap
Copy link
Author

Thanks for clarifying things @rsc, it seems like #13182 has the interface discussion covered. It is interesting, but orthogonal to my proposed change, so I will try and not conflate things further.

@rsc rsc moved this from Incoming to Active in Proposals (old) Sep 18, 2020
@rsc
Copy link
Contributor

rsc commented Sep 23, 2020

To restate, the proposal here is to add

package log

// Default return the default Logger used by top-level function like Print, SetOutput, and so on.
func Default() *Logger

I don't see much enthusiasm for this in the comments and reactions above, but let's put it in the minutes and see if more people chime in.

@jimmyfrasche
Copy link
Member

I've certainly had code that could have been simpler with something like that.

@rsc
Copy link
Contributor

rsc commented Sep 30, 2020

@jimmyfrasche why would the code have been simpler? What is the use case? We still need specific examples to evaluate whether this is worth the added complexity.

@jimmyfrasche
Copy link
Member

To inject a logger into a program, so that it can be overridden in tests or by a flag, I need to reconstruct the default logger. With something like this proposal it could just be

func main() {
  realMain(log.Default())
}

@rsc
Copy link
Contributor

rsc commented Oct 7, 2020

It seems a bit redundant but is also consistent with what we've done elsewhere (importer.Default, build.Default, net.DefaultResolver) for exposing the default functionality used by top-level functions.

Does anyone object to adding func Default() *Logger to package log?

@rsc rsc changed the title proposal: log: export log.std as log.Default proposal: log: add func Default() *Logger Oct 7, 2020
@rsc
Copy link
Contributor

rsc commented Oct 14, 2020

Based on the discussion above, this seems like a likely accept

@rsc rsc moved this from Active to Likely Accept in Proposals (old) Oct 14, 2020
@earthboundkid
Copy link
Contributor

There are places where I declare a logger just so I have a struct to pass around. If this were available, I would call log.Default() instead.

@rsc
Copy link
Contributor

rsc commented Oct 21, 2020

No change in consensus, so accepted.

@rsc rsc moved this from Likely Accept to Accepted in Proposals (old) Oct 21, 2020
@rsc rsc modified the milestones: Proposal, Backlog Oct 21, 2020
@rsc rsc changed the title proposal: log: add func Default() *Logger log: add func Default() *Logger Oct 21, 2020
@gopherbot
Copy link

Change https://golang.org/cl/264460 mentions this issue: log: expose std via new Default function

@golang golang locked and limited conversation to collaborators Oct 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
No open projects
Development

No branches or pull requests

7 participants