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

Wrapping into the standard logger #7

Closed
zonescape opened this issue Mar 21, 2019 · 4 comments
Closed

Wrapping into the standard logger #7

zonescape opened this issue Mar 21, 2019 · 4 comments

Comments

@zonescape
Copy link
Contributor

The problem

There is a field ErrorLog *log.Logger in Go's http.Server. From docs: "ErrorLog specifies an optional logger for errors accepting connections, unexpected behavior from handlers, and underlying FileSystem errors. If nil, logging is done via the log package's standard logger."

Example code that triggers logging:

package main

import (
	"net/http"
)

func handler(w http.ResponseWriter, r *http.Request) {
	w.WriteHeader(200)
	w.WriteHeader(404)
}

func main() {
	http.ListenAndServe(":8080", http.HandlerFunc(handler))
}

If we run above server and issue this command:

$ http :8080

then the server prints this error message:

2019/03/21 14:30:40 http: superfluous response.WriteHeader call from main.handler (main.go:9)

Propose

Maybe it makes sense to implement lgr.Logger wrapping into the standard logger.

Thoughts about implementation

Basically the only thing we need to implement is a custom io.Writer. Then we can pass it to the log.Logger's constructor or set it up via SetOutput() function or method. But some problems arise if we do caller logging. Even if we skip all call frames related to log package then we still be somewhere inside net/http package. If we skip more frames then we may end up in our code (unexpected behavior from handlers, as in example) but may be not (errors accepting connections, FileSystem errors). So it may be worth to log only some generic caller info like {net/http}. This info may be passed as an argument to a method/function that wraps lgr.Logger.

@umputun
Copy link
Member

umputun commented Mar 21, 2019

passing a writer to SetOutput will make some strange beast. For example, it will add all log.Logger extras (whatever defined in L-flags) and also lgr extras.

I think the wrapper should be from the opposite side, but such wrapper probably impossible to do because ErrorLog is not an interface.

@zonescape
Copy link
Contributor Author

You are right that there is no good solution. We can do something that is better than nothing only. Note that I don't propose to implement general solution but the specific solution for use with http.Server.

it will add all log.Logger extras (whatever defined in L-flags)

When creating an instance of log.Logger we can set up all flags as we want to. I think about interface like this:

// It is assumed that the returned logger will not be further adjusted
// (i.e. (*Logger).SetOutput() method will not be called).
func (*Logger) ToStd(prefix, callerInfo string) *log.Logger {}

If you think that we should leave http.Server as is (i.e. let it logging into stdout in a format unrelated to lgr.Logger) then simply close this issue.

@umputun
Copy link
Member

umputun commented Mar 22, 2019

I don't have a strong opinion on this one. The use case is narrow and even exotic, but probably worth to be covered. I see why we want to pass prefix (probably can be optional and defaulted to WARN) but not so sure about callerInfo for the same reason discussed in #8

umputun added a commit that referenced this issue Mar 22, 2019
@umputun
Copy link
Member

umputun commented Mar 22, 2019

I have added a pair of trivial wrappers for io.Writer and *log.Logger (branch adaptor).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants