kv: add replicas.uninitialized metric #73975

kvoli · 2021-12-17T15:03:05Z

This change adds replicas.unitialized metric, tracking the number of non-init replicas on a store.

Note that this metric doesn't give the exact number of uninitialized replicas because, after a process restart, uninitialized replicas can remain on disk without an in memory representation.

resolves #73368

cockroach-teamcity · 2021-12-17T15:03:15Z

This change is

irfansharif

Sweet turnaround time! And congrats on your first PR! In addition to the comments below, let's squash these changes into a single commit. We have this wiki-page on good commit hygiene that you might find interesting. For here I'd just suggest rewording the title to read something similar to what you have for the PR: "kvserver: add metric for uninitialized replicas".

irfansharif · 2021-12-20T21:36:15Z

pkg/kv/kvserver/store.go

@@ -3011,6 +3012,9 @@ func (s *Store) updateReplicationGauges(ctx context.Context) error {
 		if metrics.Quiescent {
 			quiescentCount++
 		}
+
+		uninitializedCount = int64(len(s.mu.uninitReplicas))


Weird, github reviews don't show where this map is being updated, but I could see it locally. In any case, some notes from when we looked at this offline:

The per-replica visitor above (newStoreReplicaVisitor) also iterates over uninitialized replicas. Instead of tracking+maintaining the set of uninitialized replicas in a separate map (probably more error prone), using the existing replica iteration + checking if the current replica is initialized is I think sufficient.

We'd want a test. We looked at another test to steal from where we could block the reception of a snapshot on a second node, ensure that our uninitialized count is as we'd expect, unblock the replica initialization, and ensure it's back to 0.

pkg/kv/kvserver/metrics.go

irfansharif · 2021-12-20T21:40:21Z

Note on labels: for most of them (mod do-not-merge, X-noremind, etc) we really only use them for filed issues.

irfansharif

Nice test! I only have a few minor suggestions to trim it down and we should be all set. The CI failures also suggest an update to TestChartCatalogMetrics.

pkg/kv/kvserver/client_replica_test.go

pkg/ui/workspaces/cluster-ui/src/store/nodes/nodes.fixtures.ts

kvoli

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @nvanbenschoten)

pkg/kv/kvserver/store.go, line 3016 at r4 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Weird, github reviews don't show where this map is being updated, but I could see it locally. In any case, some notes from when we looked at this offline:

The per-replica visitor above (newStoreReplicaVisitor) also iterates over uninitialized replicas. Instead of tracking+maintaining the set of uninitialized replicas in a separate map (probably more error prone), using the existing replica iteration + checking if the current replica is initialized is I think sufficient.

We'd want a test. We looked at another test to steal from where we could block the reception of a snapshot on a second node, ensure that our uninitialized count is as we'd expect, unblock the replica initialization, and ensure it's back to 0.

Done. We resolved to use the original method and reuse the test.

pkg/kv/kvserver/client_replica_test.go, line 3912 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

I think we can get rid of all usages of stickyEngineRegistry, raftConfig, and stickySpecTestServerArgs. We're not restarting an existing test server and don't need server state to be persisted across the restarts, which is what all this machinery sets out to do. Trying it out locally, all we really need is the following:
	tc := testcluster.StartTestCluster(t, 1, base.TestClusterArgs{
		ReplicationMode: base.ReplicationManual,
		ServerArgs:      base.TestServerArgs{Insecure: true},
	})
And below:
	tc.AddAndStartServer(t, base.TestServerArgs{
		Insecure: true,

Done.

pkg/kv/kvserver/client_replica_test.go, line 3945 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

While we technically can split out a separate range using a tenant prefix, we can do something more direct:
	scratchKey := tc.ScratchRange(t)
	_, repl := getFirstStoreReplica(t, tc.Server(0), scratchKey)

Done.

pkg/kv/kvserver/client_replica_test.go, line 3949 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Since there's only one sub-test, let's get rid of this t.Run outdent and make it part of the main test itself. We typically use sub-tests when testing minor alterations of a single test run (perhaps with shared setup), which isn't quite the case here.

Done.

pkg/kv/kvserver/client_replica_test.go, line 3960 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Given these are unused, s/request_type/_ and s/strings/_.

Done.

pkg/kv/kvserver/client_replica_test.go, line 3997 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

s/iota/0

Done.

pkg/kv/kvserver/client_replica_test.go, line 4001 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

[nit] Inline things here and below:

require.NoError(t, uninitStore.ComputeMetrics(ctx, 0))
// ...
require.Equal(t, int64(1), uninitStore.Metrics().UninitializedCount.Value())
// ...
require.NoError(t, uninitStore.ComputeMetrics(ctx, 0))
// ...
require.Equal(t, int64(0), uninitStore.Metrics().UninitializedCount.Value()) // now initialized

Done.

pkg/ui/workspaces/cluster-ui/src/store/nodes/nodes.fixtures.ts, line 891 at r5 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Is this diff still relevant given we're not adding it to the cluster UI? (anything in addition to the advanced debug page i.e.)

You are right, I thought it might be necessary to declare a default value. I tested without this line and it still shows for advanced debugging. Done.

kvoli

@irfansharif thanks for the comments. I've adopted your suggestions.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @nvanbenschoten)

irfansharif

Sweet, LGTM! For this kind of test I'd typically stress it locally to shake out any flakes, but because it's a new test our CI should already do that for you. For posterity:

$ dev test pkg/kv/kvserver -f=TestUninitializedMetric --stress
INFO: Found 1 test target
16 runs so far, 0 failures, over 5s
59 runs so far, 0 failures, over 10s
95 runs so far, 0 failures, over 15s
125 runs so far, 0 failures, over 20s
...

Had to first rebase on master to pick up #73982. Could also use make stress PKG=./pkg/kv/kvserver TESTS=TestUninitializedMetric.

irfansharif · 2021-12-22T15:07:06Z

pkg/kv/kvserver/client_replica_test.go

@@ -3901,6 +3901,89 @@ func TestTenantID(t *testing.T) {

 }

+//  TestUninitializedMetric


// TestUninitializedMetric ensures that uninitialized replicas are reflected in the UninitializedCount store metric.

irfansharif · 2021-12-22T15:17:06Z

pkg/kv/kvserver/client_replica_test.go

+	forceMetricTick++
+
+	require.NoError(t, <-addReplicaErr)
+	require.NoError(t, targetStore.ComputeMetrics(ctx, forceMetricTick))


Btw, instead of maintaining the forceMetricCounter, just passing 0 directly in ComputeMetrics also works (looking at other usages that's what we seem to do).

nvanbenschoten

Reviewed 3 of 5 files at r5, 1 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif and @kvoli)

-- commits, line 5 at r7:
You'll want to link this to #73368 here and the in PR description. You have "Addresses" in the PR description, but unfortunately, that is not one of the supported keywords. See https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

pkg/kv/kvserver/metrics.go, line 73 at r4 (raw file):

Previously, kvoli (Austen) wrote…

This is from: #73368. @nvanbenschoten mentions

uninitialized replicas can lie dormant in persistent state without any corresponding in-memory representation

Right, there's a small amount of state (typically just the HardState) that can be persisted by an uninitialized replica. We don't look for this on process startup and don't instantiate a Replica object for these unless that replica hears a new message. Undercounting here is mostly fine, as this persisted state is tiny and we're mostly interested in the cost of these in-memory Replica objects.

There's some discussion about this in here and here.

pkg/kv/kvserver/metrics.go, line 1249 at r7 (raw file):

	RaftLeaderNotLeaseHolderCount *metric.Gauge
	LeaseHolderCount              *metric.Gauge
	UninitializedCount            *metric.Gauge

nit: keep the ordering consistent. So move this below QuiescentCount.

pkg/kv/kvserver/store.go, line 3016 at r4 (raw file):

Previously, kvoli (Austen) wrote…

Done. We resolved to use the original method and reuse the test.

An alternative to this approach is to add a new WithUninitialized() method (similar to InOrder) to storeReplicaVisitor which avoids filtering out uninitialized replicas in storeReplicaVisitor.Visit. We could then add a small branch at the top of this visitor function that checks if the current replica is uninitialized ((*Replica).IsInitialized). If it is, increment uninitializedCount and return true.

But what we have here works too.

pkg/kv/kvserver/store.go, line 3019 at r7 (raw file):

			quiescentCount++
		}

nit: now that you've addressed Irfan's comment, there's a stray line here.

kvoli

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @irfansharif, @kvoli, and @nvanbenschoten)

-- commits, line 5 at r7:

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

You'll want to link this to #73368 here and the in PR description. You have "Addresses" in the PR description, but unfortunately, that is not one of the supported keywords. See https://docs.github.com/en/issues/tracking-your-work-with-issues/linking-a-pull-request-to-an-issue#linking-a-pull-request-to-an-issue-using-a-keyword.

Updated with a supported keyword.

pkg/kv/kvserver/metrics.go, line 73 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

Right, there's a small amount of state (typically just the HardState) that can be persisted by an uninitialized replica. We don't look for this on process startup and don't instantiate a Replica object for these unless that replica hears a new message. Undercounting here is mostly fine, as this persisted state is tiny and we're mostly interested in the cost of these in-memory Replica objects.

There's some discussion about this in here and here.

Noted, thanks for the context.

pkg/kv/kvserver/metrics.go, line 1249 at r7 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

nit: keep the ordering consistent. So move this below QuiescentCount.

Done.

pkg/kv/kvserver/store.go, line 3016 at r4 (raw file):

Previously, nvanbenschoten (Nathan VanBenschoten) wrote…

An alternative to this approach is to add a new WithUninitialized() method (similar to InOrder) to storeReplicaVisitor which avoids filtering out uninitialized replicas in storeReplicaVisitor.Visit. We could then add a small branch at the top of this visitor function that checks if the current replica is uninitialized ((*Replica).IsInitialized). If it is, increment uninitializedCount and return true.

But what we have here works too.

So it is being filtered out here. I wasn't certain. I'll keep it as is atm, since I don't know what else it might be necessary for yet.

pkg/kv/kvserver/client_replica_test.go, line 3904 at r6 (raw file):

Previously, irfansharif (irfan sharif) wrote…

// TestUninitializedMetric ensures that uninitialized replicas are reflected in the UninitializedCount store metric.

Done.

pkg/kv/kvserver/client_replica_test.go, line 3981 at r6 (raw file):

Previously, irfansharif (irfan sharif) wrote…

Btw, instead of maintaining the forceMetricCounter, just passing 0 directly in ComputeMetrics also works (looking at other usages that's what we seem to do).

Adopted this change.

This change adds a metric `replica.uninitialized` counting the number of uninitialized replicas within a store. resolves cockroachdb#73368 Release note (ops change): Added a metric `replica.uninitialized` which tracks the number of `Uninitialized` replicas in a store.

kvoli · 2021-12-22T19:04:28Z

bors r+

craig · 2021-12-22T19:33:52Z

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

craig · 2021-12-22T20:36:47Z

Build succeeded:

GitHub CI (Cockroach)

kvoli self-assigned this Dec 17, 2021

kvoli requested a review from irfansharif December 17, 2021 15:55

irfansharif reviewed Dec 20, 2021

View reviewed changes

kvoli force-pushed the replicas-uninit-metrics branch from 413b9ff to 82f95fb Compare December 21, 2021 14:26

blathers-crl bot requested a review from irfansharif December 21, 2021 14:26

kvoli force-pushed the replicas-uninit-metrics branch 5 times, most recently from c5d9d32 to 78acdf2 Compare December 21, 2021 22:46

irfansharif reviewed Dec 21, 2021

View reviewed changes

irfansharif marked this pull request as ready for review December 21, 2021 23:16

irfansharif requested a review from a team as a code owner December 21, 2021 23:16

kvoli force-pushed the replicas-uninit-metrics branch from 78acdf2 to 6fb857f Compare December 22, 2021 14:48

blathers-crl bot requested a review from irfansharif December 22, 2021 14:48

kvoli force-pushed the replicas-uninit-metrics branch from 6fb857f to e72f50f Compare December 22, 2021 14:59

kvoli commented Dec 22, 2021

View reviewed changes

kvoli linked an issue Dec 22, 2021 that may be closed by this pull request

kv: add replicas.uninitialized metric #73368

Closed

kvoli removed the do-not-merge bors won't merge a PR with this label. label Dec 22, 2021

irfansharif approved these changes Dec 22, 2021

View reviewed changes

kvoli force-pushed the replicas-uninit-metrics branch from e72f50f to 24bb005 Compare December 22, 2021 15:54

nvanbenschoten reviewed Dec 22, 2021

View reviewed changes

kvoli force-pushed the replicas-uninit-metrics branch 2 times, most recently from 1acc811 to b51b14c Compare December 22, 2021 17:03

kvoli commented Dec 22, 2021

View reviewed changes

kvoli force-pushed the replicas-uninit-metrics branch 2 times, most recently from 9f2d01d to db1e944 Compare December 22, 2021 17:26

kvoli force-pushed the replicas-uninit-metrics branch from db1e944 to 2e2ecd9 Compare December 22, 2021 17:49

craig bot merged commit 0dcac14 into cockroachdb:master Dec 22, 2021

cockroach-teamcity mentioned this pull request Dec 22, 2021

kv: add replicas.uninitialized metric cockroachdb/docs#12649

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kv: add replicas.uninitialized metric #73975

kv: add replicas.uninitialized metric #73975

kvoli commented Dec 17, 2021 •

edited

Loading

cockroach-teamcity commented Dec 17, 2021

irfansharif left a comment •

edited

Loading

irfansharif Dec 20, 2021

irfansharif commented Dec 20, 2021

irfansharif left a comment •

edited

Loading

kvoli left a comment

kvoli left a comment

irfansharif left a comment

irfansharif Dec 22, 2021

irfansharif Dec 22, 2021

nvanbenschoten left a comment

kvoli left a comment

kvoli commented Dec 22, 2021

craig bot commented Dec 22, 2021

craig bot commented Dec 22, 2021

		@@ -3901,6 +3901,89 @@ func TestTenantID(t *testing.T) {

		}

		// TestUninitializedMetric

kv: add replicas.uninitialized metric #73975

kv: add replicas.uninitialized metric #73975

Conversation

kvoli commented Dec 17, 2021 • edited Loading

cockroach-teamcity commented Dec 17, 2021

irfansharif left a comment • edited Loading

Choose a reason for hiding this comment

irfansharif Dec 20, 2021

Choose a reason for hiding this comment

irfansharif commented Dec 20, 2021

irfansharif left a comment • edited Loading

Choose a reason for hiding this comment

kvoli left a comment

Choose a reason for hiding this comment

kvoli left a comment

Choose a reason for hiding this comment

irfansharif left a comment

Choose a reason for hiding this comment

irfansharif Dec 22, 2021

Choose a reason for hiding this comment

irfansharif Dec 22, 2021

Choose a reason for hiding this comment

nvanbenschoten left a comment

Choose a reason for hiding this comment

kvoli left a comment

Choose a reason for hiding this comment

kvoli commented Dec 22, 2021

craig bot commented Dec 22, 2021

craig bot commented Dec 22, 2021

kvoli commented Dec 17, 2021 •

edited

Loading

irfansharif left a comment •

edited

Loading

irfansharif left a comment •

edited

Loading