performancecopilot / speed

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

Replace logrus with zap #43

Closed suyash closed 7 years ago

suyash commented 7 years ago

This adds a couple of functions, speed.AddLogWriter(io.Writer) and speed.SetLogWriters(...io.Writer) to allow clients to modify the writers logs are written to.

@lzap @natoscott PTAL.

lzap commented 7 years ago

This does not look like I am able to redirect speed's output but rather copy log messages to extra writer if I read it right. I would like to be able to redirect all speed's output to syslog when debug is turned on, so I can find it along with my app logs. More than that, I very much like structured logging and this is awesome thing to have (kudos). I would love to be able to redirect speed into journald along with my application logs including all the extra fields.

But overall, definitely good step foreward, two dependencies instead of one! Big up.

Edit: The reason why I want to get rid of stdout is indeed - daemon mode. It will be eventually closed anyway, the only proper way of logging for daemons is journald these days (or syslog on older platforms). And I am writing a tiny daemon app with your lovely library :-)

suyash commented 7 years ago

@lzap this allows you to specify the io.Writer interfaces logs are written to. By default the array is set to just os.Stdout. With this PR you would be able to make a call like speed.SetLogWriters([]io.Writer{}) to set the log writers to an empty array. io.Writer seems to be a pretty easy abstraction, you can create any type with a Write method and set it as a log destination.

suyash commented 7 years ago

@lzap I have added tests to demonstrate usage for the functions. Please feel free to add comments :)

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.8%) to 74.26% when pulling 87d55e9803b8622e6a39bdf809a1177996c316e3 on zap into b8996a04acf74afd7183e64251641765dc42b45b on master.

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.8%) to 74.26% when pulling f4a0c3c31330fb2d47d029f6e7a8bd5a2c992f97 on zap into b8996a04acf74afd7183e64251641765dc42b45b on master.

suyash commented 7 years ago

@lzap ping

coveralls commented 7 years ago

Coverage Status

Coverage decreased (-3.8%) to 74.26% when pulling 4995117c1d3aef3af307d9531fb08078673a770d on zap into b8996a04acf74afd7183e64251641765dc42b45b on master.