add millisecond duration encoder

[caibirdme]

Since network requests are always in milliseconds level, if procTime shows in seconds, it may look like 0.003, to many zeros. While if the procTime shows in nanoseconds, it may look like 12312312323, too big to identify. So it'd be better to support milliseconds encoder, something like 30.5, pretty intuisive.

[claassistantio]

All committers have signed the CLA.

[codecov]

Codecov Report

Merging #773 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #773      +/-   ##
==========================================
+ Coverage   98.19%   98.19%   +<.01%     
==========================================
  Files          42       42              
  Lines        2271     2275       +4     
==========================================
+ Hits         2230     2234       +4     
  Misses         33       33              
  Partials        8        8

Impacted Files	Coverage Δ
zapcore/encoder.go	85.26% <100%> (+0.64%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 66bdb5b...2b89bdd. Read the comment docs.

[prashantv]

Thanks for the contribution @caibirdme

This change looks good to me, just needs some tests. Can you please add tests for this change?

[caibirdme]

Thanks for the contribution @caibirdme

This change looks good to me, just needs some tests. Can you please add tests for this change?

Ok, no problem! I'll add those testcases soon

[caibirdme]

@prashantv I'v added the test case

[prashantv]

Thanks for adding tests

One thing I noticed while looking at this again is that we may be inconsistent in whether we're encoding a float or an integer.

For example:

• SecondsDurationEncoder uses a floating-point number of seconds
• EpochMilliEncoder uses a floating-point number of seconds since the epoch for times

Given that, I think it makes more sense for MillisDurationEncoder to also encode a floating point value.

If we want to encode an integer value, we might want to add a suffix like SecondsIntDurationEncoder and MillisIntDurationEncoder.

Open to other ideas too, but want to make sure we're consistent.

[caibirdme]

@prashantv Well, I understand that, but if we just use float64, something like 31.0000005 may not that much good. What about defining a constructor to control the precision?

func NewMillisDurationEncoder(precision int) func MilliSecondsDurationEncoder(d time.Duration, enc PrimitiveArrayEncoder)

[prashantv]

What about defining a constructor to control the precision?

Constructor will help if you manually create the encoder, but it won't be as easy to use when unmarshalling a configuration from a YAML file.

For your use-case, do you want to control the precision, or would you prefer to use int vs float?

[caibirdme]

What about defining a constructor to control the precision?

Constructor will help if you manually create the encoder, but it won't be as easy to use when unmarshalling a configuration from a YAML file.

For your use-case, do you want to control the precision, or would you prefer to use int vs float?

Actually, I don't care about the precision, what I want is just how many milliseconds the operation costs. 30 or 30.05 are both ok for me, but 30.00005 smells bad. But as the test case shows, all the other encoder could show how nanoseconds it costs(1.0000005s 1.0000005 1000000500)，I don't know if something like 1.02 will bring some inconsistencies. But really, I think float with just two decimal places is better. When people choose ms encoder, nobody cares about the nanoseconds

[prashantv]

I discussed this offline with @abhinav and we decided that we should keep it simple: In general, if you ask for X, the granularity you probably care about is X.

The millis encoder should use int, so you can keep this change as-is.

Can you please rename the method to for consistency, otherwise, this change looks good.

[prashantv]

Suggested change

	enc.AppendInt64(d.Nanoseconds() / 1e6)
	enc.AppendInt64(d.Milliseconds())

[caibirdme]

the d.Milliseconds() is a new API only for go1.13+

[caibirdme]

golang/go#28564

[prashantv]

Good point, we can migrate to it once we drop support for 1.12. This is good for now, thanks.

[prashantv]

The CLA bot is blocked on user liudeen -- if that's an old username you no longer use, can you squash your commits into one owned by the new user

[prashantv]

Thanks for the contribution @caibirdme!