Initial support for arrays #239

akshayjshah · 2017-01-11T23:55:41Z

Add basic support for logging arrays. To limit the scope of this commit,
it introduces only arrays of ObjectMarshalers, arrays of arrays, and
arrays of bools. It should, however, clarify how I intend to support
arrays of other types.

Of note, I opted not to have zap.Bools return a Field. Instead, it
returns a zapcore.ArrayMarshaler. This lets us build arrays of
heterogenous arrays. For most users, this will be papered over by the
upcoming zap.Any(key string, val interface{}) zapcore.Field function,
which will basically be a huge type switch.

(Along the way, I broke out our multi-error implementation into a separate
internal package.)

This starts to address #136, and is an alternative to the approach in #211.

From the small benchmarks included in this PR:

BenchmarkBoolsArrayMarshaler-4   	 1000000	      1028 ns/op	    1088 B/op	       3 allocs/op
BenchmarkBoolsReflect-4          	  500000	      3293 ns/op	    2280 B/op	       8 allocs/op

All allocations in the ArrayMarshaler benchmark come from allocating byte slices for encoders.

akshayjshah · 2017-01-12T00:01:17Z

zapcore/encoder.go

+	// and allocation-heavy.
+	AddReflected(key string, value interface{}) error
+	AddString(key, value string)
+}


Moved most of this here from object_encoder.go, since it seemed silly to have two interface-only files for encoders.

prashantv · 2017-01-12T07:18:14Z

array.go

+
+// Arrays wraps a slice of ArrayMarshalers so that it satisfies the
+// ArrayMarshaler interface. See the Array function for a usage example.
+func Arrays(as []zapcore.ArrayMarshaler) zapcore.ArrayMarshaler {


I don't think Arrays and Objects is going to be that useful to users.

If someone is implementing the ArrayMarshaler interface, then they probably care about performance, and wouldn't want to build up a separate new slice of []zapcore.ArrayMarshaler.

What use case are you expecting for Arrays and Objects?

Arrays is, IMO, useful precisely because most users won't be implementing ArrayMarshaler themselves - they'll be using the umpteen wrapper types we're providing, like Bools. Since we're already going down a road where we support mixed-type arrays, I'd like to take that to its logical conclusion.

I'll email you about Objects - relies on some internal examples.

prashantv · 2017-01-12T07:18:59Z

array_test.go

+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"


nit: import ordering

prashantv · 2017-01-12T07:26:38Z

array.go

+func (as arrayMarshalers) MarshalLogArray(arr zapcore.ArrayEncoder) error {
+	var errs *multierror.Error
+	for i := range as {
+		if as[i] == nil {


If we want to keep arrayMarshalers around, we should consider whether ignoring nil is the right behaviour. Should it just be an error? I think I'm learning towards remove Arrays and Objects and keep things minimal. It's easy enough for others to implement and add later.

If we really want to keep this type, can we add unit tests for the == nil branch here and in Objects?

Sure, I can add some tests. I'm going to have to come through lots and lots of zapcore and improve test coverage over the next week.

prashantv · 2017-01-12T07:29:03Z

internal/multierror/multierror.go

+// a single error.
+package multierror
+
+import "go.uber.org/zap/buffers"


is this package not internal on purpose? I really think this should be internal. Otherwise one bad user of buffers will cause all sorts of weird data races / corrupt log messages. I really don't think we want any other library to be able to put buffers into our pool.

Yep, makes sense. My bad.

prashantv · 2017-01-12T07:36:27Z

internal/multierror/multierror.go

+		return e
+	}
+	if e == nil {
+		e = &Error{}


This is a strange pattern, you have an Append that works on a pointer, sometimes modifies the value inside, sometimes returns a new pointer. Given that, why do you need this method on the pointer? Why not always work on the value? Passing around a []error by value is not expensive, makes the semantics cleaner, and reduces the need for *Error

That'll probably also avoid an allocation when there is an error

prashantv · 2017-01-12T07:39:26Z

internal/multierror/multierror_test.go

+
+	"github.com/stretchr/testify/assert"
+)
+


One slightly odd test that I think is worth having -- you really don't want Error to implement the error interface, since that could cause odd issues. So we could have a test like:

var v interface{} = Error{} _, ok := v.(error) require.False(t, ok)

prashantv · 2017-01-12T07:44:09Z

zapcore/json_encoder_test.go

@@ -136,6 +199,57 @@ func TestJSONEncoderFields(t *testing.T) {
 	}
 }

+func TestJSONEncoderArrayTypes(t *testing.T) {


Can you add unit tests for:

empty array

array with different types

prashantv · 2017-01-12T07:50:05Z

array_test.go

+		{
+			"objects",
+			Objects([]zapcore.ObjectMarshaler{
+				zapcore.ObjectMarshalerFunc(func(_ zapcore.ObjectEncoder) error { return nil }),


here and below, since you don't use any args, you can just make this func(zapcore.ObjectEncoder) error { .. }

We'll need a multierror implementation in both zap and zapcore to support arrays. Clean up the existing implementation a bit and promote it to its own internal package.

Add basic support for logging arrays. To limit the scope of this commit, it introduces only arrays of ObjectMarshalers, arrays of arrays, and arrays of bools. It should, however, clarify how I intend to support arrays of other concrete types. Of note, I opted not to have `zap.Bools` return a `Field`. Instead, it returns a `zapcore.ArrayMarshaler`. This lets us build arrays of heterogenous arrays. For most users, this will be papered over by the upcoming `zap.Any(key string, val interface{}) zapcore.Field` function, which will basically be a huge type switch.

``` BenchmarkBoolsArrayMarshaler-4 1000000 1028 ns/op 1088 B/op 3 allocs/op BenchmarkBoolsReflect-4 500000 3293 ns/op 2280 B/op 8 allocs/op ```

jcorbin · 2017-01-12T19:22:41Z

internal/multierror/multierror.go

@@ -0,0 +1,71 @@
+// Copyright (c) 2016 Uber Technologies, Inc.


Hmm, should all of these new files be 2017? When/how does that even matter?

My understanding is that copyright applies to the creative work as a whole, which is the zap project; since that project started in 2016, all copyright starts there. (Imagine that Walt Disney updates a few of the line arcs in the original Mickey Mouse drawing. Those lines don't have a separate copyright date.)

Of course, I could easily be wrong.

jcorbin · 2017-01-12T19:35:18Z

internal/multierror/multierror.go

+//
+// Note that failing to use AsError will almost certainly lead to bugs with
+// non-nil interfaces containing nil concrete values.
+func (e Error) AsError() error {


This is an interesting iteration on the MultiError pattern, so let's see if I've got the use case right:

func some() error { var errs multierror.Error // ... errs = errs.Append(thisCanFailButDoesntStopUs()) // ... return errs.AsError() }

And so, the main value of the type Error struct is those append ergonomics I think, since otherwise we could have:

func some() error { var errs []error // ... if err := thisCanFailButDoesntStopUs(); err != nil { errs = append(errs, err) } // ... return multierror.AsError(errs) }

To me, the latter is more immediately understandable since most go code has this pervasive if err ... pattern; does micro perf even matter in these cases? is there a material difference anyhow?

That is indeed the use case. While I agree that error handling is often repetitive, I think we're often encouraged to program with error values. Where possible, I'd like to make things less repetitive. (I'm treating bufio.Reader as my spirit animal here.)

jcorbin · 2017-01-12T19:47:46Z

zapcore/json_encoder.go

@@ -90,7 +90,7 @@ func (enc *jsonEncoder) AddString(key, val string) {

 func (enc *jsonEncoder) AddBool(key string, val bool) {
 	enc.addKey(key)
-	enc.bytes = strconv.AppendBool(enc.bytes, val)
+	enc.AppendBool(val)


It will probably be possible soon (desirability debatable) to have something like:

// KeyEncoder defines an encoding scheme for top-level log field names. // TODO: maybe it needs other methods for start/end key context type KeyEncoder interface { AddKey(string key) } // GenericEncoder implements Encoder given a key and an array encoder. type GenericEncoder struct { keyEnc KeyEncoder arrEnc Array Encoder } // TODO: text and json encoder then differ mostly (only?) along their KeyEncoder // TODO: maybe KeyEncoder should be EntryEncoder: encompass all builtin "entry fields", plus extra field name encoding

Maybe this only works for text-ish encoders, and maybe that's okay; maybe it works for msgpack too, not sure...

jcorbin · 2017-01-12T20:01:03Z

zapcore/json_encoder_bench_test.go

@@ -63,8 +63,8 @@ func BenchmarkZapJSON(b *testing.B) {
 		for pb.Next() {
 			enc := newJSONEncoder(cfg)
 			enc.AddString("str", "foo")
-			enc.AddInt64("int", 1)
-			enc.AddInt64("int64", 1)
+			enc.AddInt64("int64-1", 1)


What's up with this, it seems unrelated?

Yep, the key was just wrong. Noticed and fixed in passing.

jcorbin · 2017-01-12T20:11:33Z

zapcore/memory_encoder_test.go


+	wantTurducken := MapObjectEncoder{


* Add an internal multierror package We'll need a multierror implementation in both zap and zapcore to support arrays. Clean up the existing implementation a bit and promote it to its own internal package. * Add support for logging arrays Add basic support for logging arrays. To limit the scope of this commit, it introduces only arrays of ObjectMarshalers, arrays of arrays, and arrays of bools. It should, however, clarify how I intend to support arrays of other concrete types. Of note, I opted not to have `zap.Bools` return a `Field`. Instead, it returns a `zapcore.ArrayMarshaler`. This lets us build arrays of heterogenous arrays. For most users, this will be papered over by the upcoming `zap.Any(key string, val interface{}) zapcore.Field` function, which will basically be a huge type switch. * Simpler struct literals in tests * Update some comments * Add some simple benchmarks ``` BenchmarkBoolsArrayMarshaler-4 1000000 1028 ns/op 1088 B/op 3 allocs/op BenchmarkBoolsReflect-4 500000 3293 ns/op 2280 B/op 8 allocs/op ``` * Cleanups from CR

akshayjshah commented Jan 12, 2017

View reviewed changes

akshayjshah mentioned this pull request Jan 12, 2017

Support []int and []string in JSON encoder #211

Closed

prashantv reviewed Jan 12, 2017

View reviewed changes

prashantv mentioned this pull request Jan 12, 2017

Move buffers package to internal, add some test coverage #241

Merged

Akshay Shah added 5 commits January 12, 2017 11:14

Add an internal multierror package

73222c4

We'll need a multierror implementation in both zap and zapcore to support arrays. Clean up the existing implementation a bit and promote it to its own internal package.

Simpler struct literals in tests

921f06e

Update some comments

b1ea0b9

Add some simple benchmarks

8c010d8

``` BenchmarkBoolsArrayMarshaler-4 1000000 1028 ns/op 1088 B/op 3 allocs/op BenchmarkBoolsReflect-4 500000 3293 ns/op 2280 B/op 8 allocs/op ```

akshayjshah force-pushed the ajs-arrays branch from fd8edcb to 09d0045 Compare January 12, 2017 19:14

Cleanups from CR

4b45c11

akshayjshah force-pushed the ajs-arrays branch from 09d0045 to 4b45c11 Compare January 12, 2017 19:19

jcorbin reviewed Jan 12, 2017

View reviewed changes

jcorbin approved these changes Jan 12, 2017

View reviewed changes

akshayjshah merged commit 9e85108 into dev Jan 12, 2017

akshayjshah deleted the ajs-arrays branch January 12, 2017 21:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial support for arrays #239

Initial support for arrays #239

akshayjshah commented Jan 11, 2017 •

edited

Loading

akshayjshah Jan 12, 2017

prashantv Jan 12, 2017

akshayjshah Jan 12, 2017

prashantv Jan 12, 2017

prashantv Jan 12, 2017

akshayjshah Jan 12, 2017

prashantv Jan 12, 2017

akshayjshah Jan 12, 2017

prashantv Jan 12, 2017

prashantv Jan 12, 2017

prashantv Jan 12, 2017

prashantv Jan 12, 2017

jcorbin Jan 12, 2017

akshayjshah Jan 12, 2017

jcorbin Jan 12, 2017

akshayjshah Jan 12, 2017

jcorbin Jan 12, 2017

jcorbin Jan 12, 2017

akshayjshah Jan 12, 2017

jcorbin Jan 12, 2017

akshayjshah Jan 12, 2017

		@@ -0,0 +1,71 @@
		// Copyright (c) 2016 Uber Technologies, Inc.

Initial support for arrays #239

Initial support for arrays #239

Conversation

akshayjshah commented Jan 11, 2017 • 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

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

Choose a reason for hiding this comment

akshayjshah commented Jan 11, 2017 •

edited

Loading