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

Jump to bottom

net/http/httputil: ReverseProxy.ErrorLog should accept an interface #21082

Closed
kedare opened this issue Jul 19, 2017 · 7 comments
Closed

net/http/httputil: ReverseProxy.ErrorLog should accept an interface #21082

kedare opened this issue Jul 19, 2017 · 7 comments
Labels
FrozenDueToAge v2 A language change or incompatible library change

Comments

@kedare
Copy link

kedare commented Jul 19, 2017

What version of Go are you using (go version)?

go version go1.8.3 darwin/amd64

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"

What did you do?

Tried to use a custom logger on ReverseProxy.ErrorLog (like logrus)

What did you expect to see?

A program compiling fine and logging going through my logger from logrus

What did you see instead?

Compilation failed as ReverseProxy.ErrorLog has to be bound to a log.Logger.

ReverseProxy.ErrorLog should accept an interface instead, to allow using any compliant logging system.

Related to : sirupsen/logrus#588
@mvdan mvdan added the v2 A language change or incompatible library change label Jul 19, 2017
@mvdan
Copy link
Member

mvdan commented Jul 19, 2017

Backwards compatibility can't be broken, so marking as Go2.

@bradfitz
Copy link
Contributor

https://golang.org/pkg/log/#Logger.SetOutput

@kedare
Copy link
Author

kedare commented Jul 19, 2017 •
edited

mvdan: But if we use an interface that log.Logger matches, in theory it should not break anything, right ?

@mattn
Copy link
Member

mattn commented Jul 19, 2017 •
edited

Possibly breaking compatibility.

proxy.ErrorLog = makeLogger()
passLogger(proxy.ErrorLog)

makeLogger may have return typed *log.Logger. argument of passLogger is typed it.

@mvdan
Copy link
Member

mvdan commented Jul 19, 2017

If the signature changes, that breaks the Go1 compatibility promise. @mattn showed the outline of a reasonable program that would break.

@ian-kent
Copy link
Contributor

ian-kent commented Jan 4, 2018 •
edited

could we do something similar to how Cancel was deprecated in favor of Context in net/http.Request?

e.g., Logger.SetOutput maintains backwards compatibility and something like Logger.SetOutputFunc(f func(format string, args ...interface{})) with a new conditional to handle it here:

go/src/net/http/httputil/reverseproxy.go

Line 325 in 5ea2360

} else {

edit: currently have an issue with this, proposed solution for us is likely to be duplicating reverseproxy.go to make the changes we need

@ianlancetaylor
Copy link
Contributor

This should be handled as part of #5465. Closing in favor of that issue.

@ianlancetaylor ianlancetaylor mentioned this issue Feb 27, 2018
Open
@golang golang locked and limited conversation to collaborators Feb 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge v2 A language change or incompatible library change
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@bradfitz @mattn @kedare @ian-kent @ianlancetaylor @mvdan @gopherbot