Monitor Tab: Cannot see the whole numbers in the legend #873

nofar9792 · 2022-01-26T13:55:13Z

Signed-off-by: nofar9792 [email protected]

Which problem is this PR solving?

Resolves #871

Short description of the changes

Convert the duration to a suitable time unit

Before Change:

After Change:

In order to find the suitable time unit, I find the max value, and then I use this time unit and convert the y-axis ticks.
Furthermore, I did a small refactor to timeConversion - use moment when I can and do all the calculation by myself

added brackets instead of comma:

Signed-off-by: nofar9792 <[email protected]>

…ger-ui into fix-legend-numbers

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx

Signed-off-by: nofar9792 <[email protected]>

…o fix-legend-numbers

Signed-off-by: nofar9792 <[email protected]>

albertteoh · 2022-02-20T05:38:19Z

@nofar9792 can you please share a screenshot of what the graph and y-axis labels look like after these changes?

codecov · 2022-02-20T05:48:38Z

Codecov Report

Merging #873 (c283c1b) into main (161a661) will increase coverage by 0.01%.
The diff coverage is 94.87%.

❗ Current head c283c1b differs from pull request most recent head 17e560b. Consider uploading reports for the commit 17e560b to get more accurate results

@@            Coverage Diff             @@
##             main     #873      +/-   ##
==========================================
+ Coverage   95.27%   95.29%   +0.01%     
==========================================
  Files         240      240              
  Lines        7475     7495      +20     
  Branches     1868     1870       +2     
==========================================
+ Hits         7122     7142      +20     
  Misses        347      347              
  Partials        6        6

Impacted Files	Coverage Δ
...c/components/Monitor/ServicesView/serviceGraph.tsx	`98.07% <ø> (ø)`
...r-ui/src/components/Monitor/ServicesView/index.tsx	`97.02% <86.66%> (-1.85%)`	⬇️
packages/jaeger-ui/src/reducers/metrics.mock.js	`100.00% <100.00%> (ø)`
packages/jaeger-ui/src/reducers/metrics.tsx	`100.00% <100.00%> (ø)`
packages/jaeger-ui/src/utils/date.tsx	`94.66% <100.00%> (+0.30%)`	⬆️
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx	`100.00% <0.00%> (+5.55%)`	⬆️

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 161a661...17e560b. Read the comment docs.

…o fix-legend-numbers

Signed-off-by: nofar9792 <[email protected]>

nofar9792 · 2022-02-20T10:55:04Z

@nofar9792 can you please share a screenshot of what the graph and y-axis labels look like after these changes?

Thanks Albert, I added 🙏

nofar9792 · 2022-02-20T15:27:00Z

@AlonMiz

nofar9792 · 2022-02-22T12:01:28Z

@yurishkuro ?

yurishkuro · 2022-02-22T23:57:49Z

Are the before/after screenshots still accurate? I was expecting to see axis ticks like 1s, 2s, not to have Sec added to the axis title.

nofar9792 · 2022-02-23T08:00:06Z

Are the before/after screenshots still accurate? I was expecting to see axis ticks like 1s, 2s, not to have Sec added to the axis title.

We think it's redundant to write it in the ticks
@afishler

albertteoh · 2022-02-23T10:57:31Z

We think it's redundant to write it in the ticks

What about when latencies are 1ms or 1hr?

afishler-zz · 2022-02-23T15:58:30Z

We think it's redundant to write it in the ticks

What about when latencies are 1ms or 1hr?

Well, they cannot be different units on the same diagram. Once the units are set they will be the same for the whole diagram.

That's why putting the units in the title should be enough. So if the function sets the units to ms it will be all ms, if it is seconds it will be all seconds

albertteoh · 2022-02-23T22:22:37Z

That's why putting the units in the title should be enough. So if the function sets the units to ms it will be all ms, if it is seconds it will be all seconds

Ah, I see, so the title's units are dynamic based on the latency data. If that's the case, it makes sense to me.

Signed-off-by: nofar9792 <[email protected]>

nofar9792 · 2022-02-24T13:01:10Z

That's why putting the units in the title should be enough. So if the function sets the units to ms it will be all ms, if it is seconds it will be all seconds

Ah, I see, so the title's units are dynamic based on the latency data. If that's the case, it makes sense to me.

@yurishkuro @albertteoh so WDYT?

Signed-off-by: nofar9792 <[email protected]>

albertteoh · 2022-02-25T10:55:26Z

packages/jaeger-ui/src/utils/date.tsx

+  milliseconds: 'ms',
+  seconds: 'Sec',
+  minutes: 'Min',
+  hours: 'Hrs',
+  days: 'Days',


I feel these should be consistent in case and plurality. Maybe, ms, sec, min, hr, day.

It just occurred to me that min could unfortunately be confused with "minimum", so it's worth considering the plural form: ms, secs, mins, hrs, days.

https://physics.nist.gov/cuu/Units/outside.html

albertteoh · 2022-02-25T10:56:34Z

packages/jaeger-ui/src/utils/date.tsx

+
+  if (shortTimeUnit) return shortTimeUnit;
+
+  return '';


Under what conditions would timeUnit not be in timeUnitToShortTermMapper?

if i don't haveshortTimeUnit, I don't want to return undefined

albertteoh · 2022-02-25T10:58:23Z

packages/jaeger-ui/src/utils/date.tsx

@@ -40,6 +40,14 @@ const UNIT_STEPS: { unit: string; microseconds: number; ofPrevious: number }[] =
  { unit: 'μs', microseconds: 1, ofPrevious: 1000 },
 ];

+const timeUnitToShortTermMapper = {
+  milliseconds: 'ms',


Why isn't microseconds supported?

moment.js doesn't support it and in my cases (the only usages), we don't need microseconds

albertteoh · 2022-02-25T11:02:09Z

packages/jaeger-ui/src/components/Monitor/ServicesView/index.tsx

@@ -249,12 +270,13 @@ export class MonitorATMServicesViewImpl extends React.PureComponent<TProps, Stat
                metrics.serviceError.service_latencies_95
              }
              loading={metrics.loading}
-              name="Latency, ms"
+              name={`Latency, ${convertTimeUnitToShortTerm(displayTimeUnit)}`}


I prefer putting the units in brackets like "Latency (ms)" or "Latency (min)", instead of a comma.

What do you/others think?

Signed-off-by: nofar9792 <[email protected]>

albertteoh

LGTM overall, thanks! It looks like there are some code coverage issues that need addressing.

albertteoh · 2022-02-28T12:20:33Z

Just noticed, the description's "After" screenshot shows "Latency (s)" and the scale on the left goes up to a max 390.

I would have expected the units to be minutes already?

nofar9792 · 2022-02-28T12:30:30Z

Just noticed, the description's "After" screenshot shows "Latency (s)" and the scale on the left goes up to a max 390.

I would have expected the units to be minutes already?

This data is different

Signed-off-by: nofar9792 <[email protected]>

…ger-ui into fix-legend-numbers

Signed-off-by: nofar9792 <[email protected]>

nofar9792 · 2022-03-01T16:16:39Z

@albertteoh @yurishkuro What prevents us from merging it

yurishkuro · 2022-03-01T17:55:00Z

@albertteoh can you finish the review for this?

nofar9792 added 3 commits January 26, 2022 11:06

Add left margin

2d9249a

Signed-off-by: nofar9792 <[email protected]>

Add left margin

018279f

Signed-off-by: nofar9792 <[email protected]>

Merge branch 'fix-legend-numbers' of https://github.com/nofar9792/jae…

4b0952f

…ger-ui into fix-legend-numbers

nofar9792 requested review from everett980, rubenvp8510 and tiffon as code owners January 26, 2022 13:55

nofar9792 marked this pull request as draft January 26, 2022 13:55

yurishkuro reviewed Jan 26, 2022

View reviewed changes

packages/jaeger-ui/src/components/Monitor/ServicesView/serviceGraph.tsx Outdated Show resolved Hide resolved

nofar9792 marked this pull request as ready for review January 26, 2022 16:57

nofar9792 added 5 commits February 8, 2022 14:53

Temp

d217082

Signed-off-by: nofar9792 <[email protected]>

Use timeConversion

b190999

Signed-off-by: nofar9792 <[email protected]>

Merge branch 'main' of https://github.com/jaegertracing/jaeger-ui int…

2ae8fd3

…o fix-legend-numbers

Refactor timeConvention and use max vaule for timeunit in the title

df82afd

Signed-off-by: nofar9792 <[email protected]>

Semicolon

37365de

Signed-off-by: nofar9792 <[email protected]>

nofar9792 added 2 commits February 20, 2022 11:05

Merge branch 'main' of https://github.com/jaegertracing/jaeger-ui int…

2057d8f

…o fix-legend-numbers

Remove timeunit from y axis

69c6777

Signed-off-by: nofar9792 <[email protected]>

Merge

caaa913

Signed-off-by: nofar9792 <[email protected]>

Non existent timeunit

812145d

Signed-off-by: nofar9792 <[email protected]>

nofar9792 changed the title ~~Fix Bug: Cannot see the whole numbers in the legend~~ Monitor Tab: Cannot see the whole numbers in the legend Feb 24, 2022

albertteoh reviewed Feb 25, 2022

View reviewed changes

nofar9792 added 2 commits February 27, 2022 17:33

Same short term

86df0d8

Signed-off-by: nofar9792 <[email protected]>

Brackets

8fc6c44

Signed-off-by: nofar9792 <[email protected]>

albertteoh previously approved these changes Feb 28, 2022

View reviewed changes

Merge branch 'main' into fix-legend-numbers

dd134d8

nofar9792 added 2 commits February 28, 2022 15:54

Some tests

6b34e47

Signed-off-by: nofar9792 <[email protected]>

Merge branch 'fix-legend-numbers' of https://github.com/nofar9792/jae…

654ad97

…ger-ui into fix-legend-numbers

nofar9792 dismissed albertteoh’s stale review via 654ad97 February 28, 2022 13:56

nofar9792 requested review from albertteoh and yurishkuro February 28, 2022 13:59

nofar9792 added 2 commits February 28, 2022 22:01

Merge

81f26e0

Signed-off-by: nofar9792 <[email protected]>

Merge branch 'main' into fix-legend-numbers

17e560b

albertteoh approved these changes Mar 1, 2022

View reviewed changes

albertteoh merged commit f605ad6 into jaegertracing:main Mar 1, 2022

nofar9792 deleted the fix-legend-numbers branch March 10, 2022 22:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Monitor Tab: Cannot see the whole numbers in the legend #873

Monitor Tab: Cannot see the whole numbers in the legend #873

nofar9792 commented Jan 26, 2022 •

edited

Loading

albertteoh commented Feb 20, 2022

codecov bot commented Feb 20, 2022 •

edited

Loading

nofar9792 commented Feb 20, 2022

nofar9792 commented Feb 20, 2022

nofar9792 commented Feb 22, 2022

yurishkuro commented Feb 22, 2022

nofar9792 commented Feb 23, 2022

albertteoh commented Feb 23, 2022

afishler-zz commented Feb 23, 2022

albertteoh commented Feb 23, 2022

nofar9792 commented Feb 24, 2022

albertteoh Feb 25, 2022

albertteoh Feb 25, 2022

yurishkuro Feb 25, 2022

albertteoh Feb 25, 2022

nofar9792 Feb 27, 2022

albertteoh Feb 25, 2022

nofar9792 Feb 27, 2022

albertteoh Feb 25, 2022

nofar9792 Feb 27, 2022

albertteoh left a comment

albertteoh commented Feb 28, 2022

nofar9792 commented Feb 28, 2022

nofar9792 commented Mar 1, 2022

yurishkuro commented Mar 1, 2022

Monitor Tab: Cannot see the whole numbers in the legend #873

Monitor Tab: Cannot see the whole numbers in the legend #873

Conversation

nofar9792 commented Jan 26, 2022 • edited Loading

Which problem is this PR solving?

Short description of the changes

albertteoh commented Feb 20, 2022

codecov bot commented Feb 20, 2022 • edited Loading

Codecov Report

nofar9792 commented Feb 20, 2022

nofar9792 commented Feb 20, 2022

nofar9792 commented Feb 22, 2022

yurishkuro commented Feb 22, 2022

nofar9792 commented Feb 23, 2022

albertteoh commented Feb 23, 2022

afishler-zz commented Feb 23, 2022

albertteoh commented Feb 23, 2022

nofar9792 commented Feb 24, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

albertteoh left a comment

Choose a reason for hiding this comment

albertteoh commented Feb 28, 2022

nofar9792 commented Feb 28, 2022

nofar9792 commented Mar 1, 2022

yurishkuro commented Mar 1, 2022

nofar9792 commented Jan 26, 2022 •

edited

Loading

codecov bot commented Feb 20, 2022 •

edited

Loading