-
Notifications
You must be signed in to change notification settings - Fork 1.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Optional feedback, mostly around naming.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: any of FormatLevel
, EncodeLevel
, or AppendLevel
would fit better as the internal field name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: appendElementSeparator
(or addElementSeparator
if you prefer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
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).