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 time method taking Duration in Timed trait? #48

Closed
robinst opened this issue Jan 4, 2017 · 5 comments
Closed

Add time method taking Duration in Timed trait? #48

robinst opened this issue Jan 4, 2017 · 5 comments
Assignees
Milestone

Comments

@robinst
Copy link
Contributor

robinst commented Jan 4, 2017

Using time to record a timing, I've found myself having to write this to convert a Duration to milliseconds like this:

let start = Instant::now();
...
let duration = start.elapsed();
let ms = (duration.as_secs() * 1_000) + (duration.subsec_nanos() / 1_000_000) as u64;

Note that this can overflow. I decided to not care about this because this is about metrics (and if your timing overflows, your app has a bigger problem than incorrect metrics).

I wonder if you've considered adding an additional method to Timed that takes a Duration instead of a u64 and handles the conversion? It could return Err in case of overflow.

@56quarters
Copy link
Owner

I hadn't thought of it but that's a great idea. Any ideas for the potential new method name in Timed?

@robinst
Copy link
Contributor Author

robinst commented Jan 4, 2017

If you're ok with a breaking change, you could consider renaming the existing one to time_millis to make it really explicit what it is taking.

If not, the new one could be named time_duration.

@56quarters
Copy link
Owner

I'd rather not make people change working code. I think time_duration sounds good, thanks!

@56quarters 56quarters modified the milestones: 0.11.0, 0.12.0 Jan 4, 2017
@56quarters 56quarters self-assigned this Jan 25, 2017
56quarters added a commit that referenced this issue Jan 27, 2017
Add a new method for recording timings using std::time::Duration
instead of a u64 of milliseconds. The duration is converted to the
number of milliseconds before being send to the sink. Any integer
overflow will cause an error to be returned.

Fixes #48
56quarters added a commit that referenced this issue Jan 27, 2017
Add a new method for recording timings using std::time::Duration
instead of a u64 of milliseconds. The duration is converted to the
number of milliseconds before being send to the sink. Any integer
overflow will cause an error to be returned.

Fixes #48
56quarters added a commit that referenced this issue Jan 27, 2017
Add a new method for recording timings using std::time::Duration
instead of a u64 of milliseconds. The duration is converted to the
number of milliseconds before being sent to the sink. Any integer
overflow will cause an error to be returned.

Fixes #48
@56quarters
Copy link
Owner

Opened #55 to address this. Let me know what you think!

@robinst
Copy link
Contributor Author

robinst commented Jan 29, 2017

Thanks for notifying, went through the change and it looks good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants