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 millis to format_duration #259

Merged
merged 4 commits into from
Jun 24, 2019
Merged

Conversation

artem-zinnatullin
Copy link
Contributor

@artem-zinnatullin artem-zinnatullin commented May 1, 2019

Closes #258.

Shows precision of the unit previous to the major time unit.

Like this:

  • Over a second — show millis
  • Over a minute — show seconds
  • Over an hour — show minutes

Includes Rust update to use as_millis, release notes: https://github.com/rust-lang/rust/blob/master/RELEASES.md#version-1341-2019-04-25


Example of output (combined with #256):

-------- TEST STARTED (pull mode = 'serial') test_local_ignore.sh --------
:: Mainframer v3.0.0-dev

Pushing...
Push done: took 0.193 seconds.

Executing command on remote machine...

echo noop

noop

Execution done: took 0.137 seconds.
Pulling...
Pull done: took 0.177 seconds

Success: took 0.537 seconds.

-------- TEST ENDED (pull mode = 'serial') test_local_ignore.sh --------

Copy link
Contributor

@arturdryomov arturdryomov left a comment

Choose a reason for hiding this comment

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

0.0 seconds, 1.0 seconds and friends look weird. So does 0.500 seconds. Can we have 1.2 seconds and 200 milliseconds at the same time?

@JayNewstrom
Copy link

Is a build in milliseconds realistic? The builds I do will always take well over 1 second.

@artem-zinnatullin
Copy link
Contributor Author

Subsecond builds are not possible most of the time, however knowing where latency comes from is important. The better (unless annoying) we show it the more users would care about optimizing their configurations.

I for one defintely pay attention to duration output of my build tooling and try to optimize remaining tail latency

@artem-zinnatullin
Copy link
Contributor Author

0.0 seconds, 1.0 seconds and friends look weird. So does 0.500 seconds. Can we have 1.2 seconds and 200 milliseconds at the same time?

I can agree to always showing 3 digits, would that be fine with you? Like this:

0.863
1.200
1.358
1.000

@arturdryomov
Copy link
Contributor

I can agree to always showing 3 digits, would that be fine with you? Like this:

Nope, sorry. 1.000 is unnecessary verbose. We format it for humans, not for machines or statistic tools on top.

@artem-zinnatullin
Copy link
Contributor Author

You'll never see exactly 1.000, you know that.

1 digit after . is not enough for me, 2 is weird thus why 3 (milliseconds, 1/1000 of a second).

@arturdryomov
Copy link
Contributor

Yes, .000 is the corner case for sure, but the question is a bit different. Do we even care about such precision? Do users care about it? It’s one thing to see took 47 seconds and another took 47.254 seconds. I thought the idea was to show millis when we show 0 seconds, this is awkward.

@artem-zinnatullin
Copy link
Contributor Author

Maybe I didn't describe idea well enough, sorry about that.

Idea is that in relatively short timings (< 60 seconds) milliseconds matter, of course it doesn't matter when we're talking minutes.

I guess a rule of thumb could be to show previous precision of the major time unit.

Like this:

  • Over a second — show millis
  • Over a minute — show seconds
  • Over an hour — show minutes

@arturdryomov
Copy link
Contributor

I can get behind these rules. Is it covered by the current change or you’ll need adjustments?

@artem-zinnatullin
Copy link
Contributor Author

To clarify, this PR doesn't follow that rule of thumb ^, I just came up with it to have better description of what was the motivation behind the change

@artem-zinnatullin
Copy link
Contributor Author

---- RACE CONDITION DETECTED ----

ok, now that we have agreement I'll adjust PR

@artem-zinnatullin
Copy link
Contributor Author

Done! Also, had to update Rust because as_millis wasn't stable lol

#[stable(feature = "duration_as_u128", since = "1.33.0")]
#[inline]
pub const fn as_millis(&self) -> u128 {

@artem-zinnatullin
Copy link
Contributor Author

PR description updated accordingly.

@artem-zinnatullin
Copy link
Contributor Author

@ming13 can you ptal?

@artem-zinnatullin
Copy link
Contributor Author

cc @sboishtyan

@artem-zinnatullin
Copy link
Contributor Author

and @Tagakov

@artem-zinnatullin artem-zinnatullin merged commit 1f7c49b into 3.x Jun 24, 2019
@artem-zinnatullin artem-zinnatullin deleted the az/format-duration-millis branch June 24, 2019 22:34
@arturdryomov
Copy link
Contributor

@artem-zinnatullin, can we use a magical Rust package for this? I hope Rust is not that bad that there is no time formatting tool around.

@artem-zinnatullin
Copy link
Contributor Author

We prob can, but really the code is so tiny that it's not a big deal I think

@artem-zinnatullin
Copy link
Contributor Author

But if someone finds such a package, I'm def down to take a look at it

@arturdryomov
Copy link
Contributor

@artem-zinnatullin, chrono_humanize probably should work. Wasn’t updated for year though.

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

Successfully merging this pull request may close these issues.

Include milliseconds in duration output
4 participants