xds: Make locality ID string representation consistent with A78 (#8256)

Author: arjan-bal

stats/opentelemetry/e2e_test.go

@@ -698,7 +698,7 @@ func (s) TestWRRMetrics(t *testing.T) {
 	}()
 
 	targetAttr := attribute.String("grpc.target", target)
-	localityAttr := attribute.String("grpc.lb.locality", `{"region":"region-1","zone":"zone-1","subZone":"subzone-1"}`)
+	localityAttr := attribute.String("grpc.lb.locality", `{region="region-1", zone="zone-1", sub_zone="subzone-1"}`)
 
 	wantMetrics := []metricdata.Metrics{
 		{

test/xds/xds_telemetry_labels_test.go

@@ -46,7 +46,7 @@ const serviceNameValue = "grpc-service"
 const serviceNamespaceValue = "grpc-service-namespace"
 
 const localityKey = "grpc.lb.locality"
-const localityValue = `{"region":"region-1","zone":"zone-1","subZone":"subzone-1"}`
+const localityValue = `{region="region-1", zone="zone-1", sub_zone="subzone-1"}`
 
 // TestTelemetryLabels tests that telemetry labels from CDS make their way to
 // the stats handler. The stats handler sets the mutable context value that the

xds/internal/balancer/clusterimpl/balancer_test.go

@@ -185,7 +185,7 @@ func (s) TestDropByCategory(t *testing.T) {
 		TotalDrops: dropCount,
 		Drops:      map[string]uint64{dropReason: dropCount},
 		LocalityStats: map[string]load.LocalityData{
-			assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{
+			xdsinternal.LocalityID{}.ToString(): {RequestStats: load.RequestData{
 				Succeeded: (rpcCount - dropCount) * 3 / 4,
 				Errored:   (rpcCount - dropCount) / 4,
 				Issued:    rpcCount - dropCount,
@@ -251,7 +251,7 @@ func (s) TestDropByCategory(t *testing.T) {
 		TotalDrops: dropCount2,
 		Drops:      map[string]uint64{dropReason2: dropCount2},
 		LocalityStats: map[string]load.LocalityData{
-			assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{
+			xdsinternal.LocalityID{}.ToString(): {RequestStats: load.RequestData{
 				Succeeded: rpcCount - dropCount2,
 				Issued:    rpcCount - dropCount2,
 			}},
@@ -373,7 +373,7 @@ func (s) TestDropCircuitBreaking(t *testing.T) {
 		Service:    testServiceName,
 		TotalDrops: uint64(maxRequest),
 		LocalityStats: map[string]load.LocalityData{
-			assertString(xdsinternal.LocalityID{}.ToString): {RequestStats: load.RequestData{
+			xdsinternal.LocalityID{}.ToString(): {RequestStats: load.RequestData{
 				Succeeded: uint64(rpcCount - maxRequest),
 				Errored:   50,
 				Issued:    uint64(rpcCount - maxRequest + 50),
@@ -708,8 +708,8 @@ func (s) TestLoadReporting(t *testing.T) {
 	if sd.Cluster != testClusterName || sd.Service != testServiceName {
 		t.Fatalf("got unexpected load for %q, %q, want %q, %q", sd.Cluster, sd.Service, testClusterName, testServiceName)
 	}
-	testLocalityJSON, _ := testLocality.ToString()
-	localityData, ok := sd.LocalityStats[testLocalityJSON]
+	testLocalityStr := testLocality.ToString()
+	localityData, ok := sd.LocalityStats[testLocalityStr]
 	if !ok {
 		t.Fatalf("loads for %v not found in store", testLocality)
 	}
@@ -1016,11 +1016,3 @@ func (s) TestPickerUpdatedSynchronouslyOnConfigUpdate(t *testing.T) {
 		t.Fatal("Timed out waiting for client conn update to be completed.")
 	}
 }
-
-func assertString(f func() (string, error)) string {
-	s, err := f()
-	if err != nil {
-		panic(err.Error())
-	}
-	return s
-}

xds/internal/balancer/clusterimpl/picker.go

@@ -139,13 +139,9 @@ func (d *picker) Pick(info balancer.PickInfo) (balancer.PickResult, error) {
 		// This OK check also covers the case err!=nil, because SubConn will be
 		// nil.
 		pr.SubConn = scw.SubConn
-		var e error
 		// If locality ID isn't found in the wrapper, an empty locality ID will
 		// be used.
-		lIDStr, e = scw.localityID().ToString()
-		if e != nil {
-			logger.Infof("failed to marshal LocalityID: %#v, loads won't be reported", scw.localityID())
-		}
+		lIDStr = scw.localityID().ToString()
 	}
 
 	if err != nil {

xds/internal/balancer/clusterresolver/configbuilder.go

@@ -257,10 +257,7 @@ func priorityLocalitiesToClusterImpl(localities []xdsresource.Locality, priority
 		if locality.Weight != 0 {
 			lw = locality.Weight
 		}
-		localityStr, err := locality.ID.ToString()
-		if err != nil {
-			localityStr = fmt.Sprintf("%+v", locality.ID)
-		}
+		localityStr := locality.ID.ToString()
 		for _, endpoint := range locality.Endpoints {
 			// Filter out all "unhealthy" endpoints (unknown and healthy are
 			// both considered to be healthy:

xds/internal/balancer/clusterresolver/configbuilder_test.go

@@ -633,22 +633,14 @@ func TestPriorityLocalitiesToClusterImpl(t *testing.T) {
 	}
 }
 
-func assertString(f func() (string, error)) string {
-	s, err := f()
-	if err != nil {
-		panic(err.Error())
-	}
-	return s
-}
-
 func testEndpointWithAttrs(addrStrs []string, localityWeight, endpointWeight uint32, priority string, lID *internal.LocalityID) resolver.Endpoint {
 	endpoint := resolver.Endpoint{}
 	for _, a := range addrStrs {
 		endpoint.Addresses = append(endpoint.Addresses, resolver.Address{Addr: a})
 	}
 	path := []string{priority}
 	if lID != nil {
-		path = append(path, assertString(lID.ToString))
+		path = append(path, lID.ToString())
 		endpoint = internal.SetLocalityIDInEndpoint(endpoint, *lID)
 	}
 	endpoint = hierarchy.SetInEndpoint(endpoint, path)

xds/internal/balancer/wrrlocality/balancer.go

@@ -167,11 +167,7 @@ func (b *wrrLocalityBalancer) UpdateClientConnState(s balancer.ClientConnState)
 		// shouldn't happen though (this attribute that is set actually gets
 		// used to build localities in the first place), and thus don't error
 		// out, and just build a weighted target with undefined behavior.
-		locality, err := internal.GetLocalityID(addr).ToString()
-		if err != nil {
-			// Should never happen.
-			logger.Errorf("Failed to marshal LocalityID: %v, skipping this locality in weighted target")
-		}
+		locality := internal.GetLocalityID(addr).ToString()
 		ai, ok := getAddrInfo(addr)
 		if !ok {
 			return fmt.Errorf("xds_wrr_locality: missing locality weight information in address %q", addr)

xds/internal/balancer/wrrlocality/balancer_test.go

@@ -218,13 +218,13 @@ func (s) TestUpdateClientConnState(t *testing.T) {
 	// child layer.
 	wantWtCfg := &weightedtarget.LBConfig{
 		Targets: map[string]weightedtarget.Target{
-			"{\"region\":\"region-1\",\"zone\":\"zone-1\",\"subZone\":\"subzone-1\"}": {
+			"{region=\"region-1\", zone=\"zone-1\", sub_zone=\"subzone-1\"}": {
 				Weight: 2,
 				ChildPolicy: &internalserviceconfig.BalancerConfig{
 					Name: "round_robin",
 				},
 			},
-			"{\"region\":\"region-2\",\"zone\":\"zone-2\",\"subZone\":\"subzone-2\"}": {
+			"{region=\"region-2\", zone=\"zone-2\", sub_zone=\"subzone-2\"}": {
 				Weight: 1,
 				ChildPolicy: &internalserviceconfig.BalancerConfig{
 					Name: "round_robin",

xds/internal/internal.go

@@ -20,7 +20,6 @@
 package internal
 
 import (
-	"encoding/json"
 	"fmt"
 
 	"google.golang.org/grpc/resolver"
@@ -36,14 +35,10 @@ type LocalityID struct {
 	SubZone string `json:"subZone,omitempty"`
 }
 
-// ToString generates a string representation of LocalityID by marshalling it into
-// json. Not calling it String() so printf won't call it.
-func (l LocalityID) ToString() (string, error) {
-	b, err := json.Marshal(l)
-	if err != nil {
-		return "", err
-	}
-	return string(b), nil
+// ToString generates a string representation of LocalityID in the format
+// specified in gRFC A76. Not calling it String() so printf won't call it.
+func (l LocalityID) ToString() string {
+	return fmt.Sprintf("{region=%q, zone=%q, sub_zone=%q}", l.Region, l.Zone, l.SubZone)
 }
 
 // Equal allows the values to be compared by Attributes.Equal.
@@ -60,10 +55,10 @@ func (l LocalityID) Empty() bool {
 	return l.Region == "" && l.Zone == "" && l.SubZone == ""
 }
 
-// LocalityIDFromString converts a json representation of locality, into a
-// LocalityID struct.
+// LocalityIDFromString converts a string representation of locality as
+// specified in gRFC A76, into a LocalityID struct.
 func LocalityIDFromString(s string) (ret LocalityID, _ error) {
-	err := json.Unmarshal([]byte(s), &ret)
+	_, err := fmt.Sscanf(s, "{region=%q, zone=%q, sub_zone=%q}", &ret.Region, &ret.Zone, &ret.SubZone)
 	if err != nil {
 		return LocalityID{}, fmt.Errorf("%s is not a well formatted locality ID, error: %v", s, err)
 	}

xds/internal/internal_test.go

@@ -71,7 +71,7 @@ func (s) TestLocalityMatchProtoMessage(t *testing.T) {
 	}
 }
 
-func TestLocalityToAndFromJSON(t *testing.T) {
+func TestLocalityToAndFromString(t *testing.T) {
 	tests := []struct {
 		name       string
 		localityID LocalityID
@@ -81,25 +81,22 @@ func TestLocalityToAndFromJSON(t *testing.T) {
 		{
 			name:       "3 fields",
 			localityID: LocalityID{Region: "r:r", Zone: "z#z", SubZone: "s^s"},
-			str:        `{"region":"r:r","zone":"z#z","subZone":"s^s"}`,
+			str:        `{region="r:r", zone="z#z", sub_zone="s^s"}`,
 		},
 		{
 			name:       "2 fields",
 			localityID: LocalityID{Region: "r:r", Zone: "z#z"},
-			str:        `{"region":"r:r","zone":"z#z"}`,
+			str:        `{region="r:r", zone="z#z", sub_zone=""}`,
 		},
 		{
 			name:       "1 field",
 			localityID: LocalityID{Region: "r:r"},
-			str:        `{"region":"r:r"}`,
+			str:        `{region="r:r", zone="", sub_zone=""}`,
 		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			gotStr, err := tt.localityID.ToString()
-			if err != nil {
-				t.Errorf("failed to marshal LocalityID: %#v", tt.localityID)
-			}
+			gotStr := tt.localityID.ToString()
 			if gotStr != tt.str {
 				t.Errorf("%#v.String() = %q, want %q", tt.localityID, gotStr, tt.str)
 			}

xds/internal/xdsclient/tests/loadreport_test.go

@@ -44,8 +44,8 @@ import (
 )
 
 const (
-	testLocality1 = `{"region":"test-region1"}`
-	testLocality2 = `{"region":"test-region2"}`
+	testLocality1 = `{region="test-region1", zone="", sub_zone=""}`
+	testLocality2 = `{region="test-region2", zone="", sub_zone=""}`
 	testKey1      = "test-key1"
 	testKey2      = "test-key2"
 )

xds/internal/xdsclient/xdsresource/unmarshal_eds.go

@@ -169,7 +169,7 @@ func parseEDSRespProto(m *v3endpointpb.ClusterLoadAssignment) (EndpointsUpdate,
 			Zone:    l.Zone,
 			SubZone: l.SubZone,
 		}
-		lidStr, _ := lid.ToString()
+		lidStr := lid.ToString()
 
 		// "Since an xDS configuration can place a given locality under multiple
 		// priorities, it is possible to see locality weight attributes with