Ability to build logger with custom Sink #572

dimroc · 2018-04-10T23:44:39Z

Introduce the ability to pass in Sinks enabling one to customize a logger's Writer (technically WriteSyncer) and Closer.

Completes issue Ability to have custom sinks #553
Allows clean up of https://github.com/uber-go/zap/pull/506/files#diff-dc03dbc53655fac7a4c8d16b1b4144c1R85 by allowing one to place syslog logic like syslog.Dial outside of writer.go#Open (syslog writer #506)
Enables the creation of future sinks without the need to tamper with writer.go#Open

Sample usage:

func CreateLogger() *zap.Logger {
	config := zap.NewProductionConfig()
	file := path.Join("/tmp", "log.jsonl")

        RegisterSink("customsink",  func() (Sink, error) { return CustomSink{}, nil })
	config.OutputPaths = []string{"stdout", "customsink", file}
	zl, err := config.Build()

	if err != nil {
		log.Fatal(err)
	}
	return zl
}

// Complies with type Sink interface (zapcore.WriteSyncer, io.Closer)
type CustomSink struct{}

func (CustomSink) Sync() error  { return nil }
func (CustomSink) Close() error { return nil }

func (CustomSink) Write(p []byte) (int, error) {
	log.Print(string(p)) // send to syslog, or do anything
	return len(p), nil
}

CLAassistant · 2018-04-10T23:44:47Z

All committers have signed the CLA.

codecov · 2018-04-11T18:01:23Z

Codecov Report

Merging #572 into master will decrease coverage by 0.23%.
The diff coverage is 95%.

@@            Coverage Diff             @@
##           master     #572      +/-   ##
==========================================
- Coverage   97.47%   97.23%   -0.24%     
==========================================
  Files          39       40       +1     
  Lines        2017     2063      +46     
==========================================
+ Hits         1966     2006      +40     
- Misses         43       49       +6     
  Partials        8        8

Impacted Files	Coverage Δ
writer.go	`100% <100%> (ø)`	⬆️
sink.go	`92.59% <92.59%> (ø)`
buffer/buffer.go	`87.09% <0%> (-12.91%)`	⬇️
zapcore/json_encoder.go	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update eeedf31...3c29566. Read the comment docs.

dimroc · 2018-04-11T20:55:13Z

The core of the change can be found here https://github.com/uber-go/zap/pull/572/files#diff-dc03dbc53655fac7a4c8d16b1b4144c1L59

akshayjshah

Thank you for this PR, @dimroc! I'd like to discuss high-level direction with a few of the other maintainers before getting into the details of the code.

akshayjshah · 2018-04-13T19:02:27Z

config.go

+// passed options. For example, the option "stdout" will construct a logger
+// that routes output to the stdout sink, but "/filepath" will fallback
+// to writing to file.
+func (cfg Config) BuildWithSinks(sm map[string]Sink, opts ...Option) (*Logger, error) {


This is a nice idea - I'd like to make this portion of zap more extensible. I also like that this approach avoids adding global state.

However, this works very differently from extending encoders: for that, we use a global registry and a top-level RegisterEncoder function. For consistency, I'd actually prefer to add another global registry for sinks. (Generally, I much prefer dependency injection, but I think consistency is more important here.)

@prashantv and @abhinav, what do you think?

I think I'd prefer consistency as well here, and have a global registry for both encoders and sinks.

If we want users to be able to specify the exact registries, we'll likely want to add a single method that accepts both registries rather than a method for each.

Thanks for the feedback @akshayjshah @prashantv. I never saw RegisterEncoder but after seeing it, your requests make complete sense. I have since added a commit with RegisterSink to mimc that functionality. If @abhinav feels strongly about reverting back to the dependency injection style, no worries, we can just revert one commit.

dimroc · 2018-04-16T13:10:49Z

writer.go

-			// Don't close standard out.
+		if sink, err := NewSink(path); err == nil {
+			writers = append(writers, sink)
+			closers = append(closers, sink)


I kept the two lists, writers and closers, because I didn't want to be too aggressive with the refactor and change the return value of
func open(paths []string) ([]zapcore.WriteSyncer, func(), error)
to
func open(paths []string) ([]Sink, func(), error)

It would have rippled out to affect a few other funcs and we would have needed an array of []zapcore.WriteSyncer for CombineWriteSyncers anyways.

dimroc · 2018-04-16T13:21:16Z

writer.go

-			// Don't close standard out.
+		if sink, err := newSink(path); err == nil {
+			writers = append(writers, sink)
+			closers = append(closers, sink)


I kept the two lists, writers and closers, because I didn't want to be too aggressive with the refactor and change the return value of
func open(paths []string) ([]zapcore.WriteSyncer, func(), error)
to
func open(paths []string) ([]Sink, func(), error)

It would have rippled out to affect a few other funcs and we would have needed an array of []zapcore.WriteSyncer for CombineWriteSyncers anyways.

Akin to RegisterEncoder

etsangsplk · 2018-05-12T02:39:44Z

@akshayjshah
What's the decision??

ghost · 2018-05-23T03:20:36Z

I'd also like to see this merged. Any update?

se3000 · 2018-06-05T21:37:42Z

Would love to see this merged, it'd be very helpful for us. What's left to be done?

akshayjshah · 2018-06-27T03:30:18Z

This is great. I'm going to push a commit tomorrow to un-export a few of the helper structs, but the core of this is excellent - thanks for the work and the refactoring, @dimroc.

To everyone waiting on this, thanks for your patience. Work and fatherhood distracted me for the past few months.

Expect a release with this functionality by the end of the week.

@dimroc

@dimroc did all the work for this feature already. This commit un-exports NopCloserSink (since we're only using it in tests), adds an unexported function to reset the global registry of sinks, and includes the user-supplied key in the no-such-sink error message.

akshayjshah · 2018-06-27T23:44:48Z

Merged! I'm going to open PRs to prep a release tomorrow. We use this package extensively internally; for my own sanity and family life, I'm not going to cut a release on a Friday just in case something goes horribly wrong. I'll tag a new relase first thing monday morning.

prashantv · 2018-06-28T22:53:02Z

sink.go

+}
+
+func resetSinkRegistry() {
+	_sinkMutex.Lock()


Should we have this mutex? It seems like we'd be masking a race where loggers might fail to build because the scheme isn't recognized by the sink?

Certainly possible. We lock around the encoder registry; IMO, consistency with that API is important here.

prashantv · 2018-06-28T22:54:40Z

sink.go

+
+// RegisterSink adds a Sink at the given key so it can be referenced
+// in config OutputPaths.
+func RegisterSink(key string, sinkFactory func() (Sink, error)) error {


This will work well for static sinks, but I think we might need something more dynamic (e.g., a prefix match, or a URI scheme match). E.g., even for something like syslog, we'd probably need more info like port. I'd prefer to have less APIs exposed, so one that is more flexible might be better

That's a good point - I hadn't thought about this use case.

I'd prefer to avoid a URI-based scheme here, since the current special-cased sinks don't use file://. Prefix match would work, as long as we ensure that we don't get any conflicting rules.

Actually, URIs with special cases for stdout and stderr might be best 🤔

prashantv · 2018-06-28T22:54:58Z

sink.go

+// in config OutputPaths.
+func RegisterSink(key string, sinkFactory func() (Sink, error)) error {
+	_sinkMutex.Lock()
+	defer _sinkMutex.Unlock()


optional nit: blank after defer to separate it from the rest of the logic (same for newSink)

👍 will do.

DGollings · 2021-01-02T19:50:47Z

I'm either the first or the least intelligent person to attempt to use this custom sink stuff. Took me quite some time and digging through issues/code to figure this out. To anyone wondering why the example doesn't work, it has changed slightly due to #606:

func CreateLogger() *zap.Logger {
	config := zap.NewProductionConfig()
	file := path.Join("/tmp", "log.jsonl")

        RegisterSink("customsink://",  func() (Sink, error) { return CustomSink{}, nil })
	config.OutputPaths = []string{"stdout", "customsink", file}
	zl, err := config.Build()

	if err != nil {
		log.Fatal(err)
	}
	return zl
}

// Complies with type Sink interface (zapcore.WriteSyncer, io.Closer)
type CustomSink struct{}

func (CustomSink) Sync() error  { return nil }
func (CustomSink) Close() error { return nil }

func (CustomSink) Write(p []byte) (int, error) {
	log.Print(string(p)) // send to syslog, or do anything
	return len(p), nil
}

RegisterSink("customsink", func() (Sink, error) { return CustomSink{}, nil })
should be
RegisterSink("customsink://", func() (Sink, error) { return CustomSink{}, nil })

Using the same global-registry pattern that we're using for encoders, let users register new sinks (like the built-in `stdout` and `stderr`).

Ability to build logger with custom SinkFactories

e41e067

dimroc force-pushed the features/custom-sinks branch from 925dfda to e41e067 Compare April 11, 2018 18:01

Simplify SinkFactory from interface to struct

40845ba

dimroc force-pushed the features/custom-sinks branch 3 times, most recently from d38dd8c to 1bc0592 Compare April 11, 2018 20:06

dimroc changed the title ~~Ability to build logger with custom SinkFactories~~ Ability to build logger with custom Sink Apr 11, 2018

dimroc force-pushed the features/custom-sinks branch from 1bc0592 to 5398132 Compare April 11, 2018 20:50

Intro Sink, alias for WriteSyncerCloser interface

9b29574

dimroc force-pushed the features/custom-sinks branch from 5398132 to 9b29574 Compare April 11, 2018 21:42

assert that log file is created by writer

08c8e44

dimroc force-pushed the features/custom-sinks branch from 3db4940 to 08c8e44 Compare April 12, 2018 19:53

akshayjshah reviewed Apr 13, 2018

View reviewed changes

dimroc commented Apr 16, 2018

View reviewed changes

dimroc force-pushed the features/custom-sinks branch from 1773000 to e2b8087 Compare April 16, 2018 13:20

dimroc commented Apr 16, 2018

View reviewed changes

Refactor DefaultSinks() into RegisterSink

de223e7

Akin to RegisterEncoder

dimroc force-pushed the features/custom-sinks branch from e2b8087 to de223e7 Compare April 16, 2018 18:31

Add test coverage for writer erroring on newSink

125fe34

Small review cleanups

3c29566

@dimroc did all the work for this feature already. This commit un-exports NopCloserSink (since we're only using it in tests), adds an unexported function to reset the global registry of sinks, and includes the user-supplied key in the no-such-sink error message.

akshayjshah merged commit 7e7e266 into uber-go:master Jun 27, 2018

prashantv reviewed Jun 28, 2018

View reviewed changes

cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022

Ability to build logger with custom Sink (uber-go#572)

8d4533c

Using the same global-registry pattern that we're using for encoders, let users register new sinks (like the built-in `stdout` and `stderr`).

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

Ability to build logger with custom Sink #572

Ability to build logger with custom Sink #572

dimroc commented Apr 10, 2018 •

edited

Loading

CLAassistant commented Apr 10, 2018 •

edited

Loading

codecov bot commented Apr 11, 2018 •

edited

Loading

dimroc commented Apr 11, 2018

akshayjshah left a comment

akshayjshah Apr 13, 2018 •

edited

Loading

prashantv Apr 13, 2018

dimroc Apr 16, 2018

dimroc Apr 16, 2018

dimroc Apr 16, 2018

etsangsplk commented May 12, 2018

ghost commented May 23, 2018

se3000 commented Jun 5, 2018

akshayjshah commented Jun 27, 2018

akshayjshah commented Jun 27, 2018

prashantv Jun 28, 2018

akshayjshah Jun 29, 2018

prashantv Jun 28, 2018

akshayjshah Jun 29, 2018 •

edited

Loading

akshayjshah Jun 29, 2018

akshayjshah Jun 30, 2018

prashantv Jun 28, 2018

akshayjshah Jun 29, 2018

DGollings commented Jan 2, 2021

Ability to build logger with custom Sink #572

Ability to build logger with custom Sink #572

Conversation

dimroc commented Apr 10, 2018 • edited Loading

CLAassistant commented Apr 10, 2018 • edited Loading

codecov bot commented Apr 11, 2018 • edited Loading

Codecov Report

dimroc commented Apr 11, 2018

akshayjshah left a comment

Choose a reason for hiding this comment

akshayjshah Apr 13, 2018 • 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

etsangsplk commented May 12, 2018

ghost commented May 23, 2018

se3000 commented Jun 5, 2018

akshayjshah commented Jun 27, 2018

akshayjshah commented Jun 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akshayjshah Jun 29, 2018 • 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

DGollings commented Jan 2, 2021

dimroc commented Apr 10, 2018 •

edited

Loading

CLAassistant commented Apr 10, 2018 •

edited

Loading

codecov bot commented Apr 11, 2018 •

edited

Loading

akshayjshah Apr 13, 2018 •

edited

Loading

akshayjshah Jun 29, 2018 •

edited

Loading