Initial commit #1
Conversation
| type StandardLogger interface { | ||
| Print(...interface{}) | ||
| Printf(string, ...interface{}) | ||
| Printfln(...interface{}) |
There was a problem hiding this comment.
Can this be the default with Printf so we don't need the *ln versions?
There was a problem hiding this comment.
Printf will already do a newline in Go, no?
The reason for the *ln family is because they are part of the standard logger. Logrus must implement them so you can easily replace the stdlib logger.
|
Looks good at a first glance, I'll read this more carefully later. However, I do want to bring up one point that I think we should be thinking about: Do we actually want an extra library here? Logging is something Go is okay at already, and the minimalism of the "Go way" is generally nice. Not against it; I just think we should have this discussion. Every dependency is another brick in the mental model of a project. |
|
Have a look at https://github.com/golang/glog/ if you want ideas for the log level stuff. |
| } else { | ||
| entry.logger.mu.Lock() | ||
| io.Copy(entry.logger.Out, reader) | ||
| entry.logger.mu.Unlock() |
|
I agree with @burke the idea looks fine, but do we really need a more "heavy duty" logger? |
|
@burke Glad you brought that up, I'd love to have that discussion! Mostly, my motivation for this library is to replace all our I'm very open to see if we can figure out a better way, I just think out current logging could be better. I've made it compatible with the current API, so the official documentation applies here too and it can function as a drop-in. @camilo re: JSON. I'd like to use JSON because Splunk has an easy time passing it, so your |
|
Another library I think it'd be nice to have is a |
| } | ||
|
|
||
| // TODO: Other formats? | ||
| func (entry *Entry) Reader() (read *bytes.Buffer, err error) { |
There was a problem hiding this comment.
read is never assigned, err could be declared with serialized.
|
Also, I've been finding that pretty-printed JSON takes up a kinda unreasonable number of log lines, and makes logs a lot harder to skim than a single-line with hand-crafted format. |
| // TODO: new() should spawn an airbrake goroutine and this should send to | ||
| // that channel. This prevent us from spawning hundreds of goroutines in a | ||
| // hot code path generating a warning. | ||
| go entry.airbrake(reader.String()) |
There was a problem hiding this comment.
it almost feels like you should have an airbrake goroutine always reading from a channel, when 💩 hits the fan this will happen a lot
There was a problem hiding this comment.
Look a few lines up Camilobabes. There's a comment saying exactly this :)
|
I agree Burke. I'm going to play with the formatting a bit. |
This is sort of what I want the log output to look like.
| Logrus has no notion of environment. If you wish for hooks and formatters to | ||
| only be used in specific environments, you should handle that yourself. For | ||
| example, if your application has a global variable `Environment`, which is a | ||
| string representation of the environment you could do: |
There was a problem hiding this comment.
@sirupsen I'm assuming this is just some stale info. There's an Environment variable in the library:
https://github.com/Sirupsen/logrus/pull/1/files#diff-1dbd7db4a9a8f38a342128eacd61f656R21
|
@sirupsen The library looks really good and clean. I just added a few minor line notes. The only other change is the airbrake dependency but I saw you mentioned that in the PR description. |
|
@benbjohnson the library code isn't up to date from the README, code needs a cleanup and a good bunch of changes to comply. Does the API makes sense do you? Would you find this library useful? |
|
@sirupsen The API makes sense to me and it's clean. The stdlib log package works well for me for most projects but I tend to have smaller, standalone projects where the additional dependency is a bigger concern. I can see this library being really useful for internal Go projects. |
|
That's definitely the goal. Thanks for your feedback @benbjohnson, I'll implement this soon. :) |
Initial commits
Meet
logrus. Proposal for a standard Shopify Go logger. Features:@Shopify/stack: @wvanbergen @burke @graemej @justinplouffe
Other Go users at Shopify:
@snormore @fw42 @camilo @fbogsany @aybabtme @mkobetic @benbjohnson
Discuss!
There's still a good bunch of things I want to do (e.g. make Airbrake optional, support logging to Kafka, etc.) but I feel this is the first iteration that is useful.