Support fast Zap logging #361

mh-park · 2018-07-27T18:52:28Z

(This supersedes #360.)

This adds a zapcore.MarshalLogObject/Array method to all generated structs and its underlying components, enums, and non-primitive typedefs, implementing Zap marshalers (i.e. ObjectMarshaler, ArrayMarshaler). For typedefs of primitives, the logging is up to the user, simply by casting it down to the root type and using the respective Add/Append... method of the Zap encoder.

Sets and maps have nondeterministic behavior in iteration, but Zap logs/JSON has arrays that are orderly, causing issues in testing. Instead, we only write one element for those that are affected and test the loop logic separately in TestNondeterministicZapLogging.

mh-park · 2018-07-31T21:22:18Z

This change requires uber-go/zap#614.

amckinney

Sweet! Things look good on the first pass. Some minor comments.

amckinney · 2018-08-02T19:49:02Z

CHANGELOG.md

@@ -5,7 +5,9 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/en/1.0.0/)
 and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0.html).

 ## [Unreleased]
- No changes yet.
+- All structs and its underlying components, enums, and typedefs of non-primitives
+  now implement `zapcore.ObjectMarshaler` or `zapcore.ArrayMarshaler`, depending


Discrepancy between the description and the changelog with respect to typedef support.

amckinney · 2018-08-02T19:51:43Z

gen/enum.go

@@ -125,6 +125,23 @@ func enum(g Generator, spec *compile.EnumSpec) error {
 			return []byte(<$strconv>.FormatInt(int64(<$v>), 10)), nil
 		}

+		<$zapcore := import "go.uber.org/zap/zapcore">
+
+		// MarshalLogObject implements zapcore.ObjectMarshaler, allowing


nit: What about enabling or implementing instead of allowing?

amckinney · 2018-08-02T20:00:12Z

glide.yaml

@@ -12,6 +12,8 @@ import:
  version: ^1
 - package: github.com/fatih/structtag
  version: ^0.1.0
+- package: go.uber.org/zap
+  version: master


Let's pin to the latest stable release here (i.e. ^1.9.0): https://github.com/uber-go/zap/releases

We had to do a bugfix in zap (see uber-go/zap#614), and without it my tests would fail. I'll either have to pin to master or wait for the next release.

Ah got it. We should see if we can cut a release before merging this in.

amckinney · 2018-08-02T20:26:17Z

gen/field.go

+					if <$fval> != nil {
+						<- if (zapCanError .Type)>if err := <end ->
+						<$enc>.Add<zapEncoder .Type>("<.Name>", <zapMarshalerPtr .Type $fval>)
+						<- if (zapCanError .Type)>; err != nil {


This makes sense, but we need to check the same conditional twice consecutively. For readability, consider:

<- if (zapCanError .Type) -> if err := <$enc>.Add<zapEncoder .Type>("<.Name>", <zapMarshaler .Type $fval>); err != nil { return err } <- else -> <$enc>.Add<zapEncoder .Type>("<.Name>", <zapMarshaler .Type $fval>) <- end ->

It's a little more verbose, but might be more clear.

We could potentially inject a template function that would handle this for us, such as <withZapErrCheck .Type>

That would output a template which would wrap the result of the first argument in a if err := ...; err != nil { ... } based on the result of (zapCanError .Type), but this might be overkill / less readable.

TL;DR: This is fine as is - feel free to disregard these suggestions.

amckinney · 2018-08-02T20:56:20Z

gen/zap.go

+		// for consistency.
+		base64 := g.Import("encoding/base64")
+		return fmt.Sprintf("%v.StdEncoding.EncodeToString(%v)", base64, fieldValue), nil
+	case *compile.MapSpec:


We can collapse these cases:

... case *compile.MapSpec, *compile.SetSpec, *compile.ListSpec: return z.listG.zapMarshaler(g, spec, t, fieldValue) ...

they're slightly different: mapSpec --> z.mapG, setSpec --> z.setG, listSpec --> z.listG

amckinney · 2018-08-02T20:57:46Z

gen/zap.go

+		return z.listG.zapMarshaler(g, spec, t, fieldValue)
+	case *compile.StructSpec:
+		return fieldValue, nil
+	default:


We can remove the empty default cases and let it fall-through to the panic.

amckinney · 2018-08-02T20:57:59Z

gen/zap.go

+		*compile.I64Spec, *compile.DoubleSpec, *compile.StringSpec, *compile.BinarySpec:
+		return false
+
+	// Else


For consistency, how about Non-primitives?

amckinney · 2018-08-02T21:10:24Z

gen/internal/tests/typedefs/types.go

@@ -418,6 +480,13 @@ func (lhs EdgeMap) Equals(rhs EdgeMap) bool {
 	return _Map_Edge_Edge_Equals(lhs, rhs)
 }

+func (v EdgeMap) MarshalLogArray(enc zapcore.ArrayEncoder) error {
+	return ((_EdgeMap_Zapper)(([]struct {


:^)

i talked to Abhinav about this; he said we could probably clean that up, but if a user is creating ugly thrift structs, they deserve ugly code :P

amckinney · 2018-08-03T21:54:44Z

Thanks for the update! I like that it's more explicit how err is handled in all of these cases.

But now, we see a lot of repetition in the method calls. What if we took this one step further and did:

<$encAdd := <$enc>.Add<zapEncoder .Type>("<.Name>", <zapMarshalerPtr .Type $fval>)>
...
<- if (zapCanError .Type) ->
  if err := <$encAdd>; err != nil {
    return err
  }
<- else ->
  <$encAdd>
<- end ->
...

What are your thoughts?

mh-park · 2018-08-03T22:51:19Z

@amckinney I agree that looks cleaner, but I don't think <$enc>.Add<zapEncoder .Type>("<.Name>", <zapMarshalerPtr .Type $fval>) will evaluate correctly if we put it inside another <>. If we want it to evaluate the way we want it, we'll need to change the formatting a little bit: <$encAdd := printf "%s.Add%s(%q,%s)" $enc (zapEncoder .Type) .Name (zapMarshaler .Type $fval)>. Not sure which is more readable, let me know what you think.

For easier comparison:

<$enc>.Add<zapEncoder .Type>("<.Name>", <zapMarshalerPtr .Type $fval>)

<$encAdd := printf "%s.Add%s(%q,%s)" $enc (zapEncoder .Type) .Name (zapMarshaler .Type $fval)>

amckinney

Awesome, this looks really great!

amckinney · 2018-08-03T23:06:09Z

Please hold off on merging until we release go.uber.org/zap and add the release pin there.

This reverts commit 19af15f.

WIP: Zap logging support

373c29c

mh-park requested a review from abhinav July 27, 2018 18:55

mh-park added 6 commits July 27, 2018 16:11

Add error handling for enc.{Add,Append}{Array,Object} for Zap logging

31f8d9c

Replace generated files with previous commit's change applied

7ebab96

Add comment placeholders

af51150

Add test files

cb70f07

Add Zap logging tests

eb6b1d7

mh-park added 8 commits July 31, 2018 18:07

Delete comments and add tests

7eeeaaf

Add testdata files

eb8c446

Drop go version 1.7/1.8 in travis

e886c90

Refactor Zap funcs

ba7e94e

Fix template spacing

169840e

Fix formatting and delete useless comments

6406b67

oops

dc47835

Add testdata

e0703a3

mh-park changed the title ~~WIP: Zap logging support~~ Support fast Zap logging Aug 2, 2018

Update changelog

ac7fb4b

mh-park requested a review from amckinney August 2, 2018 19:49

amckinney reviewed Aug 2, 2018

View reviewed changes

mh-park added 3 commits August 2, 2018 15:21

WIP

a3a379f

code review

323ec36

Add testdata

95c440c

abhinav mentioned this pull request Aug 3, 2018

Support enum item renaming #362

Closed

wip

34c1a75

amckinney approved these changes Aug 3, 2018

View reviewed changes

mh-park added 5 commits August 3, 2018 17:16

wip

7f0fe26

Refactor for readability

329ee0c

Fix styling

2328cac

glide: Pin to zap ^1

8eb048f

Modify changelog to reflect pr message

6b9f2b3

mh-park merged commit 19af15f into dev Aug 6, 2018

mh-park deleted the minho/zap2 branch August 6, 2018 20:31

abhinav restored the minho/zap2 branch August 6, 2018 20:35

mh-park added a commit that referenced this pull request Aug 6, 2018

Revert "Support fast Zap logging (#361)"

a7b57a3

This reverts commit 19af15f.

This was referenced Aug 6, 2018

Revert "Support fast Zap logging" #365

Closed

Support fast Zap logging #366

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support fast Zap logging #361

Support fast Zap logging #361

mh-park commented Jul 27, 2018 •

edited

Loading

mh-park commented Jul 31, 2018

amckinney left a comment

amckinney Aug 2, 2018

amckinney Aug 2, 2018

amckinney Aug 2, 2018

mh-park Aug 2, 2018

amckinney Aug 3, 2018

amckinney Aug 2, 2018

amckinney Aug 2, 2018

mh-park Aug 2, 2018

amckinney Aug 3, 2018

amckinney Aug 2, 2018

amckinney Aug 2, 2018

amckinney Aug 2, 2018

mh-park Aug 2, 2018

amckinney commented Aug 3, 2018

mh-park commented Aug 3, 2018 •

edited

Loading

amckinney left a comment

amckinney commented Aug 3, 2018

Support fast Zap logging #361

Support fast Zap logging #361

Conversation

mh-park commented Jul 27, 2018 • edited Loading

mh-park commented Jul 31, 2018

amckinney 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

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

amckinney commented Aug 3, 2018

mh-park commented Aug 3, 2018 • edited Loading

amckinney left a comment

Choose a reason for hiding this comment

amckinney commented Aug 3, 2018

mh-park commented Jul 27, 2018 •

edited

Loading

mh-park commented Aug 3, 2018 •

edited

Loading