-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
New API for package log (post-GopherCon) #76
Changes from all commits
3afb956
d3c703c
f2a9e2f
651b069
9043375
8a32db2
9044c97
38ea61a
8899b82
803da74
a50819e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,23 @@ | ||
package log_test | ||
|
||
import ( | ||
"os" | ||
|
||
"github.com/go-kit/kit/log" | ||
) | ||
|
||
func ExampleContext() { | ||
logger := log.NewLogfmtLogger(os.Stdout) | ||
logger.Log("foo", 123) | ||
ctx := log.NewContext(logger).With("level", "info") | ||
ctx.Log() | ||
ctx = ctx.With("msg", "hello") | ||
ctx.Log() | ||
ctx.With("a", 1).Log("b", 2) | ||
|
||
// Output: | ||
// foo=123 | ||
// level=info | ||
// level=info msg=hello | ||
// level=info msg=hello a=1 b=2 | ||
} |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,127 @@ | ||
package levels | ||
|
||
import "github.com/go-kit/kit/log" | ||
|
||
// Levels provides a leveled logging wrapper around a logger. It has five | ||
// levels: debug, info, warning (warn), error, and critical (crit). If you | ||
// want a different set of levels, you can create your own levels type very | ||
// easily, and you can elide the configuration. | ||
type Levels struct { | ||
ctx log.Context | ||
levelKey string | ||
|
||
// We have a choice between storing level values in string fields or | ||
// making a separate context for each level. When using string fields the | ||
// Log method must combine the base context, the level data, and the | ||
// logged keyvals; but the With method only requires updating one context. | ||
// If we instead keep a separate context for each level the Log method | ||
// must only append the new keyvals; but the With method would have to | ||
// update all five contexts. | ||
|
||
// Roughly speaking, storing multiple contexts breaks even if the ratio of | ||
// Log/With calls is more than the number of levels. We have chosen to | ||
// make the With method cheap and the Log method a bit more costly because | ||
// we do not expect most applications to Log more than five times for each | ||
// call to With. | ||
|
||
debugValue string | ||
infoValue string | ||
warnValue string | ||
errorValue string | ||
critValue string | ||
} | ||
|
||
// New creates a new leveled logger, wrapping the passed logger. | ||
func New(logger log.Logger, options ...Option) Levels { | ||
l := Levels{ | ||
ctx: log.NewContext(logger), | ||
levelKey: "level", | ||
|
||
debugValue: "debug", | ||
infoValue: "info", | ||
warnValue: "warn", | ||
errorValue: "error", | ||
critValue: "crit", | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since we already have a Levels struct, can we put these config params directly in it, and have the Option type take a Levels struct (pointer)? One less type to worry about. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I have combined them. |
||
for _, option := range options { | ||
option(&l) | ||
} | ||
return l | ||
} | ||
|
||
// With returns a new leveled logger that includes keyvals in all log events. | ||
func (l Levels) With(keyvals ...interface{}) Levels { | ||
return Levels{ | ||
ctx: l.ctx.With(keyvals...), | ||
levelKey: l.levelKey, | ||
debugValue: l.debugValue, | ||
infoValue: l.infoValue, | ||
warnValue: l.warnValue, | ||
errorValue: l.errorValue, | ||
critValue: l.critValue, | ||
} | ||
} | ||
|
||
// Debug logs a debug event along with keyvals. | ||
func (l Levels) Debug(keyvals ...interface{}) error { | ||
return l.ctx.WithPrefix(l.levelKey, l.debugValue).Log(keyvals...) | ||
} | ||
|
||
// Info logs an info event along with keyvals. | ||
func (l Levels) Info(keyvals ...interface{}) error { | ||
return l.ctx.WithPrefix(l.levelKey, l.infoValue).Log(keyvals...) | ||
} | ||
|
||
// Warn logs a warn event along with keyvals. | ||
func (l Levels) Warn(keyvals ...interface{}) error { | ||
return l.ctx.WithPrefix(l.levelKey, l.warnValue).Log(keyvals...) | ||
} | ||
|
||
// Error logs an error event along with keyvals. | ||
func (l Levels) Error(keyvals ...interface{}) error { | ||
return l.ctx.WithPrefix(l.levelKey, l.errorValue).Log(keyvals...) | ||
} | ||
|
||
// Crit logs a crit event along with keyvals. | ||
func (l Levels) Crit(keyvals ...interface{}) error { | ||
return l.ctx.WithPrefix(l.levelKey, l.critValue).Log(keyvals...) | ||
} | ||
|
||
// Option sets a parameter for leveled loggers. | ||
type Option func(*Levels) | ||
|
||
// Key sets the key for the field used to indicate log level. By default, | ||
// the key is "level". | ||
func Key(key string) Option { | ||
return func(l *Levels) { l.levelKey = key } | ||
} | ||
|
||
// DebugValue sets the value for the field used to indicate the debug log | ||
// level. By default, the value is "debug". | ||
func DebugValue(value string) Option { | ||
return func(l *Levels) { l.debugValue = value } | ||
} | ||
|
||
// InfoValue sets the value for the field used to indicate the info log level. | ||
// By default, the value is "info". | ||
func InfoValue(value string) Option { | ||
return func(l *Levels) { l.infoValue = value } | ||
} | ||
|
||
// WarnValue sets the value for the field used to indicate the warning log | ||
// level. By default, the value is "warn". | ||
func WarnValue(value string) Option { | ||
return func(l *Levels) { l.warnValue = value } | ||
} | ||
|
||
// ErrorValue sets the value for the field used to indicate the error log | ||
// level. By default, the value is "error". | ||
func ErrorValue(value string) Option { | ||
return func(l *Levels) { l.errorValue = value } | ||
} | ||
|
||
// CritValue sets the value for the field used to indicate the critical log | ||
// level. By default, the value is "crit". | ||
func CritValue(value string) Option { | ||
return func(l *Levels) { l.critValue = value } | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My DRY spidey sense is triggering here. I appreciate the performance implications of the following suggestion, and I'd love to see a benchmark to see if we could take the hit. type config struct {
key string
values map[string]string
}
var (
DebugValue = Level("debug")
InfoValue = Level("info")
WarnValue = Level("warn")
ErrorValue = Level("error")
CritValue = Level("crit")
)
func Level(level string) func(string) Option {
return func(value string) Option {
return func(c *config) { c.values[level] = value }
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Clever, but I feel too clever. For better or worse, godoc currently displays functions in the package index but not variables of function type. So the function version makes the available options much easier to discover for the casual user. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,55 @@ | ||
package levels_test | ||
|
||
import ( | ||
"bytes" | ||
"os" | ||
"testing" | ||
|
||
"github.com/go-kit/kit/log" | ||
"github.com/go-kit/kit/log/levels" | ||
) | ||
|
||
func TestDefaultLevels(t *testing.T) { | ||
buf := bytes.Buffer{} | ||
logger := levels.New(log.NewLogfmtLogger(&buf)) | ||
|
||
logger.Debug("msg", "résumé") // of course you'd want to do this | ||
if want, have := "level=debug msg=résumé\n", buf.String(); want != have { | ||
t.Errorf("want %#v, have %#v", want, have) | ||
} | ||
|
||
buf.Reset() | ||
logger.Info("msg", "Åhus") | ||
if want, have := "level=info msg=Åhus\n", buf.String(); want != have { | ||
t.Errorf("want %#v, have %#v", want, have) | ||
} | ||
|
||
buf.Reset() | ||
logger.Error("msg", "© violation") | ||
if want, have := "level=error msg=\"© violation\"\n", buf.String(); want != have { | ||
t.Errorf("want %#v, have %#v", want, have) | ||
} | ||
} | ||
|
||
func TestModifiedLevels(t *testing.T) { | ||
buf := bytes.Buffer{} | ||
logger := levels.New( | ||
log.NewJSONLogger(&buf), | ||
levels.Key("l"), | ||
levels.DebugValue("dbg"), | ||
) | ||
logger.With("easter_island", "176°").Debug("msg", "moai") | ||
if want, have := `{"easter_island":"176°","l":"dbg","msg":"moai"}`+"\n", buf.String(); want != have { | ||
t.Errorf("want %#v, have %#v", want, have) | ||
} | ||
} | ||
|
||
func ExampleLevels() { | ||
logger := levels.New(log.NewLogfmtLogger(os.Stdout)) | ||
logger.Debug("msg", "hello") | ||
logger.With("context", "foo").Warn("err", "error") | ||
|
||
// Output: | ||
// level=debug msg=hello | ||
// level=warn context=foo err=error | ||
} |
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way log levels are usually (in my practice) used:
How does it work with levels.Levels?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This Levels type is very basic and doesn't support any of those use cases directly. To get them, you'd need to implement a new Levels type, which I think would be straightforward. If you think your usage generalizes to a lot of other organizations, I'd be happy to review a PR to add it as an additional type in package levels!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was expecting we would add support for something along those lines in the future. Before we cram it all into
levels.Levels
though, consider if it can be achieved with a level awarelog.Logger
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure how to do it with the current implementation of log levels.
I would need to extract current log level from the context so I can switch on it etc.
Also, what is the semantics for setting log level multiple times?
(in case of JSON sink the last one wins, if it is a regular loggger they are all printed).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I understand you correctly, you wouldn't introspect the keyvals for a level key. Rather, you'd construct each leveled logger so that it's connected to the appropriate sink, and enabled/disabled according to the configured minimum severity.
If you supply multiple level keys, they will stack. Keyvals deliberately preserve order and don't dedupe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sasha-s Since this PR has already been merged I recommend that you write up the features you want as separate issues and we'll iterate on them.