-
-
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
Conversation
// use by multiple goroutines. For this implementation that means making | ||
// only one call to l.w.Write() for each call to Log. We first collect all | ||
// of the bytes into b, and then call l.w.Write(b). | ||
for i := 1; i < len(keyvals); i += 2 { |
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.
why is 1 here ?
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.
Because keyvals contains alternating keys (in even indices) and values (in odd indices). This loop only needs to operate on the values, so it can start at index 1.
@peterbourgon I plan to work on integrating this code into the real log package. I wanted to let you know to avoid duplicated effort. |
@ChrisHines Sure! I'll leave it alone for the weekend. |
Still a WIP. I kept The above commit allows lc := log.With(logger, "k1", "v2")
lc = lc.With("k2", "v2")
lc.Log("k3", "v3") Curiously, the following is equivalent to the above: lc := log.With(logger, "k1", "v2")
lc = log.With(lc, "k2", "v2") // use the package func instead of method on Context.
lc.Log("k3", "v3") Is the convenience worth the weight of having two ways to do the same thing? Also, should we add the following interface and define type WithLogger interface {
Logger
With(keyvals ...interface{}) WithLogger
} |
Responding to my above comment, maybe the Context constructor should look like: func NewContext(logger Logger) *Context Usage would then look like below, and it would avoid the overlap between the constructor and With method without increasing verbosity much. lc := log.NewContext(logger).With("k1", "v1")
lc = lc.With("k2", "v2")
lc.Log("k3", "v3") I've talked myself into this change already. Commit coming soon. |
Feedback:
|
@peterbourgon Thanks for the feedback. I think we already talked about your first two points at GopherCon and I'm happy with them. As for your last point, I might have agreed until I started writing the documentation. Eliding the keyvals from the constructor simplified the docs quite a bit. In general maintaining the separation of concerns feels to me like less is more in this case. It also means that you only have to look for one identifier when searching for places where context is added. I have a new issue to raise. I think we agreed last week that leveled logging should be split into its own package. I have started working on that change and have run into a problem. The design for leveled logging we worked on at the conference requires access to the unexported fields of |
…ontextual logging.
The new API ideas are fully integrated and ready for review. Please take a look. |
warnValue: "warn", | ||
errorValue: "error", | ||
critValue: "crit", | ||
} |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I have combined them.
I really like the feel of this. It looks like you've ported everything to package log, so I've deleted package log2. Minus that one comment about the Levels option struct, I'm good to go with this! |
// level. By default, the value is "crit". | ||
func CritValue(value string) Option { | ||
return func(c *config) { c.critValue = value } | ||
} |
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.
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 comment
The 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.
One final review and then I think we can merge this. |
I've repeated this reasoning a couple of times in chat, now. To pre-empt further questions, can we put this explanation as a doc comment, or comment on the appropriate method/type? |
Otherwise |
…heap WIth method at the expense of the Log method.
New API for package log (post-GopherCon) Fixes #63 and some usability concerns.
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 |
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:
- there is a way to suppress levels up to a minimum severity
- there is a way to configure a log sink for each log level (so that critical ones a teed to some alerting mechanism for example)
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 aware log.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.
New API for package log (post-GopherCon) Fixes go-kit#63 and some usability concerns.
—