performancecopilot / speed

A Go implementation of the PCP instrumentation API
MIT License
37 stars 6 forks source link

Allow external configuration of logger #36

Closed lzap closed 7 years ago

lzap commented 7 years ago

I would like to have speed's internal logger under my control, ideally if I am able to provide own logger configuration (or log instance) that would be great. Currently it depends on some external custom formatter which I don't like, I would like to send everything into journald/syslog by default.

suyash commented 7 years ago

@lzap there seem to be 2 solutions to this

  1. We set all the logging configuration here (https://github.com/performancecopilot/speed/blob/master/speed.go#L25-L29). We could allow the user to configure Formatter and Level

  2. We can allow the user to set a custom function, something like

    var Logger func(string, ...interface{})

    and use that as the logging function.

@natoscott any other ideas on this?

natoscott commented 7 years ago

Bit beyond my golang expertise - @owenbutler any thoughts here?

lzap commented 7 years ago

Whatever works, my goal is to have ability to setup logger my own way in the application. In my case that would be everything into sylog/journald with default settings (no special formatters or anything like that).

suyash commented 7 years ago

An update to this, played around with http://github.com/uber-go/zap today. Looks nice.

Currently waiting for https://github.com/uber-go/zap/pull/346 to go through, then I will implement a custom logging solution built on zap using io.Writer interfaces.

lzap commented 7 years ago

I have just doublechecked that speed compiles fine on golang from RHEL7 (go version go1.6.3 linux/amd64). Is there any chance you can stick with golang system logging interface getting rid of logrus? This will make packaging easier. It already contain several deps tho.

Edit: If you can get rid of logrus and the formatter, the only dependency is github.com/codahale/hdrhistogram which is great news for packager. I plan to do Fedora and EPEL7 packages.

suyash commented 7 years ago

@lzap please check out https://github.com/uber-go/zap. It is minimum-alloc logging library that is faster than stdlib log. Once my PR goes through, I plan to provide a way to specify io.Writer interfaces that the users wants logs to go to (os.Stdout, os.File*...), so if you want to pipe your logs somewhere else, all you would need to do is create a type satisfying io.Writer interface and add it to the list of outputs.

I do plan to remove logrus and the formatter, and replace it with zap.

Will try to do a PR today as a "preview" of what things will look like.

lzap commented 7 years ago

Okay, that's fine. Thanks.

suyash commented 7 years ago

Fixed in #43

lzap commented 7 years ago

Thanks looks good.