streamingccl,db-console: add Cross-Cluster Replication metrics page

[adityamaru]

This change adds a new option to the metric dropdown dashboard
for Cross-Cluster Replication. This page will show all the user-facing
metrics relevant to C2C replication.

Informs: #95995

Release note: None

server,db-console: add feature flag for Cross-Cluster Replication dashboard

This change adds a feature flag whose value is determined by the cluster
setting cross_cluster_replication.enabled. This feature flag is then used
when rendering the metrics dashboard dropdown options to decide whether to
display the cross-cluster replication dashboard.

This change also deletes the session variable and associated cluster setting
sql.defaults.experimental_stream_replication.enabled so as to unify all
interactions with cross-cluster replication under a single cluster setting.

Fixes: #95995

Release note: None

[cockroach-teamcity]

This change is 

[adityamaru]

Adding some screenshots of the above change:

• With set cluster setting cross_cluster_replication.enabled = false;

• With set cluster setting cross_cluster_replication.enabled = true;

[adityamaru]

I've been talking to @benbardin about this diff so I expect him to lead the review, but @stevendanna / @lidorcarmel it would be nice to get your LGTM on some of the C2C cluster setting + metrics changes. Thanks!

[adityamaru]

oh this is no bueno, i'll have to inject this value or move the cluster setting to a non-ccl location.

[dhartunian]

Reviewed 3 of 3 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @benbardin, @lidorcarmel, @srosenberg, and @stevendanna)

---

pkg/ccl/streamingccl/settings.go line 19 at r2 (raw file):

// CrossClusterReplicationEnabled enables the ability to setup and control a
// cross cluster replication stream.
var CrossClusterReplicationEnabled = settings.RegisterBoolSetting(

if you wanted to, you could also read the cluster setting in the frontend instead of also wiring up a feature flag. the cluster settings could be more easily refreshed at runtime in the client as well.

if you look in clusterSettings.selectors.ts there are examples of cluster setting redux selectors you could wire up into nodeGraphs.

---

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

  crossClusterReplication: {
    label: "Cross-Cluster Replication",
    component: crossClusterReplication,

nit: just for consistency, calling this a *Dashboard would be nice.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 39 at r1 (raw file):

    </LineGraph>,
    <LineGraph
      title="KV Bytes"

do the byte computations need to be measured as "rates"? they're monotonic counters, right?

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 48 at r1 (raw file):

          title="KV Bytes"
          downsampleMax
          aggregateMax

you might want aggregateSum here. The aggregator decides how metrics from multiple nodes are aggregated together when someone looks at a "cluster" view of the metrics. I think in the case of byte counters you want to add them. I believe sum is the default aggregator (see queryFromProps in metricDataProvider/index.tsx) so you can also omit it.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 55 at r1 (raw file):

      title="SST Bytes"
      sources={storeSources}
      tooltip={`SST bytes (compressed) sent to KV by all replication jobs`}

do customers know how to disambiguate KV bytes and SST bytes?

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 62 at r1 (raw file):

          title="SST Bytes"
          downsampleMax
          aggregateMax

same as above, probably want "sum".

[benbardin]

nothing to add beyond david's comments and your own! though I'd certainly appreciate somebody who knows streamingest better reading that directory.

[adityamaru]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @lidorcarmel, @srosenberg, and @stevendanna)

---

pkg/ccl/streamingccl/settings.go line 19 at r2 (raw file):

Previously, dhartunian (David Hartunian) wrote…

if you wanted to, you could also read the cluster setting in the frontend instead of also wiring up a feature flag. the cluster settings could be more easily refreshed at runtime in the client as well.

if you look in clusterSettings.selectors.ts there are examples of cluster setting redux selectors you could wire up into nodeGraphs.

Thanks for this, it makes the change smaller! I did have to move the filter call into render instead of leaving it in the constructor to get things working. I did this because we refresh the settings in componentDidMount and so the state is only set after that method is invoked. I'm very new to redux/react so let me know if there is a better place I should move the filter so that it is not invoked on each render.

---

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

Previously, dhartunian (David Hartunian) wrote…

nit: just for consistency, calling this a *Dashboard would be nice.

ahh yes, fixed.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 39 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

do the byte computations need to be measured as "rates"? they're monotonic counters, right?

They are monotonic counters. After reading Snapshot Data Received in replication.tsx I realize I should add nonNegativeRate right?

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 48 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

you might want aggregateSum here. The aggregator decides how metrics from multiple nodes are aggregated together when someone looks at a "cluster" view of the metrics. I think in the case of byte counters you want to add them. I believe sum is the default aggregator (see queryFromProps in metricDataProvider/index.tsx) so you can also omit it.

Yup you're right, thanks. I believe I can omit both the downsample and aggregate params.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 55 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

do customers know how to disambiguate KV bytes and SST bytes?

Good question, and I think the answer is no. Another alternative is to refer to them as logical bytes and physical bytes. We do have precedence for these terms in https://www.cockroachlabs.com/docs/stable/ui-storage-dashboard.html, @lidorcarmel curious what you think?

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 62 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

same as above, probably want "sum".

Done.

[dhartunian]

Reviewed 20 of 20 files at r3.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @lidorcarmel, @srosenberg, and @stevendanna)

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 176 at r4 (raw file):

  refresh = () => {
    this.props.refreshNodes();
    this.props.refreshLiveness();

consider adding a refreshSettings() prop here (via the Selectors), and calling it. This will ensure that settings changes are refreshed when the metrics page is loaded since we now depend on them. will hopefully prevent one or two support tickets where ppl don't see what they expect.

This prop will be added in the mapDispatchToProps redux helper. (MapStateToProps lets your component access data from the Redux store, and MapDispatchToProps gives your component handles to action dispatchers that can mutate the redux store)

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 332 at r4 (raw file):

      : dashboardDropdownOptions.filter(
          option => option.label !== "Cross-Cluster Replication",
        );

This is fine to put here. If the list was long, or the filter was complex, it would be worth memoizing but there's plenty of other stuff slowing the code down here already to load and render metrics.

What you did is actually recommended here https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization until you hit performance constraints.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 39 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

They are monotonic counters. After reading Snapshot Data Received in replication.tsx I realize I should add nonNegativeRate right?

👍

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 55 at r1 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

Good question, and I think the answer is no. Another alternative is to refer to them as logical bytes and physical bytes. We do have precedence for these terms in https://www.cockroachlabs.com/docs/stable/ui-storage-dashboard.html, @lidorcarmel curious what you think?

SGTM. Especially since we have documentation around the difference.

[lidorcarmel]

streamingest stuff lgtm.

[lidorcarmel]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @adityamaru, @dhartunian, @srosenberg, and @stevendanna)

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 55 at r1 (raw file):

Previously, dhartunian (David Hartunian) wrote…

SGTM. Especially since we have documentation around the difference.

the kv bytes can be renamed to logical, I think it's accurate.

but the sst bytes is not exactly physical - it is the number of bytes in ssts that replication sends to kv, which is not really the physical bytes: physical is (I think) the bytes written to the disk. the sst bytes are different because kv may uncompress the sst (if it's small) and also, pebble may compact which will write more to the disk. I think it's ok to have the name say exactly what we do in this case.

[lidorcarmel]

@adityamaru you might want to update some text in chart_catalog.go for those metric renames.

[adityamaru]

Yup nice catch, done!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @lidorcarmel, @srosenberg, and @stevendanna)

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 176 at r4 (raw file):

Previously, dhartunian (David Hartunian) wrote…

consider adding a refreshSettings() prop here (via the Selectors), and calling it. This will ensure that settings changes are refreshed when the metrics page is loaded since we now depend on them. will hopefully prevent one or two support tickets where ppl don't see what they expect.

This prop will be added in the mapDispatchToProps redux helper. (MapStateToProps lets your component access data from the Redux store, and MapDispatchToProps gives your component handles to action dispatchers that can mutate the redux store)

We already have a refreshNodeSettings property. Following it through it seems to be driven by the refreshSettings in apiReducers.ts which looks like what we want.

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 332 at r4 (raw file):

Previously, dhartunian (David Hartunian) wrote…

This is fine to put here. If the list was long, or the filter was complex, it would be worth memoizing but there's plenty of other stuff slowing the code down here already to load and render metrics.

What you did is actually recommended here https://reactjs.org/blog/2018/06/07/you-probably-dont-need-derived-state.html#what-about-memoization until you hit performance constraints.

perfect, thanks

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/dashboards/crossClusterReplication.tsx line 55 at r1 (raw file):

Previously, lidorcarmel (Lidor Carmel) wrote…

the kv bytes can be renamed to logical, I think it's accurate.

but the sst bytes is not exactly physical - it is the number of bytes in ssts that replication sends to kv, which is not really the physical bytes: physical is (I think) the bytes written to the disk. the sst bytes are different because kv may uncompress the sst (if it's small) and also, pebble may compact which will write more to the disk. I think it's ok to have the name say exactly what we do in this case.

makes sense to me, changed kv bytes to logical bytes.

[dhartunian]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @adityamaru, @lidorcarmel, @srosenberg, and @stevendanna)

---

pkg/ui/workspaces/db-console/src/views/cluster/containers/nodeGraphs/index.tsx line 176 at r4 (raw file):

Previously, adityamaru (Aditya Maru) wrote…

We already have a refreshNodeSettings property. Following it through it seems to be driven by the refreshSettings in apiReducers.ts which looks like what we want.

ah nice! I missed that. 👍

[adityamaru]

TFTRs!

bors r+

[craig]

Build succeeded:

• Bazel Essential CI (Cockroach)

[rafiss]

@adityamaru exciting change! since this is adding a user-visible page to the DB console, it needs a release note. or since this was already merged, we'd need to make a DOCS ticket so that the page gets documented at https://www.cockroachlabs.com/docs/v22.2/ui-overview

[adityamaru]

it needs a release note. or since this was already merged, we'd need to make a DOCS ticket so that the page gets documented

Since we only plan on releasing C2C for private preview in 23.1 we've decided not to have public-facing documentation or release notes for any of the work around this new feature. In the future when we do decide to release it publically we'll have to ensure we document this along with many of the other changes that have gone in over the past release.