Add a human-optimized text encoder #123

akshayjshah · 2016-08-02T17:24:19Z

Add a text encoder that's friendlier for human consumption. By default, the TextEncoder output looks like this:

[I] 1970-01-01T00:00:00Z Something happened. foo=bar baz=bing

The timestamp format is configurable via functional options, and the encoder includes support for unsigned ints (in advance of #116).

Resolves #59.

prashantv · 2016-08-02T18:09:05Z

text_encoder.go

+	final.bytes = append(final.bytes, msg...)
+}
+
+// A TextOption is used to set options for a text encoder.


Rather than add separate options for NoTime and TimeFormat etc, I would prefer to:

change the JSONOption interface method to be something like applyJSON and have the TextOption interface use applyText. This will allow a single formatter to apply to both.

Make TextFormatter implement TextOption` as well.

Now the same NoTime and TimeFormatter types can be used as both text options and JSON options

This reduces the interface and allows the user to use the same options for both encoders.

Sure, I can do that. It makes the JSON options more complex, but it's eminently doable.

To document an offline conversation we had quite a while ago:

Consolidating these is possible, but I think it'll make them appreciably more complex. For example, the JSON encoder supports an option that sets the key for the entry's message (e.g., {"msg": "foo"} or {"the_message": "foo"}). That's not a meaningful concept for the text encoder.

I don't mind whichever direction @sectioneight wants to go with this.

I'll choose the path of least resistance here :)

Happy to figure out how best to clean up the API in a separate PR.

I'm leaning towards separating time formatters from knowing about keys, There can be separate options for formatting time, and for customizing the time key. That way the options that customize keys (like MessageKey and TimestampKey) only implement JSONOption but the time formatting options NoTime, EpochFormatter, RFC3339Formatter can apply to both JSON encoders and text encoders.

@prashantv can you open an issue describing the desired changes to the APIs so we make sure to follow up in the future? I'm one of those weird people that actually likes following up on loose ends

ascandella · 2016-09-15T21:33:47Z

This seems useful. Is this still being worked on? Would you like help shepherding it over the finish line?

akshayjshah · 2016-09-15T23:07:59Z

@sectioneight Very much so - I'm out on paternity leave tomorrow, and I'm not sure when I'm going to get back to this.

ascandella · 2016-09-15T23:36:33Z

Cool, will do. @akshayjshah do you have any local/unpushed changes before I take over this branch?

akshayjshah · 2016-09-16T01:03:07Z

Nope, this is it - all yours.

ascandella · 2016-09-25T17:58:50Z

Ok @prashantv -- I took a pass at implementing the state. I'm not sure if the semantics are correct yet -- is the AddMarshaler the only place we need to care?

ascandella · 2016-09-25T18:57:35Z

text_encoder.go

+type textEncoder struct {
+	bytes   []byte
+	timeFmt string
+	nested  *atomic.Bool


I went with an atomic Bool here -- I'm pretty sure these encoders are supposed to be goroutine safe but I don't see any sync primitives on modifying e.g. enc.bytes in the various methods.

Is locking taken care of by the logging and should the encoder not have to worry about it?

I removed the atomics after I realized encoders are only used once. Not sure why GH is still showing this...

ascandella · 2016-09-25T18:59:36Z

example_test.go

+	textLogger.Info("With Marshaler", zap.Marshaler("m", zap.LogMarshalerFunc(m)))
+
+	// Output:
+	// [I] With Marshaler m={loggable=yes number=1}


This sort of feels like it's less useful as an example but as a unit test. However, it doesn't look like the unit test framework is set up to do "end-to-end" (e.g. assert the actual output) and I wanted to show that I handles this case properly.

Is there a better place to put this?

Moved to text_logger_test.go

Still see it here -- has it been removed from example_test.go?

$ grep Marshaler example_test.go // no results

When logging to console, JSON isn't the most friendly output - it's nice for machines but not optimal for humans. This PR adds a text encoder that prioritizes human readability, along with a few time-handling options for that encoder. In advance of #116, this PR also includes support for unsigned ints.

prashantv · 2016-09-25T19:16:01Z

text_encoder.go

+
+func (enc *textEncoder) AddBool(key string, val bool) {
+	enc.addKey(key)
+	if val {


Can simplify to just use AppendBool here (same as #148)

prashantv · 2016-09-25T19:29:45Z

text_encoder.go

+}
+
+func (enc *textEncoder) AddFloat64(key string, val float64) {
+	enc.addKey(key)


we can simplify to just use strconv.AppendFloat here. In the JSON encoder, the output for nan/infinity is quoted, so a normal would get: "num": 1 but nan would get "num": "+Inf"

For the text encoder, we don't need to quote the inf/nan, and AppendFloat already handles these correctly,
https://play.golang.org/p/W3FP95QIMc

prashantv · 2016-09-25T19:33:07Z

text_encoder.go

+	enc.firstNested = true
+	enc.bytes = append(enc.bytes, '{')
+	err := obj.MarshalLog(enc)
+	enc.bytes = append(enc.bytes, '}')


We should reset firstNested to false here. I think a failing test case for this would:

add an object where the MarhsalLog adds no fields

then add a normal field (like string)

So we'd want something like emptyObj={} something=val but we might get emptyObj={}something=val

Good catch! Fixed and added negative test

ascandella · 2016-09-25T19:49:41Z

Ok @prashantv, I think I've addressed all comments (and nice catch on the nested error!)

ascandella · 2016-09-25T22:10:47Z

Still getting used to the new github review flow. Are there more comments to be addressed?

prashantv

lgtm, I think some of the examples can be removed now that they're in text_logger_test.go

prashantv · 2016-09-25T22:14:50Z

example_test.go

+	// [I] This is a text log.
+}
+
+func ExampleNew_textEncoderSingleCurlyBrace() {


Can we remove this example now that the integration test covers it

Yes, it's removed. Ugh, I still don't understand the new GH UI

prashantv · 2016-09-25T22:15:20Z

example_test.go

+	textLogger.Info("With Marshaler", zap.Marshaler("m", zap.LogMarshalerFunc(m)))
+
+	// Output:
+	// [I] With Marshaler m={loggable=yes number=1}


Still see it here -- has it been removed from example_test.go?

ascandella · 2016-09-25T23:58:24Z

@prashantv updated by moving out of example_test -- does your approval mean a self-merge is now appropriate? Or what's your team's process on GitHub?

prashantv · 2016-09-26T00:13:26Z

Yep, for contributors who are part of the organization or project, typically approval means self-merge. For external contributors, the reviewer tends to merge.

Since you are in the org, feel free to self-merge! Thanks for wrapping this up @sectioneight

prashantv · 2016-09-26T00:14:34Z

Forgot to mention, suggest "Squash and merge" -- and feel free to edit the title/body of the squashed commit, rather than leaving the default list of all commits.

akshayjshah force-pushed the ajs-text branch from cb0d867 to 7194726 Compare August 2, 2016 17:44

prashantv reviewed Aug 2, 2016
View reviewed changes

ascandella force-pushed the ajs-text branch 2 times, most recently from fe7eb32 to 29876f2 Compare September 16, 2016 01:24

ascandella force-pushed the ajs-text branch 2 times, most recently from 38c3add to 00a304c Compare September 25, 2016 18:55

ascandella reviewed Sep 25, 2016

View reviewed changes

ascandella force-pushed the ajs-text branch 2 times, most recently from 10a0c8f to f979bf7 Compare September 25, 2016 19:08

ascandella force-pushed the ajs-text branch from f979bf7 to 0bbb077 Compare September 25, 2016 19:25

Aiden Scandella added 2 commits September 25, 2016 12:27

Better naming

e62332f

Add integration test file

101b4f2

prashantv reviewed Sep 25, 2016

View reviewed changes

Address review comments

6e56af6

prashantv approved these changes Sep 25, 2016

View reviewed changes

Move integration tests out of example

6c21079

ascandella merged commit a26b259 into master Sep 26, 2016

ascandella deleted the ajs-text branch September 26, 2016 00:15

This was referenced Sep 26, 2016

Clean up API for timestamp formatting in JSON encoder and text encoder #149

Closed

Add Uintptr to the text encoder #151

Merged

raytung mentioned this pull request Apr 4, 2018

Missing text encoder #570

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a human-optimized text encoder #123

Add a human-optimized text encoder #123

akshayjshah commented Aug 2, 2016

prashantv Aug 2, 2016

akshayjshah Aug 2, 2016

akshayjshah Sep 16, 2016

ascandella Sep 24, 2016

prashantv Sep 25, 2016

ascandella Sep 26, 2016

prashantv Sep 26, 2016

ascandella Sep 26, 2016

ascandella commented Sep 15, 2016

akshayjshah commented Sep 15, 2016

ascandella commented Sep 15, 2016

akshayjshah commented Sep 16, 2016

ascandella commented Sep 25, 2016

ascandella Sep 25, 2016

ascandella Sep 25, 2016

ascandella Sep 25, 2016

ascandella Sep 25, 2016

prashantv Sep 25, 2016

ascandella Sep 26, 2016

prashantv Sep 25, 2016 •

edited

Loading

ascandella Sep 25, 2016

prashantv Sep 25, 2016

ascandella Sep 25, 2016

prashantv Sep 25, 2016

ascandella Sep 25, 2016

ascandella commented Sep 25, 2016

ascandella commented Sep 25, 2016

prashantv left a comment

prashantv Sep 25, 2016

ascandella Sep 26, 2016

prashantv Sep 25, 2016

ascandella commented Sep 25, 2016

prashantv commented Sep 26, 2016

prashantv commented Sep 26, 2016

Add a human-optimized text encoder #123

Add a human-optimized text encoder #123

Conversation

akshayjshah commented Aug 2, 2016

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

ascandella commented Sep 15, 2016

akshayjshah commented Sep 15, 2016

ascandella commented Sep 15, 2016

akshayjshah commented Sep 16, 2016

ascandella commented Sep 25, 2016

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

prashantv Sep 25, 2016 • edited Loading

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

ascandella commented Sep 25, 2016

ascandella commented Sep 25, 2016

prashantv 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

ascandella commented Sep 25, 2016

prashantv commented Sep 26, 2016

prashantv commented Sep 26, 2016

prashantv Sep 25, 2016 •

edited

Loading