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

Bring back the context logger #93

Merged
merged 4 commits into from
Oct 12, 2017
Merged

Conversation

heyLu
Copy link
Contributor

@heyLu heyLu commented Oct 12, 2017

As discussed in #85, this brings back the "context logger" that used to be part of oxy.

This allows passing a custom logger to oxy that can be changed as appropriate for applications.

This starts with the forward package. Let's review that first, and when we're happy with the design I'll add similar changes for the other packages to this PR.

Usage is currently as follows:

package main

import (
  "fmt"

  "github.com/vulcand/oxy/forward"
  "github.com/sirupsen/logrus"
)

func main() {
  logger := logrus.New()
  logger.SetLevel(logrus.WarnLevel)
  fwd, _ := forward.New(forward.Logger(logger))
  fmt.Println(fwd)
}

@heyLu heyLu mentioned this pull request Oct 12, 2017
forward/fwd.go Outdated
@@ -130,7 +141,7 @@ type UrlForwardingStateListener func(*url.URL, int)
// New creates an instance of Forwarder based on the provided list of configuration options
func New(setters ...optSetter) (*Forwarder, error) {
f := &Forwarder{
httpForwarder: &httpForwarder{flushInterval: time.Duration(100) * time.Millisecond},
httpForwarder: &httpForwarder{flushInterval: time.Duration(100) * time.Millisecond, log: log.New()},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer this to be: log.StandardLogger()

Which is where all direct logging calls to the log package get routed to. That will preserve any behavior where someone has modified the StandardLogger globally and expects the current behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that sounds good! Incidentally, that would solve some of our problems, too. :)

This means the behaviour will be the same as before, e.g. global
logrus settings will be set on the forward logger by default.
@archisgore
Copy link

I'm good with this.

@archisgore archisgore merged commit a5d1633 into vulcand:master Oct 12, 2017
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

Successfully merging this pull request may close these issues.

2 participants