Skip to content
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

Replace logrus with zap #43

Merged
merged 7 commits into from
Mar 22, 2017
Merged

Replace logrus with zap #43

merged 7 commits into from
Mar 22, 2017

Conversation

suyash
Copy link
Collaborator

@suyash suyash commented Mar 10, 2017

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
Copy link
Contributor

lzap commented Mar 10, 2017

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
Copy link
Collaborator Author

suyash commented Mar 11, 2017

@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 suyash force-pushed the zap branch 4 times, most recently from 92302b7 to 4d89b58 Compare March 11, 2017 11:12
@suyash
Copy link
Collaborator Author

suyash commented Mar 11, 2017

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

@suyash suyash force-pushed the zap branch 2 times, most recently from 7174efc to 87d55e9 Compare March 13, 2017 11:56
@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

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

@coveralls
Copy link

coveralls commented Mar 13, 2017

Coverage Status

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

@suyash
Copy link
Collaborator Author

suyash commented Mar 16, 2017

@lzap ping

Copy link
Member

@natoscott natoscott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me Suyash.

@coveralls
Copy link

Coverage Status

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

@suyash suyash merged commit 95b7e16 into master Mar 22, 2017
@suyash suyash deleted the zap branch March 22, 2017 02:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants