Re-introduce terse configuration #274

akshayjshah · 2017-02-04T03:01:00Z

This PR re-introduces a nice, terse way to configure sane-by-default loggers. It also introduces a configuration struct for loggers, since many applications want to drive logger configuration from config instead of code.

mention-bot · 2017-02-04T03:01:00Z

@akshayjshah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jcorbin and @prashantv to be potential reviewers.

akshayjshah · 2017-02-04T03:02:31Z

zapcore/encoder.go

@@ -80,16 +182,32 @@ type ObjectEncoder interface {
 // arrays even though they aren't typical in Go. Like slices, ArrayEncoders
 // aren't safe for concurrent use (though typical use shouldn't require locks).
 type ArrayEncoder interface {
+	// Built-in types
+	PrimitiveArrayEncoder


I'll break out this change into a separate PR - it was an oversight on my part. Passing an ArrayEncoder to a TimeEncoder opens us up to infinite recursion :/

Amend `zapcore.Level`'s text unmarshaling logic to convert the zero-value string to the zero-value Level, then add text unmarshaling support to atomic levels too.

Provide an all-in-one wrapper around the open/tee/lock dance.

prashantv · 2017-02-09T22:14:48Z

config.go

+// global CPU and I/O load that logging puts on your process while preserving a
+// representative subset of your logs.
+//
+// See zapcore.Sample for details.


Should we mention that these values are per-second?

prashantv · 2017-02-09T22:18:22Z

config.go

+	Development bool `json:"development",yaml:"development"`
+	// DisableCaller stops annotating logs with the calling function's file
+	// name and line number.
+	DisableCaller bool `json:"disable_caller",yaml:"disable_caller"`


Any reason for snake case in JSON? I tend to use the google JSON style guide which suggests camelCase
https://google.github.io/styleguide/jsoncstyleguide.xml#Property_Name_Format

I'm not sure what's appropriate for YAML, don't think there's any standard, I've seen hyphernated-case as well as camelCase and snake_case

Seems like everyone just ports the convention from their primary programming language. FB's API is snake-cased, Twitter's API is snake-cased, and the portions of the AWS APIs I've used are snake-cased.

I don't really care much though. Happy to change if you have a strong preference.

prashantv · 2017-02-09T22:20:08Z

config.go

+	// DisableCaller stops annotating logs with the calling function's file
+	// name and line number.
+	DisableCaller bool `json:"disable_caller",yaml:"disable_caller"`
+	// DisableStacktrace completely disables automatic stacktrace capturing.


Can you add information about when stack traces are captured (looks like error or higher for prod, and warn or higher for dev)

Great minds do indeed think alike. 👍 👍

prashantv · 2017-02-09T22:31:03Z

config.go

+			EncodeTime:     zapcore.EpochTimeEncoder,
+			EncodeDuration: zapcore.SecondsDurationEncoder,
+		},
+		OutputPaths:      []string{"stdout"},


Should the default output for logs be stderr?
logrus seems to use stderr by default: https://godoc.org/github.com/sirupsen/logrus#Logger
As does the standard go log package.

(Ideally production configs would actually use a file, but that's not a great default)

Sure, sounds good. This is yet another place where Go's standard lib chose to go down an unusual route :(

We'll have to disagree about prod logs - I still think they should log to standard out, and it's the process manager's job to pipe that to a file. That's the 12-factor standard.

prashantv · 2017-02-09T22:32:29Z

config.go

+			EncodeTime:     zapcore.ISO8601TimeEncoder,
+			EncodeDuration: zapcore.StringDurationEncoder,
+		},
+		OutputPaths:      []string{"stdout"},


same as earlier, might want to use stderr for all logs

prashantv · 2017-02-09T22:36:27Z

config.go

+	}
+
+	if cfg.Sampling != nil {
+		opts = append(opts, WrapFacility(func(fac zapcore.Facility) zapcore.Facility {


(just a question) is there a difference between wrapping the facility when passing it into New vs calling WrapFacility?

Nope. This was just a bit easier organizationally - otherwise creating the facility needed its own helper method.

prashantv · 2017-02-09T22:39:21Z

writer.go

+		return zapcore.AddSync(ioutil.Discard), nil
+	}
+	writers, err := open(paths)
+	return zapcore.Lock(zapcore.MultiWriteSyncer(writers...)), err


Should MultiWriteSyncer always lock?

Not sure exactly what you're asking here. Since we're opening paths, the WriteSyncers in question are files, so we always need a lock.

In general, though, I don't think MultiWriteSyncers should lock - it's perfectly reasonable to write to both a file and a Kafka client, in which case locking should happen underneath the tee layer.

prashantv · 2017-02-09T22:40:10Z

writer.go

+func Open(paths ...string) (zapcore.WriteSyncer, error) {
+	if len(paths) == 0 {
+		return zapcore.AddSync(ioutil.Discard), nil
+	}


can we special case len(paths) == 1 and return the WriteSyncer directly

prashantv · 2017-02-09T22:44:05Z

writer.go

+			writers = append(writers, os.Stderr)
+			continue
+		}
+		f, err := os.OpenFile(path, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0644)


Since we never read the file, I think we can use O_WRONLY.

For the mask, it is then umasked, so create uses 0666:
https://golang.org/src/os/file.go?s=7623:7662#L248

We should probably do the same

prashantv · 2017-02-09T22:45:10Z

zapcore/sampler.go

-// Sample creates a facility that samples incoming entries.
+// Sample creates a Facility that samples incoming entries, which caps the CPU
+// and I/O load of logging while preserving a representative subset of your
+// logs.


should we mention the "optimized for performance over correctness" here as well?

Sure. I was saving that until the sampler fix lands, but this is a perfectly fine time to add it.

jcorbin · 2017-02-09T22:11:51Z

config.go

+)
+
+// SamplingConfig sets a sampling strategy for the logger. Sampling caps the
+// global CPU and I/O load that logging puts on your process while preserving a


I'd prefer to soften this to "while attempting to preserve a".

jcorbin · 2017-02-09T22:14:07Z

config.go

+	// Development puts the logger in development mode, which changes the
+	// behavior of DPanicLevel and takes stacktraces more liberally.
+	Development bool `json:"development",yaml:"development"`
+	// DisableCaller stops annotating logs with the calling function's file


Say more about when it happens if not enabled, always? Only for warn and up?

Good point.

jcorbin · 2017-02-09T22:14:15Z

config.go

+	// DisableCaller stops annotating logs with the calling function's file
+	// name and line number.
+	DisableCaller bool `json:"disable_caller",yaml:"disable_caller"`
+	// DisableStacktrace completely disables automatic stacktrace capturing.


Say more about when it happens if not enabled, always? Only for error and up?

jcorbin · 2017-02-09T22:14:44Z

config.go

+	// level, so calling Config.Level.SetLevel will atomically change the log
+	// level of all loggers descended from this config. The zero value is
+	// InfoLevel.
+	Level AtomicLevel `json:"level",yaml:"level"`


How about a:

Level Level AllowLevelChange bool

And defaulting to static levels?

Why? The performance overhead is near-zero, and I'd always recommend using a dynamic level.

jcorbin · 2017-02-09T22:18:41Z

config.go

+	DisableStacktrace bool `json:"disable_stacktrace",yaml:"disable_stacktrace"`
+	// Sampling sets a sampling policy. A nil SamplingConfig disables sampling.
+	Sampling *SamplingConfig `json:"sampling",yaml:"sampling"`
+	// Encoding sets the logger's encoding. Valid values are "json" and


Interesting, I would've expected more of a:

EnableConsole bool // maybe disable instead of enable DisableStandardErrors bool FileEncoding string OutputPaths []string ErrorOutputPaths []string

The idea being:

stdout is always console encoded

files are always json encoded

errors at least go to stderr, and maybe also a file

I'm glad that we can support that setup, but I'm less of a fan of making that part of the quickstart. I remembered discussing it with you and played with it for a while, and I found it pretty jarring. IMO it's just too unusual.

If and when we add new encoders, I'd also like this config struct to support them. Again, I have msgpack in production in mind.

jcorbin · 2017-02-09T22:33:31Z

level.go

+// "panic", and "fatal").
+func (lvl *AtomicLevel) UnmarshalText(text []byte) error {
+	if lvl.l == nil {
+		lvl.l = atomic.NewInt32(int32(InfoLevel))


Prefer to defer thus until after we've parsed without err.

jcorbin · 2017-02-09T22:34:28Z

level.go

+	if err := l.UnmarshalText(text); err != nil {
+		return err
+	}
+	lvl.SetLevel(l)


Prefer to:

if lvl.l == nil { lvl.l = atomic.NewInt32(int32(l)) } else { lvl.SetLevel(l) }

jcorbin · 2017-02-09T22:36:42Z

logger.go

@@ -105,6 +99,16 @@ func (log *Logger) Named(s string) *Logger {
 	return l
 }

+// WithOptions clones the current Logger, applies the supplied Options, and
+// returns the result. It's safe to use concurrently.


"... and returns the clone." (... "cloned child logger." ?)

jcorbin · 2017-02-09T22:43:51Z

writer.go

+			writers = append(writers, os.Stderr)
+			continue
+		}
+		f, err := os.OpenFile(path, os.O_RDWR|os.O_APPEND|os.O_CREATE, 0644)


if this were a default:, you wouldn't need those continues

Yes, but then I'd need another level of nesting. To me, this is similar to an early return. Reasonable?

jcorbin · 2017-02-09T22:47:41Z

zapcore/sampler.go

@@ -65,7 +65,13 @@ func fnv32a(s string) uint32 {
 	return hash
 }

-// Sample creates a facility that samples incoming entries.
+// Sample creates a Facility that samples incoming entries, which caps the CPU
+// and I/O load of logging while preserving a representative subset of your


nit: "attempts to preserve a"

akshayjshah · 2017-02-09T23:55:41Z

Addressed all comments except:

~~1. Casing of JSON/YAML keys in config struct. Not opposed to changing, but the widely-used APIs I'm familiar with prefer snake case.~~ Converted to camel-case, since this is a Go library and Go camel-cases names.
2. Different encodings for different outputs. In practice, feels jarring to me; also complicates config somewhat.

I'll hold off on rebasing until all CR is resolved.

jcorbin

Makes sense, I can live with it :-)

Re-introduce a nice, terse way to configure sane-by-default loggers. Also, introduce a configuration struct for loggers, since many applications want to drive logger configuration from config instead of code.

akshayjshah requested review from jcorbin and prashantv February 4, 2017 03:01

akshayjshah commented Feb 4, 2017

View reviewed changes

akshayjshah force-pushed the ajs-config branch 2 times, most recently from 1c6b27f to 75f58f4 Compare February 9, 2017 01:34

akshayjshah changed the title ~~WIP: Re-introduce terse configuration~~ Re-introduce terse configuration Feb 9, 2017

akshayjshah force-pushed the ajs-config branch from 75f58f4 to a18df2a Compare February 9, 2017 01:35

Akshay Shah added 7 commits February 9, 2017 09:13

Add support for unmarshaling dynamic levels from text

7ef50e7

Amend `zapcore.Level`'s text unmarshaling logic to convert the zero-value string to the zero-value Level, then add text unmarshaling support to atomic levels too.

Rename AddStacks to AddStacktrace

89985d9

Add an option to wrap or replace a logger's Facility

88f8278

Add a utility function to open a collection of files

1d7135e

Provide an all-in-one wrapper around the open/tee/lock dance.

Re-introduce the Hooks option

df76856

Add config-based logger instantiation

3a8d313

Improve sampling documentation

c9082a0

akshayjshah force-pushed the ajs-config branch from a18df2a to c9082a0 Compare February 9, 2017 17:21

prashantv reviewed Feb 9, 2017

View reviewed changes

jcorbin suggested changes Feb 9, 2017

View reviewed changes

Fixes from CR

2a8cabd

Camel-case JSON and YAML keys

a91397a

jcorbin approved these changes Feb 10, 2017

View reviewed changes

Akshay Shah and others added 2 commits February 9, 2017 16:50

More cleanup

3c639bc

Merge branch 'dev' into ajs-config

f9cfa56

akshayjshah merged commit 4277ba7 into dev Feb 10, 2017

akshayjshah deleted the ajs-config branch February 10, 2017 16:38

akshayjshah mentioned this pull request Feb 10, 2017

Re-introduce terse logger instantiation #221

Closed

Re-introduce terse configuration #274

Re-introduce terse configuration #274

Conversation

akshayjshah commented Feb 4, 2017 • edited Loading

mention-bot commented Feb 4, 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

akshayjshah Feb 9, 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

akshayjshah Feb 9, 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

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 Feb 9, 2017 • edited Loading

jcorbin left a comment

Choose a reason for hiding this comment

akshayjshah commented Feb 4, 2017 •

edited

Loading

akshayjshah Feb 9, 2017 •

edited

Loading

akshayjshah Feb 9, 2017 •

edited

Loading

akshayjshah commented Feb 9, 2017 •

edited

Loading