Add NumbersAsStrings to EncoderConfig

[fishy]

This partially fixes https://github.com/uber-go/zap/issues/884.

This implementation is limited to top level fields, like:

Int64("int64", 123)
Float64("float64", 456)

Or sugared:

With("int64", 123, "float64", float64(456), "slice", []int{1, 2, 3})

It does NOT work with reflected fields like:

Reflect("slice", []int{1, 2, 3})
With("struct", struct{ Int64 int64 }{Int64: 123})

As for reflected fields we use go's stdlib json encoder directly, which doesn't provide such feature.

[CLAassistant]

All committers have signed the CLA.

[codecov[bot]]

Codecov Report

Merging #885 (f6eb098) into master (5b4722d) will increase coverage by 0.01%. The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
+ Coverage   98.21%   98.23%   +0.01%     
==========================================
  Files          43       43              
  Lines        1910     1922      +12     
==========================================
+ Hits         1876     1888      +12     
  Misses         27       27              
  Partials        7        7              

Impacted Files	Coverage Δ
zapcore/encoder.go	87.09% <ø> (ø)
zapcore/json_encoder.go	100.00% <100.00%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[abhinav]

Thanks for the PR, @fishy. Although this is probably solvable externally with a custom wrapper around the JSON encoder, I agree that this is something Zap should ship a solution for.

However, I'm not sure that this is the API surface we'd want to expose for this. Maybe we should provide a hook to control how numbers are encoded? Or if we don't foresee the need to encode numbers differently besides these two representations, we probably want it to take the form of an enum.

What do you think, @prashantv?

[prashantv]

Thanks for the PR, @fishy. Although this is probably solvable externally with a custom wrapper around the JSON encoder, I agree that this is something Zap should ship a solution for.

However, I'm not sure that this is the API surface we'd want to expose for this. Maybe we should provide a hook to control how numbers are encoded? Or if we don't foresee the need to encode numbers differently besides these two representations, we probably want it to take the form of an enum.

What do you think, @prashantv?

Agree, I'm not sure about the API surface. I also don't like that it only applies in some scenarios such top-level fields, but not for reflected fields (or other nested fields).

If this is a general problem for JSON encoding, I'd expect to see an option in the Go standard library in the JSON encoder, and then consistently enable it for all encodings (top-level, nested, or reflected).

[fishy]

So in JavaScript spec, there's only one type of number defined, which is 64-bit floats: https://www.w3schools.com/js/js_numbers.asp. JSON inherits that from JavaScript.

In reality, decoder/unmarshaler implementations can (and often choose to) extend that to higher precision. For example, in Go encoding/json.Decoder.UseNumbers can be used to keep int64 precision when possible (here is an example).

But that extension is not part of the json specification so it's not always guaranteed (for example, the log ingestion vendor we use doesn't do that, and we can't blame them for that because it's not part of the spec).

The reason Go's json library provided this extension as an option is probably why they don't provide an option on the encoder side to encode numbers as strings.

I certainly agree that it would be great to be able to do it for reflected fields, but that's something a lot more complex (e.g. we probably need to either push go to provide that option, which is unlikely to happen, or switch to a different json encoding library for reflected fields that provides this feature), and this partial fix will cover the majority of the cases, so I don't think this partial fix should be blocked by the complete fix.

[prashantv]

Sorry for the delay in getting back to you @fishy.

I'm still not sure zap should provide this specific option of encoding numbers as strings, but perhaps a more general extension mechanism could be used to implement this feature.

This use-case could be solved with an encoder-decorator, that can wrap the AddInt* and similar methods to encode them using any format (e.g., using AddString). This is technically possible today if you create an encoder, wrap it, then create your own core/logger, but it's a little bit painful.

We could provide options for wrapping encoders as part of construction that could simplify the above, what do you think of this approach?

[fishy]

@prashantv thanks. we are currently working around it by wrapping the core: https://github.com/reddit/baseplate.go/blob/master/log/core_wrapper.go

if there's a new API to only wrap the encoder I think that would be better than wrapping the whole core.

[fishy]

@prashantv is there an issue/PR I can follow for the progress of the new encoder-decorator api?