Reviewed 1 of 1 files at r1, 7 of 7 files at r2, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @wenyihu6)

-- commits line 29 at r2:
Could you mention the locality-tier keys which are recognized as a "zone" here.

-- commits line 35 at r2:
Could we shorten the release note and move some of this up above.

I think it suffices to mention the supported zone locality-tier keys and then the requirement that there be at most one and consistent across nodes. The example is good!

pkg/kv/kvserver/store_snapshot.go line 977 at r2 (raw file):

// shouldIncrementCrossLocalitySnapshotMetrics returns (bool, bool) - indicating
// if the two given replicas are cross-region and cross-zone respectively.
func (s *Store) shouldIncrementCrossLocalitySnapshotMetrics(

nit: It may be worth mentioning why this function deals with both x-region and x-zone are handled in the same method, perf - so that in the future someone doesn't increase the overhead because they were unaware.

pkg/roachpb/metadata.go line 622 at r2 (raw file):

// getFirstRegionFirstZone iterates through the locality tiers and returns
// multiple values containing:
// 1.The value of the first encountered "region" tier.

nit: add some spacing between "1.The..."

pkg/roachpb/metadata.go line 628 at r2 (raw file):

func (l Locality) getFirstRegionFirstZone() (string, bool, string, string, bool) {
	hasRegion, hasZone := false, false
	firstRegionValue, firstZoneKey, firstZoneValue := "", "", ""

nit: probably more idiomatic to do something like

var ( 
    hasRegion, hasZone bool
    firstRegionValue, firstZoneKey, firstZoneValue string
)

Or get that for free if you name the function return values like

func (l Locality) getFirstRegionFirstZone()
(firstRegion string, hasRegion bool, ...) {

pkg/roachpb/metadata.go line 654 at r2 (raw file):

// 2. Error indicating if either locality does not have a "region" tier key.
// 3. A boolean value indicating if this and the provided locality are
// cross-region.

cross-zone*

pkg/roachpb/metadata.go line 667 at r2 (raw file):

// be checked thoroughly for duplicate zone tier keys and key mismatches.
// However, due to frequent invocation of this function, we prefer to terminate
// the check after examining the first encountered zone tier key-value pairs.

Just checking my understanding. Ignore possibility of any invalid locality keys, so they're all either region or zone.

Is the intention for x-region to imply x-zone? i.e. should it ever be possible to have two localities which are x-region but not x-zone?

When you want to estimate out how many bytes (roughly) you are paying for at x-region rates (excluding cheaper x-zone), you would do sum(x-region) - sum(x-zone)?

If so, do we need to additionally assume clusters won't have a mix of locality=region=...,zone=..., locality=region=... and locality=zone=...?

For example

L1: region=us-east, zone=us-east-1
L2:                 zone=us-east-2
L3: region=us-west

L1 x L2 = x-zone
L1 x L3 = x-region
L2 x L3 = none

Presume 3 snaps/requests each with value 1 for each of the Li x Lj enumerated above. Thesum(x-region)-sum(x-zone) metric query would return 0 despite there really being 1 x-region and 1 x-zone.

I know we previously chatted about the x-zone condition requiring a region flag also be set, what are your thoughts on this?

pkg/roachpb/metadata.go line 668 at r2 (raw file):

// However, due to frequent invocation of this function, we prefer to terminate
// the check after examining the first encountered zone tier key-value pairs.
func (l Locality) IsCrossRegionCrossZone(other Locality) (bool, error, bool, error) {

nit: Consider naming the return values.

pkg/roachpb/metadata_test.go line 224 at r2 (raw file):

}

func TestLocalityIsCrossRegionCrossZone(t *testing.T) {

Great test!

with some small suggestions.

Reviewed 4 of 6 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @wenyihu6)

-- commits line 19 at r4:
nit: github will pickup the link fine with just #104111. No need for markdown.

-- commits line 33 at r4:
nit: Are these commit messages formatted to have correct line length?

pkg/kv/kvserver/store_snapshot.go line 1004 at r4 (raw file):

	isCrossRegion, isCrossZone := s.shouldIncrementCrossLocalitySnapshotMetrics(ctx, firstReplica, secReplica)
	switch sentOrReceived {
	case "sent":

Should this instead be a bool? It would be better to be as strict as possible.