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 - error rates - yDomain #913

Merged
merged 1 commit into from
Mar 15, 2022

Conversation

nofar9792
Copy link
Contributor

Signed-off-by: nofar9792 nofar.cohen@logz.io

Which problem is this PR solving?

Resolves #912

Short description of the changes

I changed the wrong yDomain.
it affected the main error graph domain
before:
image

after:
image

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

codecov bot commented Mar 15, 2022

Codecov Report

Merging #913 (2233202) into main (d19b02f) will decrease coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #913      +/-   ##
==========================================
- Coverage   95.29%   95.26%   -0.03%     
==========================================
  Files         240      240              
  Lines        7497     7502       +5     
  Branches     1870     1871       +1     
==========================================
+ Hits         7144     7147       +3     
- Misses        347      349       +2     
  Partials        6        6              
Impacted Files Coverage Δ
...r-ui/src/components/Monitor/ServicesView/index.tsx 97.02% <ø> (ø)
...nitor/ServicesView/operationDetailsTable/index.tsx 100.00% <100.00%> (ø)
packages/jaeger-ui/src/utils/date.tsx 94.73% <100.00%> (+0.07%) ⬆️
...nents/TracePage/TraceTimelineViewer/SpanBarRow.tsx 94.44% <0.00%> (-5.56%) ⬇️

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 a9e575f...2233202. Read the comment docs.

@@ -132,7 +132,7 @@ export class OperationTableDetails extends React.PureComponent<TProps, TState> {
<REDGraph
dataPoints={row.dataPoints.service_operation_error_rate}
color="#CD513A"
yDomain={[0, 100]}
yDomain={[0, 1]}
Copy link
Member

Choose a reason for hiding this comment

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

why is it that we have different yDomain in those two different files even though they both deal with error rate? Can we not converge on the same display methodology?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in the main graph, we want to show values in percentages.
The value we get are between 1 to 100.

ini the operation table, the user doesn't see the values/legend, so it is easier just to change the yDomain

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't really answer my question - why have two different ways when we can have one?

To put it differently, as a way of blameless postmortem to the original change where you say "I made a mistake" - I think having a single representation would help avoiding such mistakes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

multiple the values in the operation table will make the code more complex, I thought that in this case, it's better just to change the yDomain and not change the all the values

@yurishkuro yurishkuro merged commit 79e856a into jaegertracing:main Mar 15, 2022
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 - error rates - yDomain
2 participants