ui: Network latency page improvements #103837

koorosh · 2023-05-24T13:38:12Z

use connectivity endpoint as a source of connection statuses and latency;
removed popup with disconnected peers, instead it will be shown
on latency matrix;

Resolves: #96101

Release note (ui change): add "Show dead nodes" option
to show/hide dead/decommissioned nodes from latency matrix.

Release note (ui change): "No connections" popup menu is removed. Now failed
connections are displayed on the latencies matrix.

test coverage (storybook added for visual validation)

This PR depends on following PRs that should be merged before this one:

example 1. one node cannot establish connection to another one.

example 2. Node 5 is stopped and it is considered as `unavailable` before setting as `dead`

example 3. Node 3 is dead. No connection from/to node. Show error message.

example 4. Decommissioned node.

blathers-crl · 2023-05-24T13:38:18Z

Thank you for contributing to CockroachDB. Please ensure you have followed the guidelines for creating a PR.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

I have added a few people who may be able to assist in reviewing:

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

cockroach-teamcity · 2023-05-24T13:38:25Z

This change is

blathers-crl · 2023-06-06T10:44:50Z

Thank you for updating your pull request.

Before a member of our team reviews your PR, I have some potential action items for you:

We notice you have more than one commit in your PR. We try break logical changes into separate commits, but commits such as "fix typo" or "address review commits" should be squashed into one commit and pushed with --force
When CI has completed, please ensure no errors have appeared.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

koorosh · 2023-06-13T09:53:25Z

сс @dongniwang I added video to show design changes.

tbg · 2023-06-16T12:40:18Z

Great to see this!

Drive-by comments, the red in the video for the decommissioned node is surprising to me. "red" means bad but decommissioned nodes are basically "not there", so if anything shouldn't they be greyed out or something like that?

Also, can the tooltip make it clear which way the connection goes? This is something I'm always second guessing. There should be a clear n1 --> n2 somewhere. A legend update like this in addition would be nice (I might've mixed up the direction, no idea)

Also want to x-post my comment from elsewhere, it would be great to surface the circuit breaker's connection error in the tooltip if the connection is in a failed state.

koorosh · 2023-06-19T12:42:14Z

Thanks Tobias for feedback, I've updated PR to reflect your suggestions

Updated PR with following

make tooltip more informative, with error info and indication of connection direction (from, to)
bring back page auto refreshing every 30sec
fix sorting dropdown component
persist show dead nodes option in URI search params

maryliag

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @koorosh)

pkg/ui/workspaces/db-console/src/redux/connectivity.ts line 1 at r6 (raw file):

import { AdminUIState } from "src/redux/state";

you're getting a lint error here because you're missing the copyright header

blathers-crl · 2023-06-26T15:12:44Z

Thank you for updating your pull request.

My owl senses detect your PR is good for review. Please keep an eye out for any test failures in CI.

_{🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf.}

koorosh · 2023-07-03T11:37:51Z

Quick demo:
https://www.loom.com/share/9b949e78f26f4254988dee154657f726?sid=6e4a5b39-e1b7-48db-ad60-f52fac3b2f05

maryliag

a few nits from me, otherwise the ui code
but I want to make sure you get an approval from @tbg before merging

Reviewed 9 of 14 files at r6, 12 of 12 files at r7, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @gtr, @j82w, @koorosh, @THardy98, @xinhaoz, and @zachlite)

pkg/ui/workspaces/db-console/src/views/reports/containers/network/latency/index.tsx line 296 at r7 (raw file):

  let showError = !!errorMessage;
  // Show errors for all connection statuses except ESTABLISHING status that treated exceptionally.

nit: ... status that is treated...

pkg/ui/workspaces/db-console/src/views/app/components/chip/styles.styl line 42 at r7 (raw file):

  &--white
    background $colors--neutral-0
    border-color #c0c6d9

there is a color variable for this one that you can use: $colors--neutral-4

tbg

Wonderful! I left a review on the Loom. The handling of decommissioned nodes looks like it will cause problems on real-world clusters. The rest is absolutely fantastic and I look forward to this hitting the world.

koorosh · 2023-07-05T10:35:23Z

I left a review on the Loom.

Thank you. I removed "Show dead nodes" option, so now we show only active nodes.

xinhaoz · 2023-07-05T19:20:18Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

@@ -113,17 +148,16 @@ export class Network extends React.Component<NetworkProps, INetworkState> {
  refresh(props = this.props) {


It looks like this function always uses this.props. Can we remove the param and just use this.props in the body?

xinhaoz · 2023-07-05T19:25:10Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

-    if (!_.isEqual(this.props.location, prevProps.location)) {
-      this.refresh(this.props);
-    }
+  componentDidUpdate(_prevProps: NetworkProps) {


We should avoid refreshing unconditionally in componentDidUpdate moving forward (even if cacheddatareducer will implcity throttle these requests). Is there a better, more reliable way or place in the component to issue this refresh? Since this is already a widely used pattern with these db-console specific apis we can live it here for now if there's no alternative.

xinhaoz · 2023-07-05T19:26:49Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

-    displayIdentities: Identity[],
-    noConnections: NoConnection[],
-  ) {
+  renderLatencyTable(latencies: number[], displayIdentities: Identity[]) {


Can this be lifted out into a separate component?

xinhaoz · 2023-07-05T19:28:28Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

@@ -231,11 +259,9 @@ export class Network extends React.Component<NetworkProps, INetworkState> {
    const latencyTable = (


Why define this component here instead of inlining it with the rest of the JSX?

xinhaoz · 2023-07-05T19:31:30Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

+      .forEach(values => {
+        const localities = searchQuery(values.locality).split(",");
+        localities.forEach((locality: string) => {
+          if (locality !== "") {


Suggestion:

if (locality === "") { return; } ... rest of the body

xinhaoz · 2023-07-05T19:33:42Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

+            const value = locality.match(/^\w+/gi)
+              ? locality.match(/^\w+/gi)[0]
+              : null;
+            if (!sort.some(x => x.id === value)) {


Same here, change to:

if (sort.any(...)) { return; } ... rest

xinhaoz · 2023-07-05T19:35:27Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

+          {latencyTable}
+        </div>
+      </Fragment>
+    );
  }

  getSortParams = (data: Identity[]) => {


This function is quite lengthy with a lot of nested array processing, can you leave a comment summarizing what it's doing?

xinhaoz · 2023-07-05T19:37:07Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx

+                      ];
+                    } else if (
+                      itemLocalitySplited === value &&
+                      !sortValue.filters.reduce(


would an any work here?

xinhaoz · 2023-07-05T19:46:48Z

pkg/ui/workspaces/db-console/src/views/reports/containers/network/latency/index.tsx

-            );
-          }),
+        {React.Children.toArray(
+          _.map(data, (value, index) =>


nit: we should prefer native array functions for uniformity where applicable

koorosh

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @gtr, @j82w, @maryliag, @THardy98, @xinhaoz, and @zachlite)

pkg/ui/workspaces/db-console/src/redux/connectivity.ts line 1 at r6 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

you're getting a lint error here because you're missing the copyright header

Done.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 148 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

It looks like this function always uses this.props. Can we remove the param and just use this.props in the body?

Done.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 159 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

We should avoid refreshing unconditionally in componentDidUpdate moving forward (even if cacheddatareducer will implcity throttle these requests). Is there a better, more reliable way or place in the component to issue this refresh? Since this is already a widely used pattern with these db-console specific apis we can live it here for now if there's no alternative.

I tend to keep it as is to avoid potential regressions and avoid significant component refactoring in scope of this change.
This component isn't perfect at all and IMO needs lots of refactoring to be well structured and easy to maintain.
I created a separate #106244 to address such improvements.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 243 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Can this be lifted out into a separate component?

Same as comment above. This component shares state so it's not just trivial extraction of pure component.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 259 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Why define this component here instead of inlining it with the rest of the JSX?

It is used twice depending on condition, avoids code duplication.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 304 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

This function is quite lengthy with a lot of nested array processing, can you leave a comment summarizing what it's doing?

Definitely! 👍🏼

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 312 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Suggestion:

if (locality === "") { return; }
... rest of the body

I like this! Done.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 316 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

Same here, change to:
if (sort.any(...)) { return; }
... rest

Done.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/index.tsx line 338 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

would an any work here?

Done.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/latency/index.tsx line 296 at r7 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

nit: ... status that is treated...

Thanks. Done.

pkg/ui/workspaces/db-console/src/views/reports/containers/network/latency/index.tsx line 504 at r8 (raw file):

Previously, xinhaoz (Xin Hao Zhang) wrote…

nit: we should prefer native array functions for uniformity where applicable

These components heavily use lodash in many places. Prefer to change it in scope of # #106244

pkg/ui/workspaces/db-console/src/views/app/components/chip/styles.styl line 42 at r7 (raw file):

Previously, maryliag (Marylia Gutierrez) wrote…

there is a color variable for this one that you can use: $colors--neutral-4

Done.

- use connectivity endpoint as a source of connection statuses and latency; - remove popup with disconnected peers, instead it will be shown on latency matrix; Release note: None

maryliag

Reviewed 5 of 5 files at r9, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @gtr, @j82w, @THardy98, @xinhaoz, and @zachlite)

koorosh · 2023-07-17T22:38:38Z

bors r+

craig · 2023-07-17T23:43:12Z

Build failed (retrying...):

Bazel Essential CI (Cockroach)

craig · 2023-07-17T23:51:42Z

This PR was included in a batch that was canceled, it will be automatically retried

craig · 2023-07-18T01:41:41Z

Build succeeded:

Bazel Essential CI (Cockroach)

blathers-crl bot added O-community Originated from the community X-blathers-triaged blathers was able to find an owner labels May 24, 2023

blathers-crl bot requested review from andrewbaptist and Santamaura May 24, 2023 13:38

koorosh removed request for Santamaura and andrewbaptist May 25, 2023 15:44

j82w requested a review from a team May 30, 2023 23:30

koorosh force-pushed the network-latency-page-improvements branch from 9e40a2d to c385254 Compare June 6, 2023 10:44

koorosh marked this pull request as ready for review June 6, 2023 10:57

koorosh requested a review from a team June 6, 2023 10:57

koorosh force-pushed the network-latency-page-improvements branch from c385254 to 85ac137 Compare June 19, 2023 10:16

koorosh requested review from a team as code owners June 19, 2023 10:16

koorosh requested a review from a team June 19, 2023 10:16

koorosh force-pushed the network-latency-page-improvements branch from 85ac137 to c4960a8 Compare June 20, 2023 10:59

maryliag reviewed Jun 20, 2023

View reviewed changes

koorosh force-pushed the network-latency-page-improvements branch 3 times, most recently from cf97a99 to 6dffc91 Compare June 26, 2023 15:12

koorosh removed the O-community Originated from the community label Jun 26, 2023

koorosh force-pushed the network-latency-page-improvements branch 2 times, most recently from 4e48335 to 25fe356 Compare June 26, 2023 19:32

koorosh force-pushed the network-latency-page-improvements branch from 25fe356 to 4bf14ca Compare June 27, 2023 08:27

koorosh requested review from zachlite, j82w, THardy98, xinhaoz and gtr June 30, 2023 13:33

maryliag approved these changes Jul 4, 2023

View reviewed changes

tbg reviewed Jul 5, 2023

View reviewed changes

koorosh force-pushed the network-latency-page-improvements branch from 4bf14ca to e4c9a03 Compare July 5, 2023 10:25

xinhaoz reviewed Jul 5, 2023

View reviewed changes

koorosh force-pushed the network-latency-page-improvements branch from e4c9a03 to f7a97c6 Compare July 6, 2023 09:55

koorosh commented Jul 6, 2023

View reviewed changes

ui: Latency page UI improvements

8e078fd

- use connectivity endpoint as a source of connection statuses and latency; - remove popup with disconnected peers, instead it will be shown on latency matrix; Release note: None

koorosh force-pushed the network-latency-page-improvements branch from f7a97c6 to 8e078fd Compare July 17, 2023 20:33

maryliag approved these changes Jul 17, 2023

View reviewed changes

craig bot merged commit 7675ca4 into cockroachdb:master Jul 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ui: Network latency page improvements #103837

ui: Network latency page improvements #103837

koorosh commented May 24, 2023 •

edited

Loading

blathers-crl bot commented May 24, 2023

cockroach-teamcity commented May 24, 2023

blathers-crl bot commented Jun 6, 2023

koorosh commented Jun 13, 2023

tbg commented Jun 16, 2023 •

edited

Loading

koorosh commented Jun 19, 2023 •

edited

Loading

maryliag left a comment

blathers-crl bot commented Jun 26, 2023

koorosh commented Jul 3, 2023

maryliag left a comment

tbg left a comment

koorosh commented Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

xinhaoz Jul 5, 2023

koorosh left a comment

maryliag left a comment

koorosh commented Jul 17, 2023

craig bot commented Jul 17, 2023

craig bot commented Jul 17, 2023

craig bot commented Jul 18, 2023

		@@ -113,17 +148,16 @@ export class Network extends React.Component<NetworkProps, INetworkState> {
		refresh(props = this.props) {

		@@ -231,11 +259,9 @@ export class Network extends React.Component<NetworkProps, INetworkState> {
		const latencyTable = (

ui: Network latency page improvements #103837

ui: Network latency page improvements #103837

Conversation

koorosh commented May 24, 2023 • edited Loading

example 1. one node cannot establish connection to another one.

example 2. Node 5 is stopped and it is considered as unavailable before setting as dead

example 3. Node 3 is dead. No connection from/to node. Show error message.

example 4. Decommissioned node.

blathers-crl bot commented May 24, 2023

cockroach-teamcity commented May 24, 2023

blathers-crl bot commented Jun 6, 2023

koorosh commented Jun 13, 2023

tbg commented Jun 16, 2023 • edited Loading

koorosh commented Jun 19, 2023 • edited Loading

maryliag left a comment

Choose a reason for hiding this comment

blathers-crl bot commented Jun 26, 2023

koorosh commented Jul 3, 2023

maryliag left a comment

Choose a reason for hiding this comment

tbg left a comment

Choose a reason for hiding this comment

koorosh commented Jul 5, 2023

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

koorosh left a comment

Choose a reason for hiding this comment

maryliag left a comment

Choose a reason for hiding this comment

koorosh commented Jul 17, 2023

craig bot commented Jul 17, 2023

craig bot commented Jul 17, 2023

craig bot commented Jul 18, 2023

koorosh commented May 24, 2023 •

edited

Loading

example 2. Node 5 is stopped and it is considered as `unavailable` before setting as `dead`

tbg commented Jun 16, 2023 •

edited

Loading

koorosh commented Jun 19, 2023 •

edited

Loading