writer: add CombineWriteSyncers #346

suyash · 2017-03-06T15:40:16Z

Allowing functions to take interfaces as parameters allows for better
customization.

For example, to have a writer that also counts the number of writes
specifically made to its instance

type MyWriter struct {
	*os.File
	logcount int
}

func (w *MyWriter) Write(data []byte) (int, error) {
	w.logcount++
	return w.File.Write(data)
}

func (w *MyWriter) Sync() error {
	return w.File.Sync()
}

...

func main() {
	f, err := os.Create(...)
	if err != nil {
		log.Fatal(err)
	}
	defer f.Close()

	w := &MyWriter{f, 0}
	ws, err := zap.OpenWriters(os.Stdout, w)
	if err != nil {
		log.Fatal(err)
	}

	core := zapcore.NewCore(
		zapcore.NewConsoleEncoder(zap.NewProductionEncoderConfig()),
		ws, zapcore.DebugLevel)

	logger := zap.New(core)

	...

	fmt.Printf("logged %v times to MyWriter", w.logcount)
}

Before opening your pull request, please make sure that you've:

signed Uber's Contributor License Agreement;
added tests to cover your changes;
run the test suite locally (make test); and finally,
run the linters locally (make lint).

Thanks for your contribution!

suyash · 2017-03-06T16:23:37Z

cc @peter-edge @akshayjshah

bufdev · 2017-03-06T16:56:43Z

writer.go

+// OpenWriters combines the passed io.Writer objects that are compatible with
+// zapcore.WriteSyncer into a locked WriteSyncer. It returns any errors
+// encountered.
+func OpenWriters(writers ...io.Writer) (zapcore.WriteSyncer, error) {


I don't think this function should be here - it somewhat defeats the purpose of static typing. Instead, there is a function in this package that can turn a Writer into a WriteSyncer, and then a user can pass the converted Writer to OpenWriteSyncers

Agreed. In cases where this is needed, I'd much prefer to have users deal with it explicitly.

Please remove this function.

bufdev · 2017-03-06T16:57:04Z

writer.go

+// OpenWriteSyncers combines the passed set of WriteSyncer objects into a locked
+// WriteSyncer. It returns any error encountered
+func OpenWriteSyncers(writers ...zapcore.WriteSyncer) (zapcore.WriteSyncer, error) {
+	if len(writers) == 0 {


I might just return nil here

I'd prefer to return a discarder, purely for consistency with New.

bufdev · 2017-03-06T16:57:17Z

writer.go

+	if len(writers) == 1 {
+		return zapcore.Lock(writers[0]), nil
+	}
+


nit: there's a lot of newlines here, maybe just remove them :)

akshayjshah · 2017-03-06T17:00:31Z

writer.go

+// OpenWriters combines the passed io.Writer objects that are compatible with
+// zapcore.WriteSyncer into a locked WriteSyncer. It returns any errors
+// encountered.
+func OpenWriters(writers ...io.Writer) (zapcore.WriteSyncer, error) {


Agreed. In cases where this is needed, I'd much prefer to have users deal with it explicitly.

Please remove this function.

akshayjshah · 2017-03-06T17:02:44Z

writer.go

+// OpenWriteSyncers combines the passed set of WriteSyncer objects into a locked
+// WriteSyncer. It returns any error encountered
+func OpenWriteSyncers(writers ...zapcore.WriteSyncer) (zapcore.WriteSyncer, error) {
+	if len(writers) == 0 {


I'd prefer to return a discarder, purely for consistency with New.

akshayjshah · 2017-03-06T17:03:33Z

writer.go

+
+	if len(writers) == 1 {
+		return zapcore.Lock(writers[0]), nil
+	}


Let's actually make this improvement to zapcore.NewMultiWriteSyncer - if it's passed just one argument, it should return that WriteSyncer unchanged. This wrapper can then rely on that behavior.

akshayjshah · 2017-03-06T17:04:49Z

writer_test.go

+
+type testWriter struct {
+	expected []byte
+	t        *testing.T


For future extensibility, let's make this a testing.TB.

akshayjshah · 2017-03-06T17:05:32Z

writer_test.go

+	w, err := OpenWriters(tw)
+	if err != nil {
+		t.Fatalf("OpenWriters failed, error: %v", err)
+	}


Instead, please use the require package, which is already included as part of testify.

require.NoError(t, err, "OpenWriters failed.")

suyash · 2017-03-08T13:08:03Z

@akshayjshah ping

akshayjshah

Thanks for updating! There's still a little work to do here.

akshayjshah · 2017-03-08T16:39:30Z

writer.go

+		return zapcore.AddSync(ioutil.Discard), nil
+	}
+	return zapcore.Lock(zapcore.NewMultiWriteSyncer(writers...)), nil
+}


Please remove this function; I'd much rather export just Open.

@akshayjshah the whole point of this PR was to get a function in the API that takes interfaces instead of strings as output file paths.

Please see my first comment

Ack, my fault - I confused this with the previously-present OpenWriters.

Since the files/writers are already open, let's rename this to CombineWriteSyncers or ComposeWriteSyncers instead. This function can't fail, so let's also remove the error return value.

akshayjshah · 2017-03-08T16:39:56Z

zapcore/write_syncer.go

@@ -88,6 +88,9 @@ type multiWriteSyncer []WriteSyncer
 // NewMultiWriteSyncer creates a WriteSyncer that duplicates its writes
 // and sync calls, much like io.MultiWriter.
 func NewMultiWriteSyncer(ws ...WriteSyncer) WriteSyncer {
+	if len(ws) == 1 {
+		return ws[0]
+	}


Needs test coverage.

Added, Currently simply verifying that passing just one argument to NewMultiWriteSyncer simply returns the argument back.

✅ Excellent.

Allowing functions to take interfaces as parameters allows for better customization. For example, to have a writer that also counts the number of writes specifically made to its instance ``` type MyWriter struct { *os.File logcount int } func (w *MyWriter) Write(data []byte) (int, error) { w.logcount++ return w.File.Write(data) } func (w *MyWriter) Sync() error { return w.File.Sync() } ... func main() { f, err := os.Create(...) if err != nil { log.Fatal(err) } defer f.Close() w := &MyWriter{f, 0} ws, err := zap.OpenWriteSyncers(os.Stdout, zapcore.AddSync(w)) if err != nil { log.Fatal(err) } core := zapcore.NewCore( zapcore.NewConsoleEncoder(zap.NewProductionEncoderConfig()), ws, zapcore.DebugLevel) logger := zap.New(core) ... fmt.Printf("logged %v times to MyWriter", w.logcount) } ```

akshayjshah

Small nits remaining, but generally looks good.

Please don't amend your existing commits - it makes reviewing this PR more difficult. Since we always squash before merging, having lots of tiny commits in the PR won't introduce noise on master.

akshayjshah · 2017-03-08T17:21:12Z

writer.go

+		return zapcore.AddSync(ioutil.Discard), nil
+	}
+	return zapcore.Lock(zapcore.NewMultiWriteSyncer(writers...)), nil
+}


Ack, my fault - I confused this with the previously-present OpenWriters.

Since the files/writers are already open, let's rename this to CombineWriteSyncers or ComposeWriteSyncers instead. This function can't fail, so let's also remove the error return value.

akshayjshah · 2017-03-08T17:23:41Z

writer_test.go

+	w, err := OpenWriteSyncers(tw)
+	if err != nil {
+		require.NoError(t, err, "OpenWriters Failed.")
+	}


No need for the conditional.

akshayjshah · 2017-03-08T17:25:45Z

writer_test.go

+}
+
+func (w *testWriter) Write(actual []byte) (int, error) {
+	assert.Equal(w.t, w.expected, actual, "expected writer to write %v, wrote %v", string(w.expected), string(actual))


The failure message automatically contains the expected and actual values, so we can simplify this to

assert.Equal(w.t, w.expected, string(actual), "Unexpected write call.")

akshayjshah · 2017-03-08T17:26:05Z

writer_test.go

@@ -81,3 +81,28 @@ func TestOpen(t *testing.T) {
 		assert.Equal(t, tt.filenames, names, "Opened unexpected files given paths %v.", tt.paths)
 	}
 }
+
+type testWriter struct {
+	expected []byte


If we make this a string, we'll get better output on test failures and easier initialization.

akshayjshah · 2017-03-08T17:26:33Z

writer_test.go

+
+func (w *testWriter) Write(actual []byte) (int, error) {
+	assert.Equal(w.t, w.expected, actual, "expected writer to write %v, wrote %v", string(w.expected), string(actual))
+	return 0, nil


Let's fulfill the writer contract and return the correct number of bytes.

returning the length of actual

akshayjshah · 2017-03-08T17:27:21Z

zapcore/write_syncer_test.go

@@ -65,6 +65,12 @@ func TestAddSyncWriter(t *testing.T) {
 	assert.NoError(t, ws.Sync(), "Unexpected error calling a no-op Sync method.")
 }

+func TestNewMultiWriteSyncerWorksForSingleWriter(t *testing.T) {
+	w := AddSync(&bytes.Buffer{})


We can use testutils.Buffer here instead.

akshayjshah · 2017-03-08T17:27:41Z

zapcore/write_syncer.go

@@ -88,6 +88,9 @@ type multiWriteSyncer []WriteSyncer
 // NewMultiWriteSyncer creates a WriteSyncer that duplicates its writes
 // and sync calls, much like io.MultiWriter.
 func NewMultiWriteSyncer(ws ...WriteSyncer) WriteSyncer {
+	if len(ws) == 1 {
+		return ws[0]
+	}


✅ Excellent.

suyash mentioned this pull request Mar 6, 2017

Allow external configuration of logger performancecopilot/speed#36

Closed

bufdev suggested changes Mar 6, 2017

View reviewed changes

akshayjshah suggested changes Mar 6, 2017

View reviewed changes

bufdev approved these changes Mar 6, 2017

View reviewed changes

suyash changed the title ~~writer: add OpenWriters and OpenWriteSyncers~~ writer: add OpenWriteSyncers Mar 6, 2017

akshayjshah reviewed Mar 8, 2017

View reviewed changes

rename to CombineWriteSyncers

a592c8c

suyash changed the title ~~writer: add OpenWriteSyncers~~ writer: add CombineWriteSyncers Mar 8, 2017

fix Test Function name

88322a5

akshayjshah approved these changes Mar 8, 2017

View reviewed changes

Merge branch 'master' into open_io_writer

59545c4

akshayjshah merged commit 179e456 into uber-go:master Mar 8, 2017

suyash deleted the open_io_writer branch March 9, 2017 10:22

github-actions bot mentioned this pull request Apr 10, 2022

Link Checker Report scylladb/scylla-manager#3102

Closed

github-actions bot mentioned this pull request Apr 17, 2022

Link Checker Report scylladb/scylla-manager#3121

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

writer: add CombineWriteSyncers #346

writer: add CombineWriteSyncers #346

suyash commented Mar 6, 2017

suyash commented Mar 6, 2017

bufdev Mar 6, 2017

akshayjshah Mar 6, 2017

bufdev Mar 6, 2017

akshayjshah Mar 6, 2017

bufdev Mar 6, 2017

akshayjshah Mar 6, 2017

akshayjshah Mar 6, 2017

akshayjshah Mar 6, 2017

akshayjshah Mar 6, 2017

akshayjshah Mar 6, 2017

suyash commented Mar 8, 2017

akshayjshah left a comment

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

akshayjshah left a comment

akshayjshah Mar 8, 2017

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

suyash Mar 8, 2017

akshayjshah Mar 8, 2017

writer: add CombineWriteSyncers #346

writer: add CombineWriteSyncers #346

Conversation

suyash commented Mar 6, 2017

suyash commented Mar 6, 2017

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

suyash commented Mar 8, 2017

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment