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

server: support leveled logging #677

Closed
ericchiang opened this issue Nov 10, 2016 · 8 comments
Closed

server: support leveled logging #677

ericchiang opened this issue Nov 10, 2016 · 8 comments
Assignees
Milestone

Comments

@ericchiang
Copy link
Contributor

No description provided.

@ericchiang
Copy link
Contributor Author

ericchiang commented Nov 10, 2016

This may be a good time to switch to https://github.com/Sirupsen/logrus. The biggest question is if we use a global logger or a struct specific one (i'm in favor of the latter).

edit: for my own sanity, let's disable colors https://godoc.org/github.com/Sirupsen/logrus#TextFormatter

type Server struct {
    // Setup during call to NewServer. Defaults to only printing errors.
    logger logrus.FieldLogger
}

func (s *Server) debugf(format string, a ...interface{}) { s.logger.Debugf(format, a...) }
func (s *Server) infof(format string, a ...interface{})  { s.logger.Infof(format, a...) }
func (s *Server) errorf(format string, a ...interface{}) { s.logger.Errorf(format, a...) }

Probably the only levels we need to support.

Steps.

  • Replace all of our current log calls with errorf calls.
  • Centralize all error logging in the server package (remove other logging in the storage and connector packages).
  • Add lots of info calls.
  • Add debug calls for.
    • Every storage access (write a storage implementation in the server package that does this)
    • Every http request (write some middleware which does this)

@ericchiang
Copy link
Contributor Author

cc @rithujohn191

@ericchiang ericchiang changed the title server,storage: support leveled logging server: support leveled logging Nov 10, 2016
@ericchiang ericchiang added this to the v2.0.0 milestone Nov 10, 2016
@ericchiang
Copy link
Contributor Author

On second thought, storages need to be able to log things as well. We should probably pass a logger as part of storage creation.

@ericchiang
Copy link
Contributor Author

ericchiang commented Nov 17, 2016

Per conversation with @chancez we should support the JSON formatter too for log aggregation and filtering.

@rithujohn191:

To expand on the storage needing logging we want basically everything to embed a logger. Storages, connectors, the server, etc. Please, no global logger..

Probably want something like the following somewhere in cmd/dex

var level logrus.Level
switch config.Debug.Level {
case "debug":
    level = logrus.DebugLevel
case "", "info": // default to info
    level = logrus.InfoLevel
case "error":
    level = logrus.ErrorLevel
default:
    // handle error
}

var formatter logrus.Formatter
switch config.Debug.Format {
case "", "text": // default to text
    formatter = &logrus.TextFormatter{DisableColors: true}
case "json":
    formatter = &logrus.JSONFormatter{}
default:
    // handle error
}

logger := &Logger{
  Out:       os.Stderr,
  Formatter: formatter,
  Level:     level,
}

Then as we pass the logger to various components we want to add a new field

serverLogger := logger.WithField("component", "server")

Or if the server spins of a logger to the connectors.

connLogger := serverLogger.WithField("component", conn.ID)

Though maybe we can find something shorter than "component"?

@chancez
Copy link
Contributor

chancez commented Nov 17, 2016

That all looks great to me. You can use this https://godoc.org/github.com/Sirupsen/logrus#ParseLevel to get the log level from a string into a the level object though.

@chancez
Copy link
Contributor

chancez commented Nov 17, 2016

I also recommend you use this https://godoc.org/github.com/Sirupsen/logrus#FieldLogger as the object you use everywhere, rather than logrus.Logger, since that will allow you to use both logrus.Logger and logrus.Entry as your logger.

@ericchiang
Copy link
Contributor Author

You can use this https://godoc.org/github.com/Sirupsen/logrus#ParseLevel to get the log level from a string into a the level object though.

Thanks, though we don't want to support certain logging levels like panic, fatal, and warn. A switch statement is easy enough.

I also recommend you use this https://godoc.org/github.com/Sirupsen/logrus#FieldLogger as the object you use everywhere, rather than logrus.Logger, since that will allow you to use both logrus.Logger and logrus.Entry as your logger.

Yeah sounds good. Will update my examples.

@ericchiang
Copy link
Contributor Author

closed via #723

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

No branches or pull requests

3 participants