Skip to content

Commit

Permalink
Simplify Entry and co (#198)
Browse files Browse the repository at this point in the history
* Shift hook running into Meta
* Firm up a convenience Meta.Encode method
* Unify time.Now calling, drop test stub
  • Loading branch information
jcorbin committed Dec 6, 2016
1 parent 26c7560 commit 35cd796
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 81 deletions.
25 changes: 2 additions & 23 deletions entry.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,7 @@

package zap

import (
"sync"
"time"
)

var (
_timeNow = time.Now // for tests
_entryPool = sync.Pool{New: func() interface{} { return &Entry{} }}
)
import "time"

// An Entry represents a complete log message. The entry's structured context
// is already serialized, but the log level, time, and message are available
Expand All @@ -43,20 +35,7 @@ type Entry struct {
enc Encoder
}

func newEntry(lvl Level, msg string, enc Encoder) *Entry {
e := _entryPool.Get().(*Entry)
e.Level = lvl
e.Message = msg
e.Time = _timeNow().UTC()
e.enc = enc
return e
}

// Fields returns a mutable reference to the entry's accumulated context.
func (e *Entry) Fields() KeyValue {
func (e Entry) Fields() KeyValue {

This comment has been minimized.

Copy link
@prashantv

prashantv Dec 9, 2016

Collaborator

Let's revert this change back to using the pointer. Entry is not a tiny struct (it's 64 bytes) so ideally we avoid copying that when calling Fields(). Guessing this was an artifact of the previous version where we tries to use values everywhere.

This comment has been minimized.

Copy link
@jcorbin

jcorbin Dec 9, 2016

Author Contributor

Yup, we can back off on value passing for now; that torch is being firmly carried by #201

return e.enc
}

func (e *Entry) free() {
_entryPool.Put(e)
}
43 changes: 0 additions & 43 deletions entry_test.go

This file was deleted.

2 changes: 1 addition & 1 deletion hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ var (
errCaller = errors.New("failed to get caller")
// Skip Caller, Logger.log, and the leveled Logger method when using
// runtime.Caller.
_callerSkip = 3
_callerSkip = 4
)

// A Hook is executed each time the logger writes an Entry. It can modify the
Expand Down
16 changes: 3 additions & 13 deletions logger.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ package zap

import (
"os"
"time"
)

// For tests.
Expand Down Expand Up @@ -125,21 +126,10 @@ func (log *logger) log(lvl Level, msg string, fields []Field) {
return
}

temp := log.Encoder.Clone()
addFields(temp, fields)

entry := newEntry(lvl, msg, temp)
for _, hook := range log.Hooks {
if err := hook(entry); err != nil {
log.InternalError("hook", err)
}
}

if err := temp.WriteEntry(log.Output, entry.Message, entry.Level, entry.Time); err != nil {
t := time.Now().UTC()
if err := log.Encode(log.Output, t, lvl, msg, fields); err != nil {
log.InternalError("encoder", err)
}
temp.Free()
entry.free()

if lvl > ErrorLevel {
// Sync on Panic and Fatal, since they may crash the program.
Expand Down
35 changes: 34 additions & 1 deletion meta.go
Original file line number Diff line number Diff line change
Expand Up @@ -22,9 +22,18 @@ package zap

import (
"fmt"
"io"
"os"
"sync"
"time"
)

var _entryPool = sync.Pool{
New: func() interface{} {
return &Entry{}
},
}

// Meta is implementation-agnostic state management for Loggers. Most Logger
// implementations can reduce the required boilerplate by embedding a Meta.
//
Expand Down Expand Up @@ -83,6 +92,30 @@ func (m Meta) Check(log Logger, lvl Level, msg string) *CheckedMessage {
// ErrorOutput. This method should only be used to report internal logger
// problems and should not be used to report user-caused problems.
func (m Meta) InternalError(cause string, err error) {
fmt.Fprintf(m.ErrorOutput, "%v %s error: %v\n", _timeNow().UTC(), cause, err)
fmt.Fprintf(m.ErrorOutput, "%v %s error: %v\n", time.Now().UTC(), cause, err)
m.ErrorOutput.Sync()
}

// Encode runs any Hook functions and then writes an encoded log entry to the
// given io.Writer, returning any error.
func (m Meta) Encode(w io.Writer, t time.Time, lvl Level, msg string, fields []Field) error {
enc := m.Encoder.Clone()
addFields(enc, fields)
if len(m.Hooks) >= 0 {
entry := _entryPool.Get().(*Entry)
entry.Level = lvl
entry.Message = msg
entry.Time = t
entry.enc = enc
for _, hook := range m.Hooks {
if err := hook(entry); err != nil {
m.InternalError("hook", err)
}
}
msg, enc = entry.Message, entry.enc

This comment has been minimized.

Copy link
@prashantv

prashantv Dec 9, 2016

Collaborator

If a hook can modify an entry (which we're explicitly allowing here), then it shouldn't be limited to just the message and fields. Why not do the same for level and time? (Changing time would be very very odd, but I don't think having a subset of fields be mutable and others not is very clear).

This comment has been minimized.

Copy link
@jcorbin

jcorbin Dec 9, 2016

Author Contributor

Yup the current idea, in #201, is to drop (mutative) hooks completely.

_entryPool.Put(entry)
}
err := enc.WriteEntry(w, msg, lvl, t)
enc.Free()
return err
}

0 comments on commit 35cd796

Please sign in to comment.