kvserver: add x-region, x-zone Raft msg metrics to Store #105122

wenyihu6 · 2023-06-18T08:05:34Z

Previously, there were no metrics to observe cross-region, cross-zone traffic in
raft messages requests sent / received at each store.

To improve this issue, this commit adds six new store metrics -

"raft.rcvd.bytes"
"raft.sent.bytes"
"raft.rcvd.cross_region.bytes"
"raft.sent.cross_region.bytes"
"raft.rcvd.cross_zone.bytes"
"raft.sent.cross_zone.bytes"

The first two metrics track the total byte of raft messages received and sent in
a store. Additionally, there are four metrics to track the aggregate byte count
of cross-region, cross-zone Raft messages sent and received in the store.

Note that these metrics capture the byte count of requests immediately upon
message reception and just prior to message transmission. In the case of
messages containing heartbeats or heartbeat_resps, they capture the byte count
of requests with coalesced heartbeats.

To facilitate metrics updating, this commit also introduces a new raft message
handler interface OutgoingRaftMessageHandler. This interface captures outgoing
messages right before they are sent to raftSendQueue. Note that the message
may not be successfully queued if the outgoing queue is full.

Resolves: #103983

Release note (ops change): Six new metrics -
"raft.rcvd.bytes"
"raft.sent.bytes"
"raft.rcvd.cross_region.bytes"
"raft.sent.cross_region.bytes"
"raft.rcvd.cross_zone.bytes"
"raft.sent.cross_zone.bytes" - have now been added for each store.

For accurate metrics, follow these assumptions:

Configure region and zone tier keys consistently across nodes.
Within a node locality, ensure unique region and zone tier keys.
Maintain consistent configuration of region and zone tiers across nodes.

cockroach-teamcity · 2023-06-18T08:05:45Z

This change is

The commit renames `RaftMessageHandler` to `IncomingRaftMessageHandler`. Another PR is introducing a new interface `OutgoingRaftMessageHandler`, dedicated to managing messages sent. The main purpose of this PR is to make the future PR cleaner by handling the renaming process. Note that this commit does not change any existing functionality. Part of: cockroachdb#103983 Related: cockroachdb#105122 Release Note: None

tbg

Looking pretty good (only looked at last commit here)! Some of the same ideas I had in the base PRs apply here as well. It probably makes sense to settle these dependencies first, after that I think this PR will be in pretty good shape automatically.

pkg/kv/kvserver/client_raft_helpers_test.go

pkg/kv/kvserver/client_raft_test.go

pkg/kv/kvserver/store_raft.go

The commit renames `RaftMessageHandler`, `Listen`, and `Stop` to `IncomingRaftMessageHandler`, `ListenIncomingRaftMessages`, and `StopIncomingRaftMessages`. Another PR is introducing a new interface `OutgoingRaftMessageHandler`, dedicated to managing messages sent. The main purpose of this PR is to make the future PR cleaner by handling the renaming process. Note that this commit does not change any existing functionality. Part of: cockroachdb#103983 Related: cockroachdb#105122 Release Note: None

wenyihu6 · 2023-06-25T02:09:57Z

pkg/kv/kvserver/raft_transport.go line 386 at r8 (raw file):

Previously, wenyihu6 (Wenyi Hu) wrote…

I didn’t rename getHandler primarily because I didn’t want to cause too many changes on the existing interface. I was also worried that this would lead to inconsistency. If we decide to rename everything to Incoming, we should probably change Listen, Stop to ListenIncoming, StopIncoming as well. What do you think?

Update: decided to just rename everything and have updated the rename PR

tbg

Looking good, almost there! Main point is trimming down the tests.

pkg/kv/kvserver/raft_transport.go

pkg/kv/kvserver/store_raft.go

pkg/kv/kvserver/client_raft_test.go

pkg/kv/kvserver/metrics.go

105123: kvserver: rename RaftMessageHandler to IncomingRaftMessageHandler r=tbg,kvoli a=wenyihu6 The commit renames `RaftMessageHandler`, `Listen`, and `Stop` to `IncomingRaftMessageHandler`, `ListenIncomingRaftMessages`, and `StopIncomingRaftMessages`. Another PR is introducing a new interface `OutgoingRaftMessageHandler`, dedicated to managing messages sent. The main purpose of this PR is to make the future PR cleaner by handling the renaming process. Note that this commit does not change any existing functionality. Part of: #103983 Related: #105122 Release Note: None 105519: kvserver: fix and re-enable slow lease application logging r=erikgrinaker a=erikgrinaker Resolves #97209. Epic: none Release note: None Co-authored-by: Wenyi <[email protected]> Co-authored-by: Erik Grinaker <[email protected]>

wenyihu6 · 2023-06-27T02:44:18Z

Looking good, almost there! Main point is trimming down the tests.

Ack. Replaced the end to end test with one integration test for handlers and one unit test for cross locality metrics.

pkg/kv/kvserver/store_snapshot.go

pkg/kv/kvserver/client_raft_helpers_test.go

tbg · 2023-06-30T07:55:57Z

pkg/kv/kvserver/client_raft_test.go

+		require.Equal(t, expected, actual)
+	})
+
+	t.Run("sent raft message", func(t *testing.T) {


Subtests need to be independent of each other, I think these ones aren't. So either make this just one test (remove the subtests) or set up the environment anew in each subtest.

Taking it back, they are independent due to the use of diffs. So this is "fine", I still think my top-level comment about merging the tests makes sense - could always have overlooked something.

tbg · 2023-06-30T08:08:15Z

Thanks for the updates! The code looks good, the test could be improved somewhat. In the cross-region specific test, you have a nice tabledriven setup. But then you also have the other test, which tests a few more slices of the stack (in a way that seems to work out really well) but which doesn't have nice tabledriven layout. Would you consider consolidating the two? I'm thinking ditch the non-tabledriven test, moving its coverage into the tabledriven test instead and renaming that test so that it is a bit more general (testing all send/receive metrics that have to do w the raft transport). After that (and my other trivial comments above), this is good to 🚀 !

Approving preemptively.

tbg · 2023-06-30T08:09:07Z

pkg/kv/kvserver/client_raft_test.go

+		require.Equal(t, expected, actual)
+	})
+
+	t.Run("sent raft message", func(t *testing.T) {


Taking it back, they are independent due to the use of diffs. So this is "fine", I still think my top-level comment about merging the tests makes sense - could always have overlooked something.

Previously, there were no metrics to observe cross-region, cross-zone traffic in raft messages requests sent / received at each store. To improve this issue, this commit adds six new store metrics - ``` "raft.rcvd.bytes" "raft.sent.bytes" "raft.rcvd.cross_region.bytes" "raft.sent.cross_region.bytes" "raft.rcvd.cross_zone.bytes" "raft.sent.cross_zone.bytes" ``` The first two metrics track the total byte of raft messages received and sent in a store. Additionally, there are four metrics to track the aggregate byte count of cross-region, cross-zone Raft messages sent and received in the store. Note that these metrics capture the byte count of requests immediately upon message reception and just prior to message transmission. In the case of messages containing heartbeats or heartbeat_resps, they capture the byte count of requests with coalesced heartbeats. To facilitate metrics updating, this commit also introduces a new raft message handler interface `OutgoingRaftMessageHandler`. This interface captures outgoing messages right before they are sent to `raftSendQueue`. Note that the message may not be successfully queued if the outgoing queue is full. Resolves: cockroachdb#103983 Release note (ops change): Six new metrics - "raft.rcvd.bytes" "raft.sent.bytes" "raft.rcvd.cross_region.bytes" "raft.sent.cross_region.bytes" "raft.rcvd.cross_zone.bytes" "raft.sent.cross_zone.bytes" - have now been added for each store. For accurate metrics, follow these assumptions: - Configure region and zone tier keys consistently across nodes. - Within a node locality, ensure unique region and zone tier keys. - Maintain consistent configuration of region and zone tiers across nodes.

wenyihu6 · 2023-06-30T16:43:33Z

Thanks for the updates! The code looks good, the test could be improved somewhat. In the cross-region specific test, you have a nice tabledriven setup. But then you also have the other test, which tests a few more slices of the stack (in a way that seems to work out really well) but which doesn't have nice tabledriven layout. Would you consider consolidating the two? I'm thinking ditch the non-tabledriven test, moving its coverage into the tabledriven test instead and renaming that test so that it is a bit more general (testing all send/receive metrics that have to do w the raft transport). After that (and my other trivial comments above), this is good to 🚀 !

Approving preemptively.

Discussed this over slack -
The two tests couldn't be merged easily since passing in locality info directly to HandleRaftRequest and HandleRaftResponse is not possible. They both require getNodeLocality to check if it is cross region or cross zone. And to obtain this information, we would need to go back to the e2e tests as they just can't the locality info of a node without setting up a cluster iiuc. So we decided to just keep the way it is.

The last push was to rebase on master and cherry pick the last commit. Will merge when CI goes green.

wenyihu6 · 2023-06-30T19:53:16Z

Thanks a lot for the review!

bors r=tbg

craig · 2023-06-30T20:41:27Z

Build succeeded:

Bazel Essential CI (Cockroach)

wenyihu6 mentioned this pull request Jun 18, 2023

kvserver: rename RaftMessageHandler to IncomingRaftMessageHandler #105123

Merged

wenyihu6 force-pushed the outgoing-handler branch 3 times, most recently from 7aa20ec to eb9fe42 Compare June 20, 2023 02:20

wenyihu6 force-pushed the outgoing-handler branch 2 times, most recently from 7383221 to 9f95c9c Compare June 20, 2023 04:47

wenyihu6 force-pushed the outgoing-handler branch from 9f95c9c to 7c1d8ec Compare June 20, 2023 04:50

wenyihu6 marked this pull request as ready for review June 20, 2023 05:09

wenyihu6 requested a review from a team as a code owner June 20, 2023 05:09

wenyihu6 requested a review from a team June 20, 2023 05:09

wenyihu6 requested a review from a team as a code owner June 20, 2023 05:09

wenyihu6 requested a review from tbg June 20, 2023 05:09

tbg requested changes Jun 20, 2023

View reviewed changes

wenyihu6 requested a review from kvoli June 20, 2023 15:07

wenyihu6 force-pushed the outgoing-handler branch 2 times, most recently from b894549 to 026ab0c Compare June 21, 2023 12:28

wenyihu6 requested a review from tbg June 21, 2023 16:51

wenyihu6 force-pushed the outgoing-handler branch from 026ab0c to 5ae357f Compare June 21, 2023 22:34

wenyihu6 force-pushed the outgoing-handler branch from 4a6425c to 900af28 Compare June 25, 2023 02:08

wenyihu6 force-pushed the outgoing-handler branch from 900af28 to 3730490 Compare June 25, 2023 04:23

tbg requested changes Jun 26, 2023

View reviewed changes

wenyihu6 force-pushed the outgoing-handler branch 5 times, most recently from 6c0f4e8 to 40368c6 Compare June 27, 2023 02:29

wenyihu6 requested a review from tbg June 27, 2023 02:39

wenyihu6 force-pushed the outgoing-handler branch from 40368c6 to 20e25ca Compare June 29, 2023 15:16

tbg reviewed Jun 30, 2023

View reviewed changes

tbg approved these changes Jun 30, 2023

View reviewed changes

wenyihu6 force-pushed the outgoing-handler branch from 20e25ca to 5479d92 Compare June 30, 2023 16:39

craig bot merged commit d4dec5e into cockroachdb:master Jun 30, 2023

wenyihu6 deleted the outgoing-handler branch June 30, 2023 21:08

cockroach-teamcity mentioned this pull request Jul 1, 2023

PR #105122 - kvserver: add x-region, x-zone Raft msg metrics to Store cockroachdb/docs#17388

Open

tbg mentioned this pull request Jul 10, 2023

kvserver: add cross-az and cross-region replication latency #106499

Open

pav-kv mentioned this pull request Oct 26, 2023

kvserver: 23.2 replication microbenchmark regressions #111561

Closed

7 tasks

nvanbenschoten mentioned this pull request Nov 14, 2023

kv: add raft.transport.sent-bytes and raft.transport.rcvd-bytes metrics #114424

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kvserver: add x-region, x-zone Raft msg metrics to Store #105122

kvserver: add x-region, x-zone Raft msg metrics to Store #105122

wenyihu6 commented Jun 18, 2023 •

edited

Loading

cockroach-teamcity commented Jun 18, 2023

tbg left a comment

wenyihu6 commented Jun 25, 2023

tbg left a comment

wenyihu6 commented Jun 27, 2023

tbg Jun 30, 2023

tbg Jun 30, 2023

tbg commented Jun 30, 2023 •

edited

Loading

tbg Jun 30, 2023

wenyihu6 commented Jun 30, 2023

wenyihu6 commented Jun 30, 2023

craig bot commented Jun 30, 2023

kvserver: add x-region, x-zone Raft msg metrics to Store #105122

kvserver: add x-region, x-zone Raft msg metrics to Store #105122

Conversation

wenyihu6 commented Jun 18, 2023 • edited Loading

cockroach-teamcity commented Jun 18, 2023

tbg left a comment

Choose a reason for hiding this comment

wenyihu6 commented Jun 25, 2023

tbg left a comment

Choose a reason for hiding this comment

wenyihu6 commented Jun 27, 2023

tbg Jun 30, 2023

Choose a reason for hiding this comment

tbg Jun 30, 2023

Choose a reason for hiding this comment

tbg commented Jun 30, 2023 • edited Loading

tbg Jun 30, 2023

Choose a reason for hiding this comment

wenyihu6 commented Jun 30, 2023

wenyihu6 commented Jun 30, 2023

craig bot commented Jun 30, 2023

wenyihu6 commented Jun 18, 2023 •

edited

Loading

tbg commented Jun 30, 2023 •

edited

Loading