Skip to content
This repository has been archived by the owner on Apr 11, 2024. It is now read-only.

use zap instead of logrus #44

Merged
merged 12 commits into from
Sep 28, 2016
Merged

use zap instead of logrus #44

merged 12 commits into from
Sep 28, 2016

Conversation

kazegusuri
Copy link
Contributor

@kazegusuri kazegusuri commented Aug 28, 2016

use github.com/uber-go/zap instead of github.com/Sirupsen/logrus for access/error logs. This breaks compatibility about log format and achieves fully json logging.

zap is very complex to use (ex: omitempty), but slightly faster than other loggers. This is a benchmark before/after this changes for gaurun.LogPush.

before

go test github.com/mercari/gaurun/gaurun -benchmem -benchtime 5s -bench .   
BenchmarkLogPushIOSOmitempty-4        500000         12257 ns/op        1816 B/op         18 allocs/op
BenchmarkLogPushIOSFull-4             500000         13893 ns/op        2008 B/op         18 allocs/op

after

go test github.com/mercari/gaurun/gaurun -benchmem -benchtime 5s -bench .     
BenchmarkLogPushIOSOmitempty-4       1000000          6060 ns/op         945 B/op          4 allocs/op
BenchmarkLogPushIOSFull-4            1000000          6303 ns/op         945 B/op          4 allocs/op

2 allocs for zap logging. 2 allocs for ptime's format conversion.

EDIT:

After removing format conversion and computing the value instead.

go test github.com/mercari/gaurun/gaurun -benchmem -benchtime 5s -bench . 
BenchmarkLogPushIOSOmitempty-4       2000000          4574 ns/op         929 B/op          2 allocs/op
BenchmarkLogPushIOSFull-4            1000000          5030 ns/op         929 B/op          2 allocs/op

change list

  • log format change
    • [info] {"message": "...", ...} to {"level":"info","message":"..."...}
  • time format
    • 2015/08/04 15:32:35 JST (local time) to 2016-08-30T01:18:04Z (UTC)

@kazegusuri
Copy link
Contributor Author

current time format is t.Format("2006/01/02 15:04:05 MST") such as 2015/08/04 15:32:35 JST at local time. This PR changes the format to t.Format(time.RFC3339) such as 2016-08-30T01:18:04Z at UTC. If we need to keep time format, I will fix with zap.TimeFormatter.

return nil
}

// LogSetupError output error log with log package and exit immediately.
func LogSetupError(err error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

LogSetupFatal() looks better to me than LogSetupError().

@cubicdaiya
Copy link
Contributor

@kazegusuri How is the progress?

@kazegusuri kazegusuri changed the title [WIP] use zap instead of logrus use zap instead of logrus Sep 27, 2016
@kazegusuri
Copy link
Contributor Author

Remove WIP from title, please take a look.

)

func init() {
LogAccess = zap.New(zap.NewJSONEncoder(zap.RFC3339Formatter("time")), zap.DiscardOutput)
Copy link
Contributor

Choose a reason for hiding this comment

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

Unify time format, please.

- package: github.com/mercari/gcm
version: 987b1dc4ce9034b698395d35de1aadf999388b8f
- package: github.com/fukata/golang-stats-api-handler
version: e7ee1630fdb679c86ec8e3d0755e3576675fdb10
- package: github.com/stretchr/testify
version: v1.1.3
- package: github.com/uber-go/zap
Copy link
Contributor

Choose a reason for hiding this comment

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

Specify version of zap.

@cubicdaiya cubicdaiya merged commit af81953 into master Sep 28, 2016
@kazegusuri kazegusuri deleted the zap-logger branch September 28, 2016 09:49
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants