diff --git a/pkg/kv/kvclient/kvcoord/dist_sender.go b/pkg/kv/kvclient/kvcoord/dist_sender.go index f243945bcc04..e962a94fbc71 100644 --- a/pkg/kv/kvclient/kvcoord/dist_sender.go +++ b/pkg/kv/kvclient/kvcoord/dist_sender.go @@ -2296,12 +2296,12 @@ func (ds *DistSender) sendToReplicas( if ds.BatchRequestInterceptor != nil { ds.BatchRequestInterceptor(ba) } - shouldIncCrossRegion, shouldIncCrossZone := ds.checkAndUpdateBatchRequestMetrics(ctx, ba) + comparisonResult := ds.checkAndUpdateCrossLocalityBatchMetrics(ctx, ba) br, err = transport.SendNext(ctx, ba) if ds.BatchResponseInterceptor != nil { ds.BatchResponseInterceptor(br) } - ds.checkAndUpdateBatchResponseMetrics(br, shouldIncCrossRegion, shouldIncCrossZone) + ds.updateCrossLocalityBatchMetrics(br, comparisonResult) ds.maybeIncrementErrCounters(br, err) if err != nil { @@ -2557,71 +2557,67 @@ func (ds *DistSender) sendToReplicas( } } -// isCrossRegionCrossZoneBatch returns (bool, bool) - indicating if the given -// batch request is cross-region and cross-zone respectively. -func (ds *DistSender) isCrossRegionCrossZoneBatch( +// getCrossLocalityComparison compares the localities of the current node and +// the destination range node to determine if the given batch request is +// cross-region and cross-zone. +func (ds *DistSender) getCrossLocalityComparison( ctx context.Context, ba *kvpb.BatchRequest, -) (bool, bool) { +) roachpb.LocalityComparisonType { gatewayNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(ba.GatewayNodeID) if err != nil { log.VEventf(ctx, 2, "failed to perform look up for node descriptor %s", err) - return false, false + return roachpb.LocalityComparisonType_UNDEFINED } destinationNodeDesc, err := ds.nodeDescs.GetNodeDescriptor(ba.Replica.NodeID) if err != nil { log.VEventf(ctx, 2, "failed to perform look up for node descriptor %s", err) - return false, false + return roachpb.LocalityComparisonType_UNDEFINED } - isCrossRegion, regionErr, isCrossZone, zoneErr := gatewayNodeDesc.Locality.IsCrossRegionCrossZone(destinationNodeDesc.Locality) + + comparisonResult, regionErr, zoneErr := gatewayNodeDesc.Locality.CompareWithLocality(destinationNodeDesc.Locality) if regionErr != nil { - log.VEventf(ctx, 2, "%v", regionErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross region %v", regionErr) } if zoneErr != nil { - log.VEventf(ctx, 2, "%v", zoneErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross zone %v", zoneErr) } - return isCrossRegion, isCrossZone + return comparisonResult } -// checkAndUpdateBatchRequestMetrics updates the batch requests metrics in a -// more meaningful way. Cross-region metrics monitor activities across different -// regions. Cross-zone metrics monitor cross-zone activities within the same -// region or in cases where region tiers are not configured. The check result is -// returned here to avoid redundant check for metrics updates after receiving -// batch responses. -func (ds *DistSender) checkAndUpdateBatchRequestMetrics( +// checkAndUpdateCrossLocalityBatchMetrics updates the batch requests metrics in +// a more meaningful way. Cross-region metrics monitor activities across +// different regions. Cross-zone metrics monitor cross-zone activities within +// the same region or in cases where region tiers are not configured. The +// locality comparison result is returned here to avoid redundant check for +// metrics updates after receiving batch responses. +func (ds *DistSender) checkAndUpdateCrossLocalityBatchMetrics( ctx context.Context, ba *kvpb.BatchRequest, -) (shouldIncCrossRegion bool, shouldIncCrossZone bool) { +) roachpb.LocalityComparisonType { ds.metrics.ReplicaAddressedBatchRequestBytes.Inc(int64(ba.Size())) - isCrossRegion, isCrossZone := ds.isCrossRegionCrossZoneBatch(ctx, ba) - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - ds.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossRegion = true - } - } else { - if isCrossZone { - ds.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossZone = true - } - } - return shouldIncCrossRegion, shouldIncCrossZone + comparisonResult := ds.getCrossLocalityComparison(ctx, ba) + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + ds.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + ds.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. + } + return comparisonResult } -// checkAndUpdateBatchResponseMetrics updates the batch response metrics based -// on the shouldIncCrossRegion and shouldIncCrossZone parameters. These -// parameters are determined during the initial check for batch requests. The -// underlying assumption is that if requests were cross-region or cross-zone, -// the response should be as well. -func (ds *DistSender) checkAndUpdateBatchResponseMetrics( - br *kvpb.BatchResponse, shouldIncCrossRegion bool, shouldIncCrossZone bool, +// updateCrossLocalityBatchMetrics updates the batch response metrics based on +// the comparisonResult parameter determined during the initial batch requests +// check. The underlying assumption is that the response should match the +// cross-region or cross-zone nature of the request. +func (ds *DistSender) updateCrossLocalityBatchMetrics( + br *kvpb.BatchResponse, comparisonResult roachpb.LocalityComparisonType, ) { ds.metrics.ReplicaAddressedBatchResponseBytes.Inc(int64(br.Size())) - if shouldIncCrossRegion { + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: ds.metrics.CrossRegionBatchResponseBytes.Inc(int64(br.Size())) - } - if shouldIncCrossZone { + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: ds.metrics.CrossZoneBatchResponseBytes.Inc(int64(br.Size())) } } diff --git a/pkg/kv/kvserver/allocator/storepool/store_pool.go b/pkg/kv/kvserver/allocator/storepool/store_pool.go index 286bcce4e18a..17a8d405e449 100644 --- a/pkg/kv/kvserver/allocator/storepool/store_pool.go +++ b/pkg/kv/kvserver/allocator/storepool/store_pool.go @@ -1344,22 +1344,11 @@ func (sp *StorePool) GetNodeLocalityString(nodeID roachpb.NodeID) string { return sp.getNodeLocalityWithString(nodeID).str } -// getNodeLocality returns the locality information for the given node. -func (sp *StorePool) getNodeLocality(nodeID roachpb.NodeID) roachpb.Locality { +// GetNodeLocality returns the locality information for the given node. +func (sp *StorePool) GetNodeLocality(nodeID roachpb.NodeID) roachpb.Locality { return sp.getNodeLocalityWithString(nodeID).locality } -// IsCrossRegionCrossZone takes in two replicas and compares the locality of -// them based on their replica node IDs. It returns (bool, error, bool, error) -// where the boolean values indicate whether the two replicas' nodes are in -// different regions, different zones, along with any lookup errors. -func (sp *StorePool) IsCrossRegionCrossZone( - firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor, -) (bool, error, bool, error) { - return sp.getNodeLocality(firstReplica.NodeID).IsCrossRegionCrossZone( - sp.getNodeLocality(secReplica.NodeID)) -} - // IsStoreReadyForRoutineReplicaTransfer returns true iff the store's node is // live (as indicated by its `NodeLivenessStatus`) and thus a legal candidate // to receive a replica. diff --git a/pkg/kv/kvserver/replica_command.go b/pkg/kv/kvserver/replica_command.go index 8ba1d7f34ff9..af2b6fef7a73 100644 --- a/pkg/kv/kvserver/replica_command.go +++ b/pkg/kv/kvserver/replica_command.go @@ -3152,7 +3152,7 @@ func (r *Replica) followerSendSnapshot( r.store.metrics.DelegateSnapshotSendBytes.Inc(inc) } r.store.metrics.RangeSnapshotSentBytes.Inc(inc) - r.store.updateCrossLocalitySnapshotMetrics( + r.store.checkAndUpdateCrossLocalitySnapshotMetrics( ctx, req.CoordinatorReplica, req.RecipientReplica, inc, true /* isSent */) switch header.Priority { diff --git a/pkg/kv/kvserver/store_snapshot.go b/pkg/kv/kvserver/store_snapshot.go index f8bce0e375e6..d2ac642b3418 100644 --- a/pkg/kv/kvserver/store_snapshot.go +++ b/pkg/kv/kvserver/store_snapshot.go @@ -973,58 +973,52 @@ func (s *Store) checkSnapshotOverlapLocked( return nil } -// shouldIncrementCrossLocalitySnapshotMetrics returns (bool, bool) - indicating -// if the two given replicas are cross-region and cross-zone respectively. -func (s *Store) shouldIncrementCrossLocalitySnapshotMetrics( +// getCrossLocalityComparison compares the localities of the two given replicas. +func (s *Store) getCrossLocalityComparison( ctx context.Context, firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor, -) (bool, bool) { - isCrossRegion, regionErr, isCrossZone, zoneErr := s.cfg.StorePool.IsCrossRegionCrossZone( - firstReplica, secReplica) +) roachpb.LocalityComparisonType { + firstLocality := s.cfg.StorePool.GetNodeLocality(firstReplica.NodeID) + secLocality := s.cfg.StorePool.GetNodeLocality(secReplica.NodeID) + comparisonResult, regionErr, zoneErr := firstLocality.CompareWithLocality(secLocality) if regionErr != nil { log.VEventf(ctx, 2, "unable to determine if snapshot is cross region %v", regionErr) } if zoneErr != nil { log.VEventf(ctx, 2, "unable to determine if snapshot is cross zone %v", zoneErr) } - return isCrossRegion, isCrossZone + return comparisonResult } -// updateCrossLocalitySnapshotMetrics updates the snapshot metrics in a more -// meaningful way. Cross-region metrics monitor activities across different -// regions. Cross-zone metrics monitor any cross-zone activities within the same -// region or if the region tiers are not configured. -func (s *Store) updateCrossLocalitySnapshotMetrics( +// checkAndUpdateCrossLocalitySnapshotMetrics updates the snapshot metrics in +// a more meaningful way. Cross-region metrics monitor activities across +// different regions. Cross-zone metrics monitor cross-zone activities within +// the same region or in cases where region tiers are not configured. +func (s *Store) checkAndUpdateCrossLocalitySnapshotMetrics( ctx context.Context, firstReplica roachpb.ReplicaDescriptor, secReplica roachpb.ReplicaDescriptor, inc int64, isSent bool, ) { - isCrossRegion, isCrossZone := s.shouldIncrementCrossLocalitySnapshotMetrics(ctx, firstReplica, secReplica) + comparisonResult := s.getCrossLocalityComparison(ctx, firstReplica, secReplica) if isSent { - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - s.metrics.RangeSnapShotCrossRegionSentBytes.Inc(inc) - } - } else { - if isCrossZone { - s.metrics.RangeSnapShotCrossZoneSentBytes.Inc(inc) - } + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + s.metrics.RangeSnapShotCrossRegionSentBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + s.metrics.RangeSnapShotCrossZoneSentBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. } } else { // isReceived - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - s.metrics.RangeSnapShotCrossRegionRcvdBytes.Inc(inc) - } - } else { - if isCrossZone { - s.metrics.RangeSnapShotCrossZoneRcvdBytes.Inc(inc) - } + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + s.metrics.RangeSnapShotCrossRegionRcvdBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + s.metrics.RangeSnapShotCrossZoneRcvdBytes.Inc(inc) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. } } } @@ -1145,7 +1139,7 @@ func (s *Store) receiveSnapshot( recordBytesReceived := func(inc int64) { s.metrics.RangeSnapshotRcvdBytes.Inc(inc) - s.updateCrossLocalitySnapshotMetrics( + s.checkAndUpdateCrossLocalitySnapshotMetrics( ctx, header.RaftMessageRequest.FromReplica, header.RaftMessageRequest.ToReplica, inc, false /* isSent */) switch header.Priority { diff --git a/pkg/roachpb/metadata.go b/pkg/roachpb/metadata.go index ee56469552e1..91d2b1427c19 100644 --- a/pkg/roachpb/metadata.go +++ b/pkg/roachpb/metadata.go @@ -675,14 +675,11 @@ func (l Locality) getFirstRegionFirstZone() ( return firstRegionValue, hasRegion, firstZoneKey, firstZoneValue, hasZone } -// IsCrossRegionCrossZone returns multiple values containing: -// 1. A boolean value indicating if this and the provided locality are -// cross-region. -// 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-zone. -// 4. Error indicating if either locality does not have a "zone" tier key or if -// the first "zone" tier keys used by two localities are different. +// CompareWithLocality returns the comparison result between this and the +// provided other locality along with any lookup errors. Possible errors include +// 1. if either locality does not have a "region" tier key. 2. if either +// locality does not have a "zone" tier key or if the first "zone" tier keys +// used by two localities are different. // // Limitation: // - It is unfortunate that the tier key is hardcoded here. Ideally, we would @@ -699,14 +696,14 @@ func (l Locality) getFirstRegionFirstZone() ( // a single function to avoid overhead. If you are adding additional locality // tiers comparisons, it is recommended to handle them within one tier list // iteration. -func (l Locality) IsCrossRegionCrossZone( +func (l Locality) CompareWithLocality( other Locality, -) (isCrossRegion bool, regionErr error, isCrossZone bool, zoneErr error) { +) (_ LocalityComparisonType, regionErr error, zoneErr error) { firstRegionValue, hasRegion, firstZoneKey, firstZone, hasZone := l.getFirstRegionFirstZone() firstRegionValueOther, hasRegionOther, firstZoneKeyOther, firstZoneOther, hasZoneOther := other.getFirstRegionFirstZone() - isCrossRegion = firstRegionValue != firstRegionValueOther - isCrossZone = firstZone != firstZoneOther + isCrossRegion := firstRegionValue != firstRegionValueOther + isCrossZone := firstZone != firstZoneOther if !hasRegion || !hasRegionOther { isCrossRegion = false @@ -718,7 +715,15 @@ func (l Locality) IsCrossRegionCrossZone( zoneErr = errors.Errorf("localities must have a valid zone tier key for cross-zone comparison") } - return isCrossRegion, regionErr, isCrossZone, zoneErr + if isCrossRegion { + return LocalityComparisonType_CROSS_REGION, regionErr, zoneErr + } else { + if isCrossZone { + return LocalityComparisonType_SAME_REGION_CROSS_ZONE, regionErr, zoneErr + } else { + return LocalityComparisonType_SAME_REGION_SAME_ZONE, regionErr, zoneErr + } + } } // SharedPrefix returns the number of this locality's tiers which match those of diff --git a/pkg/roachpb/metadata.proto b/pkg/roachpb/metadata.proto index 01aebf22f1a7..c8cbfc7824f5 100644 --- a/pkg/roachpb/metadata.proto +++ b/pkg/roachpb/metadata.proto @@ -434,6 +434,22 @@ message Tier { optional string value = 2 [(gogoproto.nullable) = false]; } +// LocalityComparisonType represents different types of comparison results that +// indicate the relationship between two localities. +enum LocalityComparisonType { + // CROSS_REGION indicates that the two localities have different region tiers. + CROSS_REGION = 0; + // SAME_REGION_CROSS_ZONE indicates that the two localities have the same + // region tiers but different zone tiers. + SAME_REGION_CROSS_ZONE = 1; + // SAME_REGION_SAME_ZONE indicates that the two localities have same region + // and same zone tiers. + SAME_REGION_SAME_ZONE = 2; + // UNDEFINED represents an undefined comparison result, indicating error + // behavior. + UNDEFINED = 3; +} + message Version { option (gogoproto.equal) = true; option (gogoproto.goproto_stringer) = false; diff --git a/pkg/roachpb/metadata_test.go b/pkg/roachpb/metadata_test.go index de8940fbfaed..cc95cff2c4fa 100644 --- a/pkg/roachpb/metadata_test.go +++ b/pkg/roachpb/metadata_test.go @@ -221,7 +221,7 @@ func TestLocalityMatches(t *testing.T) { } } -func TestLocalityIsCrossRegionCrossZone(t *testing.T) { +func TestLocalityCompareWithLocality(t *testing.T) { regionErrStr := "localities must have a valid region tier key for cross-region comparison" zoneErrStr := "localities must have a valid zone tier key for cross-zone comparison" @@ -251,82 +251,81 @@ func TestLocalityIsCrossRegionCrossZone(t *testing.T) { for _, tc := range []struct { l string other string - isCrossRegion bool - isCrossZone bool + localityType LocalityComparisonType crossRegionErr string crossZoneErr string }{ // -------- Part 1: check for different zone tier alternatives -------- // Valid tier keys, same regions and same zones. {l: "region=us-west,zone=us-west-1", other: "region=us-west,zone=us-west-1", - isCrossRegion: false, isCrossZone: false, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: "", crossZoneErr: ""}, // Valid tier keys, different regions and different zones. {l: "region=us-west,zone=us-west-1", other: "region=us-east,zone=us-west-2", - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // Valid tier keys, different regions and different zones. {l: "region=us-west,availability-zone=us-west-1", other: "region=us-east,availability-zone=us-east-1", - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // Valid tier keys, same regions and different zones. {l: "region=us-west,az=us-west-1", other: "region=us-west,other-keys=us,az=us-east-1", - isCrossRegion: false, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: "", crossZoneErr: ""}, // Invalid zone tier key and different regions. {l: "region=us-west,availability-zone=us-west-1", other: "region=us-east,zone=us-east-1", - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // Valid zone tier key (edge case), different zones and regions. {l: "region=us-west,zone=us-west-1", other: "region=us-east,zone=us-west-2,az=us-west-1", - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // Missing zone tier key and different regions. {l: "region=us-west,zone=us-west-1", other: "region=us-east", - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // Different region and different zones with non-unique & invalid zone tier key. {l: "region=us-west,zone=us-west-1,az=us-west-2", other: "az=us-west-1,region=us-west,zone=us-west-1", - isCrossRegion: false, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: "", crossZoneErr: zoneErrStr}, // Different regions and different zones with non-unique & valid zone tier key. {l: "region=us-west,az=us-west-2,zone=us-west-1", other: "region=us-west,az=us-west-1", - isCrossRegion: false, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: "", crossZoneErr: ""}, // Invalid region tier key and different zones. {l: "country=us,zone=us-west-1", other: "country=us,zone=us-west-2", - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // Missing region tier key and different zones. {l: "az=us-west-1", other: "region=us-east,az=us-west-2", - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // Invalid region and zone tier key. {l: "invalid-key=us-west,zone=us-west-1", other: "region=us-east,invalid-key=us-west-1", - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // Invalid region and zone tier key. {l: "country=us,dc=us-west-2", other: "country=us,dc=us-west-2", - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // -------- Part 2: single region, single zone -------- // One: (both) Two: (region) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr(secRegionStr, ""), - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // One: (both) Two: (zone) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr("", secZoneStr), - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // One: (region) Two: (region) {l: makeLocalityStr(firstRegionStr, ""), other: makeLocalityStr(secRegionStr, ""), - isCrossRegion: true, isCrossZone: false, crossRegionErr: "", crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: zoneErrStr}, // One: (zone) Two: (zone) {l: makeLocalityStr("", firstZoneStr), other: makeLocalityStr("", secZoneStr), - isCrossRegion: false, isCrossZone: true, crossRegionErr: regionErrStr, crossZoneErr: ""}, + localityType: LocalityComparisonType_SAME_REGION_CROSS_ZONE, crossRegionErr: regionErrStr, crossZoneErr: ""}, // One: (region) Two: (zone) {l: makeLocalityStr(firstRegionStr, ""), other: makeLocalityStr("", secZoneStr), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (both) Two: (both) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr(secRegionStr, secZoneStr), - isCrossRegion: true, isCrossZone: true, crossRegionErr: "", crossZoneErr: ""}, + localityType: LocalityComparisonType_CROSS_REGION, crossRegionErr: "", crossZoneErr: ""}, // One: (none) Two: (none) {l: makeLocalityStr("", ""), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (region) Two: (none) {l: makeLocalityStr(firstRegionStr, ""), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (zone) Two: (none) {l: makeLocalityStr("", firstZoneStr), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, // One: (both) Two: (none) {l: makeLocalityStr(firstRegionStr, firstZoneStr), other: makeLocalityStr("", ""), - isCrossRegion: false, isCrossZone: false, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, + localityType: LocalityComparisonType_SAME_REGION_SAME_ZONE, crossRegionErr: regionErrStr, crossZoneErr: zoneErrStr}, } { t.Run(fmt.Sprintf("%s-crosslocality-%s", tc.l, tc.other), func(t *testing.T) { var l Locality @@ -334,24 +333,21 @@ func TestLocalityIsCrossRegionCrossZone(t *testing.T) { require.NoError(t, l.Set(tc.l)) require.NoError(t, other.Set(tc.other)) type localities struct { - isCrossRegion bool - isCrossZone bool + localityType LocalityComparisonType crossRegionErr string crossZoneErr string } - isCrossRegion, crossRegionErr, isCrossZone, crossZoneErr := l.IsCrossRegionCrossZone(other) - crossRegionErrStr := "" - if crossRegionErr != nil { - crossRegionErrStr = crossRegionErr.Error() + localityType, regionErr, zoneErr := l.CompareWithLocality(other) + regionErrStr := "" + if regionErr != nil { + regionErrStr = regionErr.Error() } - crossZoneErrStr := "" - if crossZoneErr != nil { - crossZoneErrStr = crossZoneErr.Error() + zoneErrStr := "" + if zoneErr != nil { + zoneErrStr = zoneErr.Error() } - actual := localities{isCrossRegion, isCrossZone, - crossRegionErrStr, crossZoneErrStr} - expected := localities{tc.isCrossRegion, tc.isCrossZone, - tc.crossRegionErr, tc.crossZoneErr} + actual := localities{localityType, regionErrStr, zoneErrStr} + expected := localities{tc.localityType, tc.crossRegionErr, tc.crossZoneErr} require.Equal(t, expected, actual) }) } diff --git a/pkg/server/node.go b/pkg/server/node.go index ad35e68ad0b5..11fa232a403d 100644 --- a/pkg/server/node.go +++ b/pkg/server/node.go @@ -1325,82 +1325,80 @@ func (n *Node) batchInternal( return br, nil } -// isCrossRegionCrossZoneBatch returns (bool, bool) - indicating if the given -// batch request is cross-region and cross-zone respectively. -func (n *Node) isCrossRegionCrossZoneBatch( +// getCrossLocalityComparison compares the localities of the gateway node and +// the current node to determine if the given batch request is cross-region and +// cross-zone. +func (n *Node) getCrossLocalityComparison( ctx context.Context, ba *kvpb.BatchRequest, -) (bool, bool) { +) roachpb.LocalityComparisonType { gossip := n.storeCfg.Gossip if gossip == nil { log.VEventf(ctx, 2, "gossip is not configured") - return false, false + return roachpb.LocalityComparisonType_UNDEFINED } gatewayNodeDesc, err := gossip.GetNodeDescriptor(ba.GatewayNodeID) if err != nil { log.VEventf(ctx, 2, - "failed to perform look up for node descriptor %+v", err) - return false, false + "failed to perform look up for node descriptor %v", err) + return roachpb.LocalityComparisonType_UNDEFINED } - isCrossRegion, regionErr, isCrossZone, zoneErr := n.Descriptor.Locality. - IsCrossRegionCrossZone(gatewayNodeDesc.Locality) + comparisonResult, regionErr, zoneErr := n.Descriptor.Locality.CompareWithLocality(gatewayNodeDesc.Locality) if regionErr != nil { - log.VEventf(ctx, 2, "%v", regionErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross region %v", regionErr) } if zoneErr != nil { - log.VEventf(ctx, 2, "%v", zoneErr) + log.VEventf(ctx, 2, "unable to determine if batch is cross zone %v", zoneErr) } - return isCrossRegion, isCrossZone + return comparisonResult } -// checkAndUpdateBatchRequestMetrics updates the batch requests metrics in a -// more meaningful way. Cross-region metrics monitor activities across different -// regions. Cross-zone metrics monitor cross-zone activities within the same -// region or in cases where region tiers are not configured. The check result is -// returned here to avoid redundant check for metrics updates after receiving -// batch responses. -func (n *Node) checkAndUpdateBatchRequestMetrics( +// checkAndUpdateCrossLocalityBatchMetrics updates the batch requests metrics in +// a more meaningful way. Cross-region metrics monitor activities across +// different regions. Cross-zone metrics monitor cross-zone activities within +// the same region or in cases where region tiers are not configured. The +// locality comparison result is returned here to avoid redundant check for +// metrics updates after receiving batch responses. +func (n *Node) checkAndUpdateCrossLocalityBatchMetrics( ctx context.Context, ba *kvpb.BatchRequest, shouldIncrement bool, -) (shouldIncCrossRegion bool, shouldIncCrossZone bool) { +) roachpb.LocalityComparisonType { if !shouldIncrement { - return false, false + // shouldIncrement is set to false using testing knob in specific tests to + // filter out metrics changes caused by irrelevant batch requests. + return roachpb.LocalityComparisonType_UNDEFINED } n.metrics.BatchRequestsBytes.Inc(int64(ba.Size())) - isCrossRegion, isCrossZone := n.isCrossRegionCrossZoneBatch(ctx, ba) - if isCrossRegion { - if !isCrossZone { - log.VEventf(ctx, 2, "unexpected: cross region but same zone") - } else { - n.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossRegion = true - } - } else { - if isCrossZone { - n.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) - shouldIncCrossZone = true - } - } - return shouldIncCrossRegion, shouldIncCrossZone -} - -// checkAndUpdateBatchResponseMetrics updates the batch response metrics based -// on the shouldIncCrossRegion and shouldIncCrossZone parameters. These -// parameters are determined during the initial check for batch requests. The -// underlying assumption is that if requests were cross-region or cross-zone, -// the response should be as well. -func (n *Node) checkAndUpdateBatchResponseMetrics( - br *kvpb.BatchResponse, shouldIncCrossRegion bool, shouldIncCrossZone bool, shouldIncrement bool, + comparisonResult := n.getCrossLocalityComparison(ctx, ba) + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: + n.metrics.CrossRegionBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: + n.metrics.CrossZoneBatchRequestBytes.Inc(int64(ba.Size())) + case roachpb.LocalityComparisonType_SAME_REGION_SAME_ZONE: + // No metrics or error reporting. + } + return comparisonResult +} + +// updateCrossLocalityBatchMetrics updates the batch response metrics based on +// the comparisonResult parameter determined during the initial batch requests +// check. The underlying assumption is that the response should match the +// cross-region or cross-zone nature of the request. +func (n *Node) updateCrossLocalityBatchMetrics( + br *kvpb.BatchResponse, comparisonResult roachpb.LocalityComparisonType, shouldIncrement bool, ) { if !shouldIncrement { + // shouldIncrement is set to false using testing knob in specific tests to + // filter out metrics changes caused by irrelevant batch requests. return } n.metrics.BatchResponsesBytes.Inc(int64(br.Size())) - if shouldIncCrossRegion { + switch comparisonResult { + case roachpb.LocalityComparisonType_CROSS_REGION: n.metrics.CrossRegionBatchResponseBytes.Inc(int64(br.Size())) - } - if shouldIncCrossZone { + case roachpb.LocalityComparisonType_SAME_REGION_CROSS_ZONE: n.metrics.CrossZoneBatchResponseBytes.Inc(int64(br.Size())) } } @@ -1426,7 +1424,7 @@ func (n *Node) Batch(ctx context.Context, args *kvpb.BatchRequest) (*kvpb.BatchR // requests that are irrelevant to our tests. shouldIncrement = fn(args) } - shouldIncCrossRegion, shouldIncCrossZone := n.checkAndUpdateBatchRequestMetrics(ctx, args, shouldIncrement) + comparisonResult := n.checkAndUpdateCrossLocalityBatchMetrics(ctx, args, shouldIncrement) // NB: Node.Batch is called directly for "local" calls. We don't want to // carry the associated log tags forward as doing so makes adding additional @@ -1484,7 +1482,7 @@ func (n *Node) Batch(ctx context.Context, args *kvpb.BatchRequest) (*kvpb.BatchR // requests that are irrelevant to our tests. shouldIncrement = fn(br) } - n.checkAndUpdateBatchResponseMetrics(br, shouldIncCrossRegion, shouldIncCrossZone, shouldIncrement) + n.updateCrossLocalityBatchMetrics(br, comparisonResult, shouldIncrement) if buildutil.CrdbTestBuild && br.Error != nil && n.testingErrorEvent != nil { n.testingErrorEvent(ctx, args, errors.DecodeError(ctx, br.Error.EncodedError)) }