ui: cluster overview

[couchand]

Part of the initial steps of making a homepage for the admin UI with the cluster viz as the main attraction, here is some work towards a structure for the page and the overview on the top. Clearly lots of work to do here, but I thought I'd get it up so we could get some eyes on it early.

Contributes to #19512.
See #19643 regarding naming things.

[cockroach-teamcity]

This change is 

[couchand]

Just wrapped up the first pass of the bar chart for capacity:

Now I'm gonna turn to getting the real data in there.

[dianasaur323]

@couchand what kind of feedback do you think would be most helpful? I'm assuming more so from the engineering team than from design and product?

[couchand]

@dianasaur323 at this point mostly just answers to the questions in #19643 are what would be helpful. I think I've got my finger on the several minor deviations from the mockup, and after hooking up the real data I'll take care of those, but feel free to let me know if there's anything major that I'm missing.

[mrtracy]

I had a couple of organization suggestions, mostly looks fine though.

---

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 23 at r1 (raw file):

const formatPercentage = d3.format("0.1%");
// TODO(couchand): Actual byte formatter.

We already have a byte formatting function available under src/util/format

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 107 at r1 (raw file):

    return (
      <div className="cluster-summary__section">
        <h3 className="cluster-summary__section--title">Replication Status</h3>

A few BEM notes:

• Use of the double-dash (--) indicates a modifier class; i.e. a class that is applied to indicate a different mode. This appears to be trying to make a sub-element.

• Sub-elements are discouraged. That is, instead of cluster-summary__section__title, use cluster-summary__title.

---

pkg/ui/styl/pages/cluster.styl, line 9 at r1 (raw file):

    width $chart-width
    &:first-child
      margin-top 10px

New styles should go right alongside the code... i.e. this should be in /views/cluster/containers/clusterOverview/cluster.styl. I would like to get rid of these top-level stylus files as much as possible.

---

Comments from Reviewable

[dianasaur323]

@couchand okay great! agreed that there are still some deviations from the mock ups, but i'll leave that to you for now. i'll take a look at that issue, although I don't have time today or tomorrow.

[couchand]

Review status: 0 of 7 files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 23 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

We already have a byte formatting function available under src/util/format

Yep! I've just been pushing up the intermediate commits, look at the latest pls. Or not yet, since I'm about to send up some more.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 107 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

A few BEM notes:

• Use of the double-dash (--) indicates a modifier class; i.e. a class that is applied to indicate a different mode. This appears to be trying to make a sub-element.

• Sub-elements are discouraged. That is, instead of cluster-summary__section__title, use cluster-summary__title.

Ugh at least BEM provides some structure, but it's still crazy trying to apply it to real components. As far as I can tell it just assumes everything is a two-level structure which, of course, oversimplifies things. As you can see below, at some point I just stopped bothering, figuring I'd go back and do a pass renaming everything at the end.

---

pkg/ui/styl/pages/cluster.styl, line 9 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

New styles should go right alongside the code... i.e. this should be in /views/cluster/containers/clusterOverview/cluster.styl. I would like to get rid of these top-level stylus files as much as possible.

Aside: what are your thoughts on using one of the React inline style libraries?

---

Comments from Reviewable

[couchand]

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions.

---

pkg/ui/src/redux/nodes.ts, line 175 at r9 (raw file):

          result.usedBytes += BytesUsed(n);
          result.usedMem += n.metrics[MetricConstants.rss];
          result.totalRanges += n.metrics[MetricConstants.ranges];

@mrtracy can you confirm if this change is sound? it really feels wrong, but I just cargo-culted the rest...

---

Comments from Reviewable

[mrtracy]

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions, some commit checks pending.

---

pkg/ui/src/redux/nodes.ts, line 175 at r9 (raw file):

Previously, couchand (Andrew Couch) wrote…

@mrtracy can you confirm if this change is sound? it really feels wrong, but I just cargo-culted the rest...

That looks correct.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 107 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Ugh at least BEM provides some structure, but it's still crazy trying to apply it to real components. As far as I can tell it just assumes everything is a two-level structure which, of course, oversimplifies things. As you can see below, at some point I just stopped bothering, figuring I'd go back and do a pass renaming everything at the end.

BEM wants a certain level of complexity in terms of a block; even if elements of a block are nested, each element is treated as a child of the block (and not a child of another element in the block).

If you encounter a situation where the two-level naming is not adequate because of the complicated structure of the block, BEM would prefer breaking that block into multiple blocks (rather than using element-of-element). I don't think that applies in this situation, though.

---

pkg/ui/styl/pages/cluster.styl, line 9 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

Aside: what are your thoughts on using one of the React inline style libraries?

It's not high on my priority list. We've gone through two CSS refactorings already, and I don't think we've given this one a chance.

---

Comments from Reviewable

[couchand]

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/ui/src/redux/nodes.ts, line 175 at r9 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

That looks correct.

To clarify my concern: if multiple nodes have replicas of the same range, will this not double-count them?

---

Comments from Reviewable

[mrtracy]

Review status: 0 of 8 files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

pkg/ui/src/redux/nodes.ts, line 175 at r9 (raw file):

Previously, couchand (Andrew Couch) wrote…

To clarify my concern: if multiple nodes have replicas of the same range, will this not double-count them?

For this metric, each node counts only the ranges for which it either
1.) The leader or 2.) There is no leader, and it is the first node in the rangedescriptor list

The intent is for this to be the total count of ranges when aggregated. It is possible to double-count due to active membership changes, but this number is accurate enough for monitoring purposes.

---

Comments from Reviewable

[couchand]

Thanks for the clarification @mrtracy, that makes sense.

I think this is ready to go (module squashing commits), PTAL, thanks!

[mrtracy]

Review status: 0 of 9 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 48 at r25 (raw file):

  el.datum(scaled);

  axis.tickFormat(function (d) {

You're modifying a package global here. These functions and variables should be moved into the CapacityChart class.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 70 at r25 (raw file):

  axisGroup.selectAll("text").attr("y", AXIS_MARGIN + TICK_SIZE);

  const bg = el.selectAll(".bg")

I think it would be much clearer just to forgo all this enter/exit logic and just add these elements to the JSX; there's always exactly one of each of these elements, d3 can easily select them if they are already present.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 95 at r25 (raw file):

}

export class CapacityChart extends React.Component<CapacityChartProps, {}> {

Should have descriptive comments on exported members.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 104 at r25 (raw file):

  }

  shouldComponentUpdate(props: CapacityChartProps) {

Why is this being done in shouldComponentUpdate? I would expect it to be in componentDidUpdate.

---

pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl, line 19 at r25 (raw file):

    padding-left 20px
    color gray
    display grid

This is really kind of... odd. I'm not sure how I feel about it, but it seems to make the CSS very closely bound to exactly whats currently being displayed. Would this not have been possible with a flexbox, or without using grid-template-areas?

Maybe it's fine, it just looks different than anything i've seen in CSS before and my immediate instinct is that it's too tightly coupled.

---

pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl, line 25 at r25 (raw file):

    grid-template-areas "cap-t cap-t . live-t live-t live-t . rep-t rep-t rep-t ." "cap-m cap-c . live-a live-b live-c . rep-a rep-b rep-c ." "cap-a cap-a . live-1 live-2 live-3 . rep-1 rep-2 rep-3 ."

    .cluster-summary__section

Use &__section here. The & parent reference operator doesn't just insert the parent's name, it also promotes the child rule to the parent's level.

That is, this currently compiles to:

.cluster-summary .cluster-summary__section{
}

But &__section would compile to:

.cluster-summary__section

Avoiding the nesting of CSS rules is one of the design goals of BEM, to keep the specificity flat.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 82 at r25 (raw file):

          <span className="value">{ liveNodes }</span>
        </div>
        <div className="cluster-summary__label live-nodes">

The metric/label groups are repeated a lot, seems like a great opportunity just to replace it with a function.

function clusterSummaryMetric(label: JSX.Element, value: JSX.Element)

Then these groups could look more like:

{clusterSummaryMetric(
  <span>Live<br />Nodes</span>,
  suspectNodes, 
  suspectNodes ? "warning" : ""
)}
{clusterSummaryMetric(
  <span>Dead<br />Nodes</span>,
  deadNodes, 
  deadNodes ? "alert" : ""
)}

You could even do them as stateless components, although that's up to you:

<ClusterSummaryMetric
  label={<span>Dead<br />Nodes</span>}
  value={deadNodes}, 
  valueClassName={deadNodes ? "alert" : ""}
/>

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 89 at r25 (raw file):

        </div>
        <div className="cluster-summary__label suspect-nodes">
          <span className="label">Suspect<br />Nodes</span>

I don't think the internal span is necessary, you're in the label div and the span isn't adding any additional information.

Also, is there no way to express this in CS without using the
labels?

---

pkg/ui/src/views/cluster/containers/nodesOverview/index.tsx, line 373 at r25 (raw file):

 */
function NodesPage() {
  return <div>

Parenthesis around Multi-line JSX

---

Comments from Reviewable

[couchand]

Review status: 0 of 9 files reviewed at latest revision, 13 unresolved discussions, some commit checks pending.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 48 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

You're modifying a package global here. These functions and variables should be moved into the CapacityChart class.

Welp.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 70 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I think it would be much clearer just to forgo all this enter/exit logic and just add these elements to the JSX; there's always exactly one of each of these elements, d3 can easily select them if they are already present.

I'll respectfully disagree. I think the whole chart can be encapsulated in a D3 component without coupling together a React component and a D3 component. The React integration can be generalized further, per the thread below on shouldComponentUpdate, then all the chart's details can be self-contained.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 95 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Should have descriptive comments on exported members.

Done.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 104 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Why is this being done in shouldComponentUpdate? I would expect it to be in componentDidUpdate.

Well we'd rather that React not muck about the internals of this component after the initial render, so we want shouldComponentUpdate to always return false. It's returning undefined implicitly, which I think is as good, except it does spit out a warning, so I'll update that. I think the pattern is not as clear when it's all inlined with the implementation of the chart, so I'll pull that out as an HOC to clarify.

---

pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl, line 19 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

This is really kind of... odd. I'm not sure how I feel about it, but it seems to make the CSS very closely bound to exactly whats currently being displayed. Would this not have been possible with a flexbox, or without using grid-template-areas?

Maybe it's fine, it just looks different than anything i've seen in CSS before and my immediate instinct is that it's too tightly coupled.

I did it with flexbox first, but it's really tough to get things to line up precisely in both dimensions. CSS grid is specifically meant for lining things up in two dimensions, whereas flexbox is intended for layout in one dimension.

I agree that the CSS gets coupled tightly to the contents, that's why I asked about inline styling. There's not really any getting around it here, since getting the contents to look right demands that each be treated individually. Putting the styles inline would make that slightly less gross.

---

pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl, line 25 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Use &__section here. The & parent reference operator doesn't just insert the parent's name, it also promotes the child rule to the parent's level.

That is, this currently compiles to:

.cluster-summary .cluster-summary__section{
}

But &__section would compile to:

.cluster-summary__section

Avoiding the nesting of CSS rules is one of the design goals of BEM, to keep the specificity flat.

Oh that's slick. Done.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 107 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

BEM wants a certain level of complexity in terms of a block; even if elements of a block are nested, each element is treated as a child of the block (and not a child of another element in the block).

If you encounter a situation where the two-level naming is not adequate because of the complicated structure of the block, BEM would prefer breaking that block into multiple blocks (rather than using element-of-element). I don't think that applies in this situation, though.

I don't know, just using your first point as an example, cluster-summary__title counds like it's the title for the whole cluster summary (which exists elsewhere), not the title for a particular section. Maybe it should be cluster-summary__section-title as a workaround? I'm not really worried about it, just trying to illustrate the point.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 82 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

The metric/label groups are repeated a lot, seems like a great opportunity just to replace it with a function.

function clusterSummaryMetric(label: JSX.Element, value: JSX.Element)

Then these groups could look more like:

{clusterSummaryMetric(
  <span>Live<br />Nodes</span>,
  suspectNodes, 
  suspectNodes ? "warning" : ""
)}
{clusterSummaryMetric(
  <span>Dead<br />Nodes</span>,
  deadNodes, 
  deadNodes ? "alert" : ""
)}

You could even do them as stateless components, although that's up to you:

<ClusterSummaryMetric
  label={<span>Dead<br />Nodes</span>}
  value={deadNodes}, 
  valueClassName={deadNodes ? "alert" : ""}
/>

Good call. Can't do it as a component yet, that requires React 16.

...

So I tried this, but after removing the internal span, the explicit version is clearer than trying to use a function.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 89 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I don't think the internal span is necessary, you're in the label div and the span isn't adding any additional information.

Also, is there no way to express this in CS without using the
labels?

Yeah, the internal span isn't needed, it's an artifact of some restructuring. I left it in since that's where the warning/alert class was set, but that can easily be moved up a level. Done.

---

pkg/ui/src/views/cluster/containers/nodesOverview/index.tsx, line 373 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Parenthesis around Multi-line JSX

Done.

---

Comments from Reviewable

[mrtracy]

LGTM

---

Review status: 0 of 10 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

---

pkg/ui/src/views/cluster/containers/clusterOverview/capacity.tsx, line 104 at r25 (raw file):

Previously, couchand (Andrew Couch) wrote…

Well we'd rather that React not muck about the internals of this component after the initial render, so we want shouldComponentUpdate to always return false. It's returning undefined implicitly, which I think is as good, except it does spit out a warning, so I'll update that. I think the pattern is not as clear when it's all inlined with the implementation of the chart, so I'll pull that out as an HOC to clarify.

Okay, I understand the reasoning behind this. We can disagree on the use of D3 for now, but if we're escaping to D3, this works.

---

pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl, line 19 at r25 (raw file):

Previously, couchand (Andrew Couch) wrote…

I did it with flexbox first, but it's really tough to get things to line up precisely in both dimensions. CSS grid is specifically meant for lining things up in two dimensions, whereas flexbox is intended for layout in one dimension.

I agree that the CSS gets coupled tightly to the contents, that's why I asked about inline styling. There's not really any getting around it here, since getting the contents to look right demands that each be treated individually. Putting the styles inline would make that slightly less gross.

I see the benefits of grid, but I'm less sold on the use of grid-template-areas in this way (where the top-level container is linked to its components rigidly). It seems like something that might work if there were fewer components, or a more exotic layout, but for me it's breaking down when you have 16 named elements which have their own visual relationships to each other (i.e. the live nodes title appearing on top of the life counts.

This is fine for now, no other components directly depend on it. However, I would like to address this before the strategy gets copy/pasted to different components.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 107 at r1 (raw file):

Previously, couchand (Andrew Couch) wrote…

I don't know, just using your first point as an example, cluster-summary__title counds like it's the title for the whole cluster summary (which exists elsewhere), not the title for a particular section. Maybe it should be cluster-summary__section-title as a workaround? I'm not really worried about it, just trying to illustrate the point.

Then the problem is that "cluster-summary" isn't the proper name for this block, because its being confused with the entire "cluster summary" section, even though it doesn't describe the entire section.

However, even in the case where this did contain the entire section, the use of "title" for the section title (i.e. "Cluster Overview") and the use of "section-title" for the section titles would be appropriate. The difference between "section-title" and "section__title" is subtle, but it's an important rule that gives predictable structure.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 82 at r25 (raw file):

Previously, couchand (Andrew Couch) wrote…

Good call. Can't do it as a component yet, that requires React 16.

...

So I tried this, but after removing the internal span, the explicit version is clearer than trying to use a function.

I'll take your word for it.

---

Comments from Reviewable

[couchand]

Review status: 0 of 10 files reviewed at latest revision, 11 unresolved discussions, all commit checks successful.

---

pkg/ui/src/views/cluster/containers/clusterOverview/cluster.styl, line 19 at r25 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

I see the benefits of grid, but I'm less sold on the use of grid-template-areas in this way (where the top-level container is linked to its components rigidly). It seems like something that might work if there were fewer components, or a more exotic layout, but for me it's breaking down when you have 16 named elements which have their own visual relationships to each other (i.e. the live nodes title appearing on top of the life counts.

This is fine for now, no other components directly depend on it. However, I would like to address this before the strategy gets copy/pasted to different components.

Roger, and generally agreed. The alternative is to use the grid lines to specify where things go, which is less clear, IMO. This all would be much better if we could list each string on a new line, as basically every grid example shows (is there any way to do that with stylus?).

I'm gonna merge this, but keep thinking about it, because it's hard to imagine what the most natural way to express these constraints would be, but I want to find it.

---

pkg/ui/src/views/cluster/containers/clusterOverview/index.tsx, line 107 at r1 (raw file):

Previously, mrtracy (Matt Tracy) wrote…

Then the problem is that "cluster-summary" isn't the proper name for this block, because its being confused with the entire "cluster summary" section, even though it doesn't describe the entire section.

However, even in the case where this did contain the entire section, the use of "title" for the section title (i.e. "Cluster Overview") and the use of "section-title" for the section titles would be appropriate. The difference between "section-title" and "section__title" is subtle, but it's an important rule that gives predictable structure.

Sure, my point was just that there really is more than two levels of structure here.

---

Comments from Reviewable