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

Monitor Tab - show 95 Latency in a more readable time-unit #893

Merged
merged 7 commits into from
Feb 28, 2022

Conversation

nofar9792
Copy link
Contributor

Which problem is this PR solving?

Resolves #892

Short description of the changes

before:
Screen Shot 2022-01-25 at 16 09 10

after:
Screen Shot 2022-02-24 at 0 31 09

nofar9792 added 3 commits January 31, 2022 15:13
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #893 (c84549d) into main (fefb034) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #893   +/-   ##
=======================================
  Coverage   95.29%   95.29%           
=======================================
  Files         240      240           
  Lines        7466     7466           
  Branches     1866     1866           
=======================================
  Hits         7115     7115           
  Misses        345      345           
  Partials        6        6           
Impacted Files Coverage Δ
...nitor/ServicesView/operationDetailsTable/index.tsx 100.00% <100.00%> (ø)
packages/jaeger-ui/src/utils/date.tsx 94.36% <100.00%> (ø)

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 fefb034...c84549d. Read the comment docs.

@yurishkuro
Copy link
Member

Don't have a problem with this PR itself, but I am not a fan of this timeConversion function, it reduces precision more than necessary. E.g. 5900ms would be displayed as 5s - this is very misleading.

Could we fix this function to:

  1. use rounding, not floor function
  2. keep at least 2 significant digits
  3. use lower-case units

@nofar9792
Copy link
Contributor Author

nofar9792 commented Feb 24, 2022

Don't have a problem with this PR itself, but I am not a fan of this timeConversion function, it reduces precision more than necessary. E.g. 5900ms would be displayed as 5s - this is very misleading.

Could we fix this function to:

  1. use rounding, not floor function
  2. keep at least 2 significant digits
  3. use lower-case units

lower-case units? change timeConversion or use toLowerCase only in my case?

@yurishkuro
Copy link
Member

change timeConversion. It's weird that it mixes cases between us/ms and Sec/Hrs

@nofar9792
Copy link
Contributor Author

nofar9792 commented Feb 24, 2022

change timeConversion. It's weird that it mixes cases between us/ms and Sec/Hrs

OK, it is going to affect this place too:
Screen Shot 2022-02-24 at 22 56 07

Signed-off-by: nofar9792 <nofar.cohen@logz.io>
@nofar9792
Copy link
Contributor Author

change timeConversion. It's weird that it mixes cases between us/ms and Sec/Hrs

done

@nofar9792
Copy link
Contributor Author

@yurishkuro ping

yurishkuro
yurishkuro previously approved these changes Feb 28, 2022
nofar9792 added 2 commits February 28, 2022 16:23
Signed-off-by: nofar9792 <nofar.cohen@logz.io>
@yurishkuro yurishkuro merged commit 7294884 into jaegertracing:main Feb 28, 2022
@nofar9792 nofar9792 deleted the latency-unit-time branch February 28, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Monitor Tab - show 95 Latency in a more readable time-unit
2 participants