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

proposal: log: export log.std as log.Default #39057

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

proposal: log: export log.std as log.Default #39057

carnott-snap opened this issue May 13, 2020 · 9 comments
Labels
Projects
Milestone

Comments

@carnott-snap
Copy link

@carnott-snap carnott-snap commented May 13, 2020

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
@gopherbot gopherbot added the Proposal label May 13, 2020
@mvdan
Copy link
Member

@mvdan 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 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

@jimmyfrasche jimmyfrasche commented May 15, 2020

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

@carnott-snap carnott-snap commented May 15, 2020

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

@carnott-snap carnott-snap commented May 26, 2020

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 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 Jun 10, 2020
@carnott-snap
Copy link
Author

@carnott-snap carnott-snap commented Jun 10, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Incoming
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.