Improve duration formatting #647

jamesfer · 2020-10-06T01:21:50Z

Which problem is this PR solving?

Resolves #319

Short description of the changes

Improves the duration formatting in the UI so that larger durations are easier to read. I've listed some examples below comparing the old and new formatting:

Old	New	Comment
0ms	0μs	Smallest displayed unit used to be ms, now it is μs
1.5s	1.5s	Durations less than 1 minute are displayed in decimals
1.5m	1m 30s	Units are split instead of using decimal points
183840s	2d 3h 4m	No more then 3 units are displayed as going all the way to ms on a day long duration seemed overkill
173040	2d 4m	Units that are 0 are hidden

Each of these behaviours are open for discussion, I just tried to pick something that looked sensible.

In addition, this PR also removes the inputUnit parameter of the formatDuration function. I did this because the parameter would significantly complicate the implementation of the function and it isn't used anywhere I could find in the code base. The duration is always assumed to be given in microseconds which is consistent with the behaviour of other functions in the file. If a duration of a different unit is ever used, then it is probably best to create a function that will convert it to microseconds before passing it to the formatDuration function.

yurishkuro · 2020-10-06T04:23:36Z

Thanks for the PR.

I am not sold on multi-unit form being the best representation. E.g. it's much clearer to show 1.5s than 1s 500 ms, but that's the easy one (and also sub-second values are normal decimals that require no tricks, unlike h:m:s that use different multiples). I think two nearby units could be ok, like 4d 2h, but not 4d 2m (irrelevant precision). More than two units seems like overkill always.

jamesfer · 2020-10-06T04:40:41Z

They sound like good points. I'll change the number of units displayed to 2 so 2d 3h 52m would turn into 2d 4h (due to rounding) and 4d 0h 3m 20s would be displayed as just 4d.

I agree with you on using decimal points for seconds, milliseconds and microseconds as they are decimal based, but I think splitting the units is clearer on minutes, hours and days due to the odd numbers of units. Would the following displays be better?

1.5μs
1.5ms
1.5s
1m 30s
1h 30m
1d 12h

yurishkuro · 2020-10-06T05:36:42Z

all these examples look good to me.

jamesfer · 2020-10-06T22:40:06Z

@yurishkuro Just updated the PR to match those examples

yurishkuro · 2020-10-06T23:56:31Z

packages/jaeger-ui/src/utils/date.test.js

@@ -0,0 +1,71 @@
+// Copyright (c) 2017 Uber Technologies, Inc.


Suggested change

// Copyright (c) 2017 Uber Technologies, Inc.

// Copyright (c) 2020 The Jaeger Authors

yurishkuro · 2020-10-07T00:26:53Z

trying to fix code coverage run #649

codecov · 2020-10-07T00:48:13Z

Codecov Report

Merging #647 into master will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #647   +/-   ##
=======================================
  Coverage   93.74%   93.75%           
=======================================
  Files         227      227           
  Lines        5902     5906    +4     
  Branches     1483     1484    +1     
=======================================
+ Hits         5533     5537    +4     
  Misses        328      328           
  Partials       41       41

Impacted Files	Coverage Δ
packages/jaeger-ui/src/utils/date.tsx	`91.30% <100.00%> (+0.82%)`	⬆️

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 6827f70...2f0f24c. Read the comment docs.

yurishkuro · 2020-10-07T01:44:50Z

packages/jaeger-ui/src/utils/date.test.js

+  // it('rounds the number of microseconds', () => {
+  //   const input = 256 * ONE_MILLISECOND + 0.7;
+  //   expect(formatDuration(input)).toBe('256ms 1μs');
+  // });


these look like they should be deleted?

Expands the number of units that the formatDuration function displays. Previously the function only used milliseconds and seconds to display durations, now it will support all units up to days. This is to make it easier to read very long durations in the UI that would have previously only been displayed in seconds. As a side effect of this change, there are a few changes to the way existing durations are displayed: * 0ms -> 0μs * 0.15ms -> 150μs * 1.7s -> 1s 700ms Resolves jaegertracing#319 Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

The inputUnit parameter was not used anywhere and significantly complicated the implementation of the formatDuration function. Now, all inputs are assumed to be in microseconds which is consistent with a few other methods in the date.tsx file. Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

Reduces the number of units displayed by the dateFormat function to a maximum of 2. So "2d 5h 20m" will now be "2d 5h". Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

This change will display durations less than a minute using decimals as 1.3s is easier to comprehend than 1s 300ms as it was being displayed before. Durations larger than a minute will still use multiple units. Eg. 5m 27s Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

yurishkuro

Thanks!

* Add more units to duration formatting Expands the number of units that the formatDuration function displays. Previously the function only used milliseconds and seconds to display durations, now it will support all units up to days. This is to make it easier to read very long durations in the UI that would have previously only been displayed in seconds. As a side effect of this change, there are a few changes to the way existing durations are displayed: * 0ms -> 0μs * 1.5μs * 1.5ms * 1.5s * 1m 30s * 1h 30m * 1d 12h Resolves jaegertracing#319 Signed-off-by: James Ferguson <jamesferguson497@gmail.com> * Remove inputUnit parameter of formatDuration The inputUnit parameter was not used anywhere and significantly complicated the implementation of the formatDuration function. Now, all inputs are assumed to be in microseconds which is consistent with a few other methods in the date.tsx file. Signed-off-by: James Ferguson <jamesferguson497@gmail.com> * Reduce date format units to 2 Reduces the number of units displayed by the dateFormat function to a maximum of 2. So "2d 5h 20m" will now be "2d 5h". Signed-off-by: James Ferguson <jamesferguson497@gmail.com> * Use decimals when formating μs, ms and s durations This change will display durations less than a minute using decimals as 1.3s is easier to comprehend than 1s 300ms as it was being displayed before. Durations larger than a minute will still use multiple units. Eg. 5m 27s Signed-off-by: James Ferguson <jamesferguson497@gmail.com> * Fix date.test.js licence Signed-off-by: James Ferguson <jamesferguson497@gmail.com> Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

jamesfer requested review from everett980, rubenvp8510 and tiffon as code owners October 6, 2020 01:21

jamesfer force-pushed the improve-duration-formatting branch from 782ccf3 to 90b6cc7 Compare October 6, 2020 01:24

yurishkuro reviewed Oct 6, 2020

View reviewed changes

yurishkuro reviewed Oct 7, 2020

View reviewed changes

jamesfer force-pushed the improve-duration-formatting branch from f3dbf2b to b015843 Compare October 7, 2020 02:45

jamesfer added 5 commits October 7, 2020 13:48

Reduce date format units to 2

17b525e

Reduces the number of units displayed by the dateFormat function to a maximum of 2. So "2d 5h 20m" will now be "2d 5h". Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

Fix date.test.js licence

2f0f24c

Signed-off-by: James Ferguson <jamesferguson497@gmail.com>

jamesfer force-pushed the improve-duration-formatting branch from b015843 to 2f0f24c Compare October 7, 2020 02:48

yurishkuro approved these changes Oct 7, 2020

View reviewed changes

yurishkuro merged commit 6afb01e into jaegertracing:master Oct 7, 2020

jamesfer deleted the improve-duration-formatting branch October 7, 2020 03:04

This was referenced Jul 30, 2021

Sync relevant jaeger-ui PRs grafana/grafana#37404

Closed

Jaeger: Sync bug fixes and quality of life improvements from upstream grafana/grafana#37420

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve duration formatting #647

Improve duration formatting #647

jamesfer commented Oct 6, 2020 •

edited

Loading

yurishkuro commented Oct 6, 2020

jamesfer commented Oct 6, 2020

yurishkuro commented Oct 6, 2020

jamesfer commented Oct 6, 2020

yurishkuro Oct 6, 2020

jamesfer Oct 7, 2020

yurishkuro commented Oct 7, 2020

codecov bot commented Oct 7, 2020 •

edited

Loading

yurishkuro Oct 7, 2020

yurishkuro left a comment

		@@ -0,0 +1,71 @@
		// Copyright (c) 2017 Uber Technologies, Inc.

	// Copyright (c) 2017 Uber Technologies, Inc.
	// Copyright (c) 2020 The Jaeger Authors

Improve duration formatting #647

Improve duration formatting #647

Conversation

jamesfer commented Oct 6, 2020 • edited Loading

Which problem is this PR solving?

Short description of the changes

yurishkuro commented Oct 6, 2020

jamesfer commented Oct 6, 2020

yurishkuro commented Oct 6, 2020

jamesfer commented Oct 6, 2020

yurishkuro Oct 6, 2020

Choose a reason for hiding this comment

jamesfer Oct 7, 2020

Choose a reason for hiding this comment

yurishkuro commented Oct 7, 2020

codecov bot commented Oct 7, 2020 • edited Loading

Codecov Report

yurishkuro Oct 7, 2020

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

jamesfer commented Oct 6, 2020 •

edited

Loading

codecov bot commented Oct 7, 2020 •

edited

Loading