Flatten serialized entries #107
Conversation
| require.True(t, cm.OK(), "Expected CheckedMessage to be OK at enabled levels.") | ||
| cm.Write(Int("magic", 42)) | ||
| assert.Equal( | ||
| t, | ||
| `{"msg":"Info.","level":"info","ts":0,"fields":{"magic":42}}`, | ||
| output()[0], | ||
| `{"magic":42,"msg":"Info.","level":"info"}`, |
There was a problem hiding this comment.
While the JSON output isn't intended for users, it will often be consumed by humans. The current ordering of keys isn't very human-friendly, I'd prefer: time, level, msg, fields. (specifically, the time, level and message should be before the fields).
Since we're copying the bytes in clone anyway, I don't think it'll be any less efficient to create a new buffer, append the time/level/message fields before copying over the fields buffer.
akshayjshah
force-pushed
the
ajs-encoders
branch
2 times, most recently
from
771711d to
f52eddd
Compare
|
Updated. |
| } | ||
| final.bytes = append(final.bytes, enc.bytes...) | ||
| } | ||
| final.bytes = append(final.bytes, "}\n"...) |
There was a problem hiding this comment.
Let's change this to:
final.bytes = append(final.bytes, '}', '\n')It looks like this is slightly faster,
https://github.com/prashantv/go-bench/blob/master/append_bytes_test.go
BenchmarkAppendString-8 500000000 3.72 ns/op 0 B/op 0 allocs/op
BenchmarkAppendBytes-8 2000000000 1.63 ns/op 0 B/op 0 allocs/op
There was a problem hiding this comment.
Every nanosecond counts!
There was a problem hiding this comment.
🎶 Eeeeevery nanosecond is precious... 🎶
|
lgtm with minor comments |
Log aggregation and processing systems typically require some information in the top-level JSON. In zap, we included the message, level, and timestamp, but that's clearly not enough to support many use cases. We could support adding information to this envelope via a separate API, but the simplest option is to flatten the entry. This is a small change to the production code, but it requires large changes to the tests. To keep review semi-sane, this commit includes only the production code changes.
Amend all the tests to expect flattened logs.
akshayjshah
force-pushed
the
ajs-encoders
branch
from
e0442b3 to
0dddb39
Compare
Log aggregation and processing systems typically require some metadata in the top-level JSON. In zap, we included the message, level, and timestamp, but that's clearly not enough to support many use cases. We could support adding information to this envelope via a separate API, but the simplest option is to flatten the entry.
This is a small change to the production code, but it requires large changes to the tests. To keep review semi-sane, the production and test changes are in separate commits.