Clean up hooks and logger config #102
Conversation
Add a constructor so that users can also create no-op fields. I considered calling this `Nop`, but `zap.Nop()` looks like it should return a logger instead of a Field.
Reduce boilerplate by introducing an Entry struct and pulling common logger elements into a Config struct (that can be shared between Logger implementations). Along the way, start logging timestamps as floating-point seconds since epoch.
| // returns a modified message and an error. | ||
| type hook func(Level, string, KeyValue) (string, error) | ||
| // A hook is executed each time the logger writes a message. It can modify the | ||
| // message, but must not retain references to the message or any of its |
There was a problem hiding this comment.
s/message/entry (since the whole entry is mutable)
| final.bytes = append(final.bytes, `","ts":`...) | ||
| final.bytes = strconv.AppendInt(final.bytes, ts.UnixNano(), 10) | ||
| final.bytes = strconv.AppendFloat(final.bytes, timeToSeconds(e.Time), 'f', -1, 64) |
There was a problem hiding this comment.
Let's add a test for writing out a future time (maybe +100 years) and ensure that it's written without exponents
|
lgtm, just add a small test for the time formatting |
|
|
||
| var ( | ||
| _timeNow = time.Now // for tests | ||
| _entryPool = sync.Pool{New: func() interface{} { return &Entry{} }} |
There was a problem hiding this comment.
this is pretty odd. ISTM that Entry is safe to copy, why bother using pointers at all? Just pass it around by value.
There was a problem hiding this comment.
It is safe to copy, but it's a little large - at some point pre-1.0, we should test to see which option is faster.
Amendment: it's possible that I'm misunderstanding your comment. In general, we need to pass entries to hooks as a pointer, since hooks should be able to modify the entry's message (see AddCaller as an example). However, you're right to point out that it may be faster to create the entry on the stack in Logger.log and pass a reference to the hooks.
There was a problem hiding this comment.
type Entry struct {
Level Level // int32
Time time.Time // int64, int32, pointer
Message string // represented as reflect.StringHeader: uintptr, int
enc encoder // interface, two pointers
}Assuming 64-bit, that gives us:
4 + 8 + 4 + 8 + 8 + 8 + 8 + 8 = 56 bytes
Hard to imagine that using a sync.Pool is faster?
There was a problem hiding this comment.
Regarding your amendment: some benchmarking is needed, since creating the entry on the stack and passing a reference to the hooks may still cause the Go compiler to promote the Entry to the heap. Such is the price of fast compilation :(
This PR is a mish-mash of interwoven changes I made on a plane. Oops.
It accomplishes a few goals:
Entrytype, which packages together the data passed between loggers, hooks, and encoders. At the end of this series of diffs, we'll be able to move most ofLogger.WriteEntry's logic onto the entry itself, which will remove the ugly type-cast insidejsonEncoder.Remaining tasks for this round of changes: