Add options for JSON encoders #105
Conversation
Add options for the JSON encoder, and supply options to control the message, timestamp, and level formatting. Users can also supply their own formatters if they have unusual requirements.
Allow users to pass options in the JSON encoder constructor, and validate that those options are honored. To minimize the test changes required, introduce a performance regression in `WriteEntry`. When we flatten the serialized message (in the next PR), we'll remove the extra truncate-and-copy step.
akshayjshah
force-pushed
the
ajs-encoders
branch
from
936772d
to
38839af
Compare
| @@ -30,5 +33,5 @@ type encoder interface { | |||
| AddFields([]Field) | |||
| Clone() encoder | |||
| Free() | |||
| WriteEntry(io.Writer, *Entry) error | |||
| WriteEntry(io.Writer, string, Level, time.Time) error | |||
There was a problem hiding this comment.
This undoes a change made in the previous PR - after some more thought, I think it's better to keep encoders and entries separate. This removes the concern about type mismatches between the receiver of WriteEntry and Entry.encoder.
I do still think that the WriteEntry name is appropriate, though, since we're writing out a single log line.
| import "time" | ||
|
|
||
| // JSONOption is used to set options for a JSON encoder. | ||
| type JSONOption interface { |
There was a problem hiding this comment.
Does this need to be exported?
There was a problem hiding this comment.
I'd also unexport the Apply method so that the custom formatter types won't display Apply in the documentation
There was a problem hiding this comment.
Unexporting Apply definitely makes sense...not sure why I exported it to begin with. We should keep JSONOption exported, though, since it'll be part of the public NewJSONEncoder signature.
|
Completed the first pass, the formatters look good. I'd like to reduce the exported API a fair bit more though (unexporting |
Unexport some options-related functionality, remove some stubs, and clean up multiple frees.
|
lgtm |
Benchmarks
Pre: Post: |
Let users specify how they'd like the message, level, and timestamp formatted for each log entry.
In order to keep the proposed change small, this PR introduces a small performance regression in
jsonEncoder.WriteEntry- it copies more bytes than necessary. In the next PR, I'm going to flatten the message structure, which will remove this extra copy and require many changes to the test code.(I'm holding off on marking the related issues resolved until we export the
newJSONEncoderfunction.)(edit by Prashant: this brings internal support for #33)