ui: set overview page node list data with stale tag if node is dead

[iAaronBuck]

71618: ui: set overview page node list data with stale tag if node is dead

Issue: #71618
Epic: None

Release note (ui change): Currently, the stale node metrics displayed to a user in the Cluster Overview Nodes Table may mislead users in to thinking that they are current values when in fact they are stale. This change rectifies that and adds a stale tag to metrics displayed to the user. This allows for users to be informed about the staleness of the data displayed to them regarding dead nodes.

[cockroach-teamcity]

This change is 

[zachlite]

Nice first PR! I appreciate the description and the screenshot!
See my comments below.
Additionally, there should be a Release Note since we've made a user-facing UI change.

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

---

-- commits line 2 at r1:
Nit: Commit messages should provide an indication of the changes made in the commit.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 271 at r1 (raw file):

      key: "uptime",
      dataIndex: "uptime",
      render: (_text: string, record: NodeStatusRow) =>

Rather than repeat this logic a few times, you could write a function:
formatWithPossibleStaleIndicator(value: string, status: AggregatedNodeStatus | LivenessStatus): string

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 272 at r1 (raw file):

      dataIndex: "uptime",
      render: (_text: string, record: NodeStatusRow) =>
        _text +

Nit: use string interpolation with ${} instead of concatenating with +
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#string_interpolation

[iAaronBuck]

Thanks for the suggestions!

I have implemented them and hope that this amendment is good to go.

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

---

-- commits line 2 at r1:

Previously, zachlite wrote…

Nit: Commit messages should provide an indication of the changes made in the commit.

Thanks for pointing this out, I'll be sure to put the commit message in the proper place next time, rather than just as a comment in the PR itself.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 271 at r1 (raw file):

Previously, zachlite wrote…

Rather than repeat this logic a few times, you could write a function:
formatWithPossibleStaleIndicator(value: string, status: AggregatedNodeStatus | LivenessStatus): string

I implemented a change along the lines of your suggestion. The second value is sent to render with type NodeStatusRow, so I was not able to set it to type AggregatedNodeStatus | LivenessStatus as suggested. Please let me know if this is fine.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 272 at r1 (raw file):

Previously, zachlite wrote…

Nit: use string interpolation with ${} instead of concatenating with +
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#string_interpolation

Thanks for the suggestion! I have implemented it.

[zachlite]

displayed to the user, as shown below.

The release notes get parsed and go into auto-generated documentation, so the "as shown below" reference won't make any sense when someone comes across your comment in the release notes.

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

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 225 at r3 (raw file):

  text: string,
  record: NodeStatusRow,
): string => `${text}${getStaleIndicator(record.status)}`;

Nit: I don't like getStaleIndicator returning an empty string. This seems unnecessary.
Try this:

// formatWithPossibleStatusIndicator

if (status.NODE_STATUS_DEAD || status.DEAD) {
  return `${text} (stale)`;
}

return text;

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 293 at r3 (raw file):

      ),
      render: (_text: string, record: NodeStatusRow) =>
        util.Percentage(record.usedCapacity, record.availableCapacity),

You still need this

[iAaronBuck]

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

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 225 at r3 (raw file):

if (status.NODE_STATUS_DEAD || status.DEAD) {
return ${text} (stale);
}

return text;
Thanks for your suggestion. I implemented it.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 293 at r3 (raw file):

Previously, zachlite wrote…

You still need this

ty, I brought it back.

[zachlite]

See my latest comments before you merge.

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

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 282 at r5 (raw file):

      key: "uptime",
      dataIndex: "uptime",
      render: formatWithPossibleStaleIndicator,

Since you added a render fn, you can remove this.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodesOverview/index.tsx line 291 at r5 (raw file):

    {
      key: "replicas",
      dataIndex: "replicas",

Same.

[iAaronBuck]

bors r+

[craig]

🕐 Waiting for PR status (GitHub check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

[craig]

GitHub status checks took too long to complete, so bors is giving up. You can adjust bors configuration to have it wait longer if you like.

[iAaronBuck]

bors r+

[craig]

Build failed (retrying...):

• Bazel Essential CI (Cockroach)

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)