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

add millisecond duration encoder #773

Merged
merged 1 commit into from Jan 9, 2020

Conversation

caibirdme
Copy link
Contributor

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

claassistantio commented Dec 14, 2019

CLA assistant check
All committers have signed the CLA.

@codecov
Copy link

codecov bot commented Dec 14, 2019

Codecov Report

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

Impacted file tree graph

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

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @caibirdme

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

@caibirdme
Copy link
Contributor Author

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

@prashantv I'v added the test case

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

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.

zapcore/encoder.go Outdated Show resolved Hide resolved
@caibirdme
Copy link
Contributor Author

@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
Copy link
Collaborator

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

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

Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

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.

zapcore/encoder.go Outdated Show resolved Hide resolved
// MillisDurationEncoder serializes a time.Duration to an integer number of
// milliseconds elapsed.
func MillisDurationEncoder(d time.Duration, enc PrimitiveArrayEncoder) {
enc.AppendInt64(d.Nanoseconds() / 1e6)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

@prashantv
Copy link
Collaborator

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

make ms encoder compatible for go version <=1.13.0 & add testcase for ms encoder

change name of ms encoder
Copy link
Collaborator

@prashantv prashantv left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution @caibirdme!

@prashantv prashantv merged commit 73a5f64 into uber-go:master Jan 9, 2020
cgxxv pushed a commit to cgxxv/zap that referenced this pull request Mar 25, 2022
make ms encoder compatible for go version <=1.13.0 & add testcase for ms encoder
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.

None yet

3 participants