Skip to content
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

Merged
merged 3 commits into from
Feb 2, 2017
Merged

Conversation

akshayjshah
Copy link
Contributor

@akshayjshah akshayjshah commented Feb 2, 2017

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:

  1. It highlights our need for a user-facing defaults + functional options paradigm (Re-introduce terse logger instantiation #221). Once we have one of those, most of the long struct declarations introduced here will go away.
  2. In zapcore, I didn't attempt to guard against nil formatters; I'd rather let the user-facing zap package define safe defaults (also part of Re-introduce terse logger instantiation #221).

After pre-factoring, the second commit is quite small. It makes durations and times first-class types in encoders (including arrays of each).

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.
@mention-bot
Copy link

@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.

@akshayjshah akshayjshah requested a review from jcorbin February 2, 2017 02:43
@akshayjshah akshayjshah added this to the 1.0.0 milestone Feb 2, 2017
@akshayjshah akshayjshah changed the base branch from ajs-encoder-config to dev February 2, 2017 02:44
Copy link
Contributor

@jcorbin jcorbin left a 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.

TimeKey: "ts",
CallerKey: "caller",
StacktraceKey: "stacktrace",
TimeFormatter: func(t time.Time, enc zapcore.ArrayEncoder) { enc.AppendInt64(t.UnixNano() / int64(time.Millisecond)) },
Copy link
Contributor

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.

Copy link
Contributor Author

@akshayjshah akshayjshah Feb 2, 2017

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.

// 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)
Copy link
Contributor

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).

Copy link
Contributor Author

@akshayjshah akshayjshah Feb 2, 2017

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 {
Copy link
Contributor

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.

Copy link
Contributor Author

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.

if ent.Caller.Defined {
if final.LevelKey != "" {
final.addKey(final.LevelKey)
final.LevelFormatter(ent.Level, final)
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

// All the formatters may have been no-ops.
final.bytes = append(final.bytes, ',')
}
final.separateElements()
Copy link
Contributor

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)

Copy link
Contributor Author

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.
@akshayjshah akshayjshah merged commit bf2ad4e into dev Feb 2, 2017
@akshayjshah akshayjshah deleted the ajs-time branch February 2, 2017 22:09
akshayjshah added a commit that referenced this pull request Feb 15, 2017
* 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants