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 - Request rate number more readable #890

Merged

Conversation

nofar9792
Copy link
Contributor

Which problem is this PR solving?

Resolves #889

Short description of the changes

  • Formating the request rate value

before:
Screen Shot 2022-01-25 at 16 07 35

after:
image

before:
Screen Shot 2022-01-25 at 16 13 00

after:
Screen Shot 2022-01-25 at 16 10 24

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

codecov bot commented Feb 23, 2022

Codecov Report

Merging #890 (35702dd) into main (cc2831f) will increase coverage by 0.05%.
The diff coverage is 100.00%.

❗ Current head 35702dd differs from pull request most recent head 668e5e7. Consider uploading reports for the commit 668e5e7 to get more accurate results

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #890      +/-   ##
==========================================
+ Coverage   95.37%   95.43%   +0.05%     
==========================================
  Files         240      240              
  Lines        7462     7466       +4     
  Branches     1865     1866       +1     
==========================================
+ Hits         7117     7125       +8     
+ Misses        339      335       -4     
  Partials        6        6              
Impacted Files Coverage Δ
...nitor/ServicesView/operationDetailsTable/index.tsx 100.00% <100.00%> (ø)
...mponents/TracePage/TraceStatistics/tableValues.tsx 97.93% <0.00%> (+1.37%) ⬆️

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 cc2831f...668e5e7. Read the comment docs.

}

// Truncate number to two decimal places
return `${value.toString().match(/^-?\d+(?:\.\d{0,2})?/)![0]} req/s`;
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is not so much a question of decimal places, but of precision. E.g. 2.666667 is just as useless (too much precision) as 26666.67, but this code will allow the latter, whereas I would prefer to represent it as 26.6k - never more than 3 digits of precision

Choose a reason for hiding this comment

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

Good point and totally accepted. However, will be better just to go this path right now, and just improve on another iteration. Your comment definitely enhances the solution, but the current step will already provide an improvement which is immediate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yurishkuro yurishkuro merged commit fefb034 into jaegertracing:main Feb 28, 2022
@nofar9792 nofar9792 deleted the request-rate-two-decimal-places branch February 28, 2022 14:19
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 - Request rate number more readable
3 participants