Refactor zap around Facility #201

jcorbin · 2016-12-06T01:31:59Z

Current state (as of 2016-12-23: done / in review

akshayjshah

This is good stuff - the reduction in repeated logic is great.

To your specific questions:

I'm uncomfortable making Logger a concrete type; I'd like to keep it an interface an un-export the concrete type. It's not important to me that other folks be able to implement options - it's important that they be able to implement loggers.
I can be convinced, but right off the bat I don't love the name Facility. What do you think about Core?
Perhaps the standard log signature should return an error? That gives the Facility a way to pipe information to the logger's error output.
I like the unification of Entry and CheckedMessage, but I don't like the caveat on Write. Let's think more about how to make that type less error-prone. We could take the Java-esque approach of allowing field access only through getters and setters, or we could embed an Entry and a Facility in a CheckedEntry type.

akshayjshah · 2016-12-06T05:38:59Z

entry.go

-	e.Time = _timeNow().UTC()
-	e.enc = enc
-	return e
+// XXX compat with *CheckedMessage, drop?


I'm on the fence about this. To me, if e := logger.Check(....); e.OK() { } is nicer than if e := logger.Check(....); e != nil { }, but it's less idiomatic. I'm fine with removing this little nicety in favor of painful explicitness.

yeah, go is nothing if not the land of nil checking ;-)

akshayjshah · 2016-12-06T05:41:25Z

entry.go

-func (e *Entry) Fields() KeyValue {
-	return e.enc
+// Write writes the entry any logger reference stored. This only works if the
+// Entry was returned by Logger.Check.


Let's consider ways to avoid this slight weirdness. Perhaps un-export all fields and provide getters & setters? Then there's no way to get an Entry except to use logger.Check...

Will go the route of a CheckedEntry that embeds a Facility and Entry.

akshayjshah · 2016-12-06T05:42:45Z

facility.go

+	Enabled(Entry) bool
+	Log(Entry, ...Field)
+	// XXX idea on how we could restore internal enoding error repuorting:
+	//     SetErrorOutput(ws WriteSyncer)


Do we need this, or can the logger communicate to the Meta with returned errors?

Great point above, all we need to do is make it Log(Entry, ...Field) error, then reporting isn't the facilities's concern.

akshayjshah · 2016-12-06T05:45:02Z

facility.go

+func newIOFacility(enc Encoder, out WriteSyncer) *ioFacility {
+	if out == nil {
+		out = newLockedWriteSyncer(os.Stdout)
+	}


Do we need this little bit of magic? I'd prefer to keep default settings at the top level if we can.

Fair point, may be best moved to func WriterFacility(Encoder, io.Writer) Facility

akshayjshah · 2016-12-06T05:47:49Z

logger.go

+
+	Development bool
+	Hooks       []Hook
+	ErrorOutput WriteSyncer


These shoudn't be exported if we're going to make Logger a concrete type; they're not safe to interact with while the Logger is in use.

akshayjshah · 2016-12-06T06:11:39Z

Haven't traced the code flow in detail, but Entry definitely escapes to the heap; let's make sure we pool it and have a very clear story for where it's returned to the pool (preferably one that's entirely controlled by the Logger, so implementors of our various interfaces don't have to get pooling right).

jcorbin · 2016-12-06T19:46:35Z

Will introduce *CheckedEntry which we can pool and/or verify its heap escape velocity; then Entry will only ever be passed and received by value.

jcorbin · 2016-12-06T19:51:42Z

I'll internalize the Logger type to be just logger again, and then re-introduce the Logger type as an interface.

After that, the next big rocks to be pushed:

Entry => CheckedEntry split
Drop the LevelEnabler from logger; demote it from Option status
At least use LevelEnabler in most concrete Facility types; take LevelEnabler as part of the Facility contract if necessary for composition

As you can see in this work: f764dcf#diff-133c628d7af54d3a9d7bf19081e3ed57R29 , I already bumped up against this need with the tee-spy tests.

* Shift hook running into Meta * Firm up a convenience Meta.Encode method * Unify time.Now calling, drop test stub

jcorbin · 2016-12-09T18:34:22Z

Updated the lead comment with current status notes.

* add the levelenablerfunc for #201 * add the test for levelenablerfunc #205 * remove the comment in test and change code into the buf.Line()

jcorbin · 2016-12-23T00:46:29Z

Addressed feedback and rebased on latest dev.

jcorbin · 2016-12-23T00:52:29Z

Aside from minor changes, this last push contained:

dropped hook support: addCaller and addStack are now in the canon (please consider the new TODO inside the logger struct)
nuked WriterFacility default to stdout
made WriterFacility take a WriteSyncer
axed Facility.Log; just have logger.Log use Facility.Check all the time
- this compounded for another win around the new addCaller / addStack shape: we avoid collecting if the Log is actually a noop
brought back safeToWrite inside of CheckedEntry

I punted pre-allocating or carving out any static Facility space inside CheckedEntry for future work.

jcorbin · 2016-12-23T17:58:58Z

entry.go

+// Write writes the entry to any Facility references stored, returning any
+// errors, and returns the CheckedEntry reference to a pool for immediate
+// re-use.
+func (ce *CheckedEntry) Write(fields ...Field) error {


I want to point out to reviewers this subtle semantic change:

prior (*CheckedMessage).Write was void return, it internally noted any error

new (*CheckedEntry).Write returns the error

Is this okay or should I restore the prior behavior that users of Logger.Check().Write() don't have to handle errors?

I'd prefer to restore the previous behavior. To me, it's unreasonable to expect users to handle errors from the logging library.

We can and should pass around errors internally, but I think we should hide them from the user.

However, let's take this up in a follow-on PR. This one's gone on long enough :)

Since we know the types of the file name and line number, we don't need to use fmt.Sprintf and pay that allocation cost. We can instead manage a byte slice ourselves, which saves a few small allocations. (Eventually, we should pool these byte buffers to save a further allocation.)

akshayjshah · 2016-12-30T01:08:38Z

@jcorbin - I've tacked on a few commits that clean up some typos and fix up a few uncontroversial bits and pieces. Review at your leisure.

Once tests pass, I'm going to merge this PR so that we can move on. Let's address any remaining issues in follow-on PRs (particularly the issue you raised about CheckedEntry.Write returning errors).

…201

* Add AddCallerSkip option * Improve and expand caller skip test * Fix logger Check v Level-log skipping * logger: drop old Log method; not actually part of the interface since #201

Pivot the zap API significantly to make `Facility` the central extension point. This greatly reduces the surface area for extension authors to implement, and it alters some APIs to make composition more straightforward.

* Add AddCallerSkip option * Improve and expand caller skip test * Fix logger Check v Level-log skipping * logger: drop old Log method; not actually part of the interface since #201

akshayjshah reviewed Dec 6, 2016

View reviewed changes

This was referenced Dec 6, 2016

Make DPanic work correctly for composed loggers #202

Closed

Convert DFatal => DPanic(Level) #197

Merged

Simplify Entry #198

Merged

jcorbin force-pushed the logger_v_meta branch 2 times, most recently from ecaf1d5 to 3f09d8c Compare December 7, 2016 01:12

jcorbin mentioned this pull request Dec 7, 2016

Add filter logger #205

Closed

4 tasks

jcorbin force-pushed the logger_v_meta branch 4 times, most recently from c633e2e to 87316ea Compare December 8, 2016 23:21

arthurkiller mentioned this pull request Dec 9, 2016

Add the LevelEnablerFunc and test case #208

Merged

4 tasks

jcorbin referenced this pull request Dec 9, 2016

Simplify Entry and co (#198)

35cd796

* Shift hook running into Meta * Firm up a convenience Meta.Encode method * Unify time.Now calling, drop test stub

jcorbin mentioned this pull request Dec 9, 2016

Really controversial topic: completely remove panic/fatal? #207

Closed

jcorbin force-pushed the logger_v_meta branch from 87316ea to 8006f12 Compare December 9, 2016 18:27

jcorbin mentioned this pull request Dec 9, 2016

expose Enabled in Logger interface #190

Closed

jcorbin force-pushed the logger_v_meta branch 3 times, most recently from eb90a65 to d61fc60 Compare December 10, 2016 01:30

akshayjshah pushed a commit that referenced this pull request Dec 12, 2016

Add the LevelEnablerFunc and test case (#208)

05dadc4

* add the levelenablerfunc for #201 * add the test for levelenablerfunc #205 * remove the comment in test and change code into the buf.Line()

jcorbin force-pushed the logger_v_meta branch 4 times, most recently from b616807 to 76e507a Compare December 13, 2016 22:42

jcorbin pushed a commit that referenced this pull request Dec 13, 2016

Add the LevelEnablerFunc and test case (#208)

7ebe804

* add the levelenablerfunc for #201 * add the test for levelenablerfunc #205 * remove the comment in test and change code into the buf.Line()

jcorbin changed the base branch from master to dev December 13, 2016 22:43

This was referenced Dec 23, 2016

WIP: Add Caller/Callers fields #183

Closed

AddCaller hook can't be wrapped #40

Closed

Joshua T Corbin added 7 commits December 23, 2016 09:23

fixup! Drop hook support

5cf000e

logger.Log: trivial rename

454b555

logger.Check: trivial cleanup and docs

1bf8913

Simplify Logger.{Log,Fatal,Panic,DPanic} over Logger.Check

3c8ac71

logger: simplify convenience method decls

f456dd9

logger.check: further avoid annotating unwritten terminal entries

72ed144

logger.Log: better name the internal error a "write"

af6df22

jcorbin commented Dec 23, 2016

View reviewed changes

Joshua T Corbin and others added 6 commits December 23, 2016 10:00

CheckedEntry.Write: doc terminal behavior

37b0671

Size CheckWriteAction to use a bit less space

ac734c0

Fix WriterFacility docs

170b576

Short-circuit With if no fields are passed

ead0327

Spelling and formatting fixups

5e8fead

akshayjshah changed the title ~~Refactor zap around Faclity~~ Refactor zap around Facility Dec 30, 2016

akshayjshah merged commit 3378a0a into dev Dec 30, 2016

akshayjshah mentioned this pull request Jan 3, 2017

Consider separating leaf and middleware Facilities #224

Closed

jcorbin mentioned this pull request Jan 9, 2017

Add finer grained caller annotation support #227

Merged

jcorbin pushed a commit that referenced this pull request Jan 9, 2017

logger: drop old Log method; not actually part of the interface since #…

4183fab

…201

akshayjshah deleted the logger_v_meta branch January 24, 2017 17:24

skipor mentioned this pull request Feb 19, 2017

Make caller formatting configurable #319

Closed

liuzengh mentioned this pull request Aug 28, 2023

zap: always return a new child logger from *Logger.With #1342

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor zap around Facility #201

Refactor zap around Facility #201

jcorbin commented Dec 6, 2016 •

edited

Loading

akshayjshah left a comment

akshayjshah Dec 6, 2016

jcorbin Dec 6, 2016

akshayjshah Dec 6, 2016

jcorbin Dec 6, 2016

akshayjshah Dec 6, 2016

jcorbin Dec 6, 2016

akshayjshah Dec 6, 2016

jcorbin Dec 6, 2016

akshayjshah Dec 6, 2016

akshayjshah commented Dec 6, 2016

jcorbin commented Dec 6, 2016

jcorbin commented Dec 6, 2016 •

edited

Loading

jcorbin commented Dec 9, 2016

jcorbin commented Dec 23, 2016

jcorbin commented Dec 23, 2016

jcorbin Dec 23, 2016

akshayjshah Dec 28, 2016

akshayjshah Dec 30, 2016

akshayjshah commented Dec 30, 2016

Refactor zap around Facility #201

Refactor zap around Facility #201

Conversation

jcorbin commented Dec 6, 2016 • edited Loading

akshayjshah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah commented Dec 6, 2016

jcorbin commented Dec 6, 2016

jcorbin commented Dec 6, 2016 • edited Loading

jcorbin commented Dec 9, 2016

jcorbin commented Dec 23, 2016

jcorbin commented Dec 23, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah commented Dec 30, 2016

jcorbin commented Dec 6, 2016 •

edited

Loading

jcorbin commented Dec 6, 2016 •

edited

Loading