Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upui: Add tooltips to Time Series on the Replication dashboard #26328
Conversation
piyush-singh
requested a review
from cockroachdb/admin-ui-prs
as a
code owner
Jun 1, 2018
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
|
This change is |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
CLAassistant
commented
Jun 1, 2018
•
|
|
piyush-singh
requested a review
from
Amruta-Ranade
Jun 1, 2018
couchand
requested changes
Jun 4, 2018
Can you add the "fixes #..." to the commit message please? Also, I think a release note is in order here.
It looks like you have some lint failures: https://teamcity.cockroachdb.com/viewLog.html?tab=buildLog&logTab=tree&filter=debug&expand=all&buildId=692842&_focus=1511
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
piyush-singh
Jun 4, 2018
@couchand updated commit message and addressed all feedback, if it looks good, let me know and I can grab feedback from @Amruta-Ranade
piyush-singh
commented
Jun 4, 2018
|
@couchand updated commit message and addressed all feedback, if it looks good, let me know and I can grab feedback from @Amruta-Ranade |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
piyush-singh
commented
Jun 26, 2018
|
@couchand and @Amruta-Ranade for a final pass |
| <Axis label="replicas"> | ||
| <Metric name="cr.store.replicas" title="Replicas" /> | ||
| <Metric name="cr.store.replicas.quiescent" title="Quiescent" /> | ||
| </Axis> | ||
| </LineGraph>, | ||
| <LineGraph title="Range Operations" sources={storeSources}> | ||
| <LineGraph title="Range Operations" sources={storeSources} | ||
| tooltip = {`Ranges with split, add, or remove operations ${tooltipSelection}.`} |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
couchand
Jun 27, 2018
Member
I think this is counting the number of operations, not the number of ranges with those operations. For the most part it would be the same, but not always. For instance, if a range is split twice, we expect it to be counted twice here.
also please no spaces around the =. surprised that our linter didn't complain about this, but i think the jsx checking is kind of lax.
couchand
Jun 27, 2018
Member
I think this is counting the number of operations, not the number of ranges with those operations. For the most part it would be the same, but not always. For instance, if a range is split twice, we expect it to be counted twice here.
also please no spaces around the =. surprised that our linter didn't complain about this, but i think the jsx checking is kind of lax.
| sources={storeSources} | ||
| tooltip={( | ||
| <div> | ||
| Snapshots {tooltipSelection} <br/> |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| tooltip={( | ||
| <div> | ||
| Snapshots {tooltipSelection} <br/> | ||
| When a node is far behind the log file for a range, the cluster can |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
| <dt>Applied (Preemptive)</dt> | ||
| <dd>Snapshots applied {tooltipSelection} per second that were anticipated ahead of time</dd> | ||
| <dt>Reserved</dt> | ||
| <dd>Slots {tooltipSelection} reserved per second for incoming snapshots that will be sent to a node</dd> |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
couchand
Jun 27, 2018
Member
the end of this description reads a little awkward after tooltipSelection has been replaced
couchand
Jun 27, 2018
Member
the end of this description reads a little awkward after tooltipSelection has been replaced
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
Amruta-Ranade
Jul 11, 2018
Contributor
@piyush-singh I need your help to parse the tooltips. Can we schedule a quick session to walk through the PR?
|
@piyush-singh I need your help to parse the tooltips. Can we schedule a quick session to walk through the PR? |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
piyush-singh
commented
Jul 17, 2018
|
@Amruta-Ranade any time this week works for me! |
| <div> | ||
| Number of replicas and quiesced replicas {tooltipSelection}. | ||
| <br/> | ||
| Quiesced replicas have no pending reads or writes and have temporarily disabled consensus protocols. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
piyush-singh
Jul 23, 2018
from @Amruta-Ranade : We should make this clearer to the user, specifically what action needs to be taken when this number is high or low.
piyush-singh
Jul 23, 2018
from @Amruta-Ranade : We should make this clearer to the user, specifically what action needs to be taken when this number is high or low.
| <div> | ||
| Snapshots {tooltipSelection} <br/> | ||
| When a node is far behind the log file for a range, the cluster can | ||
| send it a snapshot of the range and it can start following the log from there. |
This comment has been minimized.
Show comment
Hide comment
This comment has been minimized.
piyush-singh
Jul 23, 2018
from @Amruta-Ranade : this needs to be simplified to make it easier for users outside of CRL to understand.
piyush-singh
Jul 23, 2018
from @Amruta-Ranade : this needs to be simplified to make it easier for users outside of CRL to understand.
piyush-singh commentedJun 1, 2018
•
edited
Release note: Added tooltips for descriptions of time series graphs in Replication Dashboard.
Fixes #17670