Make times and durations first-class in encoders #264
Conversation
Refactor the JSON encoder's configuration to separate the desired keys (for information contained in each entry) from the desired time, duration, and level formats. This paves the way for a solution to #259.
|
@akshayjshah, thanks for your PR! By analyzing the history of the files in this pull request, we identified @jcorbin and @peter-edge to be potential reviewers. |
benchmarks/zap_bench_test.go
Outdated
| TimeKey: "ts", | ||
| CallerKey: "caller", | ||
| StacktraceKey: "stacktrace", | ||
| TimeFormatter: func(t time.Time, enc zapcore.ArrayEncoder) { enc.AppendInt64(t.UnixNano() / int64(time.Millisecond)) }, |
There was a problem hiding this comment.
I feel like these functions could be useful, maybe even exported, vars in zapcore, e.g.: func TimeEncodedAsEpochMS(t time.Time, enc zapcore.ArrayEncoder) { enc.AppendInt64(t.UnixNano() / int64(time.Millisecond)) }. Also, I feel that such a default should be floating point ms, not truncated int ms.
There was a problem hiding this comment.
Agreed, but I'd like to defer exporting such functions until we do config; I suspect that the two will be closely related. In this case, I'd want the benchmarks to use whatever the default configuration is.
I'm all for changing the default (agreed that what we have is weird). I'll do that in a separate PR because it introduces lots of test assertion changes.
zapcore/encoder.go
Outdated
| // Configure the primitive representations of common complex types. For | ||
| // example, some users may want all time.Times serialized as floating-point | ||
| // seconds since epoch, while others may prefer ISO8601 strings. | ||
| LevelFormatter func(Level, ArrayEncoder) |
There was a problem hiding this comment.
nit: why not keep with the "Encoder" nomenclature, and call these e.g. LevelEncoder?
Or, since they're raw functions, the lest KoN'd form would be func EncodeLevel(Level, ArrayEncoder).
There was a problem hiding this comment.
EncodeFoo and FormatFoo sound good. LevelEncoder sounds like JSONEncoder, but is a whole different beast.
| @@ -60,10 +52,10 @@ type jsonEncoder struct { | |||
| // libraries will ignore duplicate key-value pairs (typically keeping the last | |||
| // pair) when unmarshaling, but users should attempt to avoid adding duplicate | |||
| // keys. | |||
| func NewJSONEncoder(cfg JSONConfig) Encoder { | |||
| func NewJSONEncoder(cfg EncoderConfig) Encoder { | |||
There was a problem hiding this comment.
I guess it's mildly weird that we store a pointer to a passed value, rather than letting the caller pass a pointer (e.g. so that multiple json-encoded facilities can share the same config). Doubly so if this new config achieves it's namesake of being the config for all encoders, for sharing b/w console and json.
There was a problem hiding this comment.
Accepting a pointer to config suggests, to me, that the config is optional. In this case, it's not.
In the case when we have both JSON and console encoders running, I don't think they'll share configs - they'll want different time and level formatters, at least.
zapcore/json_encoder.go
Outdated
| if ent.Caller.Defined { | ||
| if final.LevelKey != "" { | ||
| final.addKey(final.LevelKey) | ||
| final.LevelFormatter(ent.Level, final) |
There was a problem hiding this comment.
nit: any of FormatLevel, EncodeLevel, or AppendLevel would fit better as the internal field name.
zapcore/json_encoder.go
Outdated
| // All the formatters may have been no-ops. | ||
| final.bytes = append(final.bytes, ',') | ||
| } | ||
| final.separateElements() |
There was a problem hiding this comment.
nit: appendElementSeparator (or addElementSeparator if you prefer)
Rename FooFormatter to EncodeFoo, and rename separateElements to addElementSeparator.
* Refactor JSON encoder's configuration Refactor the JSON encoder's configuration to separate the desired keys (for information contained in each entry) from the desired time, duration, and level formats. This paves the way for a solution to #259. * Make times and durations first-class in encoders * Feedback from CR Rename FooFormatter to EncodeFoo, and rename separateElements to addElementSeparator.
This PR resolves #259 in two steps.
The first commit is a pre-factoring. It makes encoder configuration more modular, splitting configuration of message/name/timestamp keys from the formatting of the values. (This was enabled by our work on array support.) It paves the way to fix #149 too. Two things to note about that change:
After pre-factoring, the second commit is quite small. It makes durations and times first-class types in encoders (including arrays of each).