Use an empty MetricsRecorderList instead of a NoOpMetricsRecorder in …

…TestClientConn
arjan-bal · Jan 27, 2025 · 7d97e03 · 7d97e03
1 parent 931c661
commit 7d97e03
Show file tree

Hide file tree

Showing 10 changed files with 48 additions and 43 deletions.
diff --git a/balancer/pickfirst/pickfirstleaf/metrics_test.go b/balancer/pickfirst/pickfirstleaf/metrics_test.go
@@ -30,6 +30,7 @@ import (
 	"google.golang.org/grpc/internal"
 	"google.golang.org/grpc/internal/stubserver"
 	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/testutils/stats"
 	testgrpc "google.golang.org/grpc/interop/grpc_testing"
 	testpb "google.golang.org/grpc/interop/grpc_testing"
 	"google.golang.org/grpc/resolver"
@@ -79,7 +80,7 @@ func (s) TestPickFirstMetrics(t *testing.T) {
 		Addresses:     []resolver.Address{{Addr: ss.Address}}},
 	)
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	cc, err := grpc.NewClient(r.Scheme()+":///", grpc.WithStatsHandler(tmr), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(r))
 	if err != nil {
 		t.Fatalf("NewClient() failed with error: %v", err)
@@ -123,7 +124,7 @@ func (s) TestPickFirstMetricsFailure(t *testing.T) {
 		Addresses:     []resolver.Address{{Addr: "bad address"}}},
 	)
 	grpcTarget := r.Scheme() + ":///"
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	cc, err := grpc.NewClient(grpcTarget, grpc.WithStatsHandler(tmr), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithResolvers(r))
 	if err != nil {
 		t.Fatalf("NewClient() failed with error: %v", err)

diff --git a/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go b/balancer/pickfirst/pickfirstleaf/pickfirstleaf_ext_test.go
@@ -40,6 +40,7 @@ import (
 	"google.golang.org/grpc/internal/stubserver"
 	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/internal/testutils/pickfirst"
+	"google.golang.org/grpc/internal/testutils/stats"
 	"google.golang.org/grpc/resolver"
 	"google.golang.org/grpc/resolver/manual"
 	"google.golang.org/grpc/status"
@@ -884,7 +885,7 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TF_AfterEndOfList(t *testing.T) {
 	triggerTimer, timeAfter := mockTimer()
 	pfinternal.TimeAfterFunc = timeAfter
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	dialer := testutils.NewBlockingDialer()
 	opts := []grpc.DialOption{
 		grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, pickfirstleaf.Name)),
@@ -973,7 +974,7 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TriggerConnectionDelay(t *testing.T) {
 	triggerTimer, timeAfter := mockTimer()
 	pfinternal.TimeAfterFunc = timeAfter
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	dialer := testutils.NewBlockingDialer()
 	opts := []grpc.DialOption{
 		grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, pickfirstleaf.Name)),
@@ -1033,7 +1034,7 @@ func (s) TestPickFirstLeaf_HappyEyeballs_TF_ThenTimerFires(t *testing.T) {
 	triggerTimer, timeAfter := mockTimer()
 	pfinternal.TimeAfterFunc = timeAfter
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	dialer := testutils.NewBlockingDialer()
 	opts := []grpc.DialOption{
 		grpc.WithDefaultServiceConfig(fmt.Sprintf(`{"loadBalancingConfig": [{"%s":{}}]}`, pickfirstleaf.Name)),

diff --git a/balancer/rls/cache_test.go b/balancer/rls/cache_test.go
@@ -25,7 +25,7 @@ import (
 	"github.com/google/go-cmp/cmp"
 	"github.com/google/go-cmp/cmp/cmpopts"
 	"google.golang.org/grpc/internal/backoff"
-	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/testutils/stats"
 )
 
 var (
@@ -120,7 +120,7 @@ func (s) TestLRU_BasicOperations(t *testing.T) {
 
 func (s) TestDataCache_BasicOperations(t *testing.T) {
 	initCacheEntries()
-	dc := newDataCache(5, nil, &testutils.NoopMetricsRecorder{}, "")
+	dc := newDataCache(5, nil, &stats.NoopMetricsRecorder{}, "")
 	for i, k := range cacheKeys {
 		dc.addEntry(k, cacheEntries[i])
 	}
@@ -134,7 +134,7 @@ func (s) TestDataCache_BasicOperations(t *testing.T) {
 
 func (s) TestDataCache_AddForcesResize(t *testing.T) {
 	initCacheEntries()
-	dc := newDataCache(1, nil, &testutils.NoopMetricsRecorder{}, "")
+	dc := newDataCache(1, nil, &stats.NoopMetricsRecorder{}, "")
 
 	// The first entry in cacheEntries has a minimum expiry time in the future.
 	// This entry would stop the resize operation since we do not evict entries
@@ -163,7 +163,7 @@ func (s) TestDataCache_AddForcesResize(t *testing.T) {
 
 func (s) TestDataCache_Resize(t *testing.T) {
 	initCacheEntries()
-	dc := newDataCache(5, nil, &testutils.NoopMetricsRecorder{}, "")
+	dc := newDataCache(5, nil, &stats.NoopMetricsRecorder{}, "")
 	for i, k := range cacheKeys {
 		dc.addEntry(k, cacheEntries[i])
 	}
@@ -194,7 +194,7 @@ func (s) TestDataCache_Resize(t *testing.T) {
 
 func (s) TestDataCache_EvictExpiredEntries(t *testing.T) {
 	initCacheEntries()
-	dc := newDataCache(5, nil, &testutils.NoopMetricsRecorder{}, "")
+	dc := newDataCache(5, nil, &stats.NoopMetricsRecorder{}, "")
 	for i, k := range cacheKeys {
 		dc.addEntry(k, cacheEntries[i])
 	}
@@ -221,7 +221,7 @@ func (s) TestDataCache_ResetBackoffState(t *testing.T) {
 	}
 
 	initCacheEntries()
-	dc := newDataCache(5, nil, &testutils.NoopMetricsRecorder{}, "")
+	dc := newDataCache(5, nil, &stats.NoopMetricsRecorder{}, "")
 	for i, k := range cacheKeys {
 		dc.addEntry(k, cacheEntries[i])
 	}
@@ -251,7 +251,7 @@ func (s) TestDataCache_Metrics(t *testing.T) {
 		{size: 4},
 		{size: 5},
 	}
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	dc := newDataCache(50, nil, tmr, "")
 
 	dc.updateRLSServerTarget("rls-server-target")

diff --git a/balancer/rls/picker_test.go b/balancer/rls/picker_test.go
@@ -31,8 +31,8 @@ import (
 	"google.golang.org/grpc/credentials/insecure"
 	"google.golang.org/grpc/internal/grpcsync"
 	"google.golang.org/grpc/internal/stubserver"
-	"google.golang.org/grpc/internal/testutils"
 	rlstest "google.golang.org/grpc/internal/testutils/rls"
+	"google.golang.org/grpc/internal/testutils/stats"
 	"google.golang.org/grpc/metadata"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/types/known/durationpb"
@@ -266,7 +266,7 @@ func (s) Test_RLSDefaultTargetPicksMetric(t *testing.T) {
 	// Register a manual resolver and push the RLS service config through it.
 	r := startManualResolverWithConfig(t, rlsConfig)
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	cc, err := grpc.NewClient(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(tmr))
 	if err != nil {
 		t.Fatalf("grpc.NewClient() failed: %v", err)
@@ -312,7 +312,7 @@ func (s) Test_RLSTargetPicksMetric(t *testing.T) {
 	// Register a manual resolver and push the RLS service config through it.
 	r := startManualResolverWithConfig(t, rlsConfig)
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	// Dial the backend.
 	cc, err := grpc.NewClient(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(tmr))
 	if err != nil {
@@ -350,7 +350,7 @@ func (s) Test_RLSFailedPicksMetric(t *testing.T) {
 	// Register a manual resolver and push the RLS service config through it.
 	r := startManualResolverWithConfig(t, rlsConfig)
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	// Dial the backend.
 	cc, err := grpc.NewClient(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(tmr))
 	if err != nil {

diff --git a/balancer/weightedroundrobin/balancer_test.go b/balancer/weightedroundrobin/balancer_test.go
@@ -31,8 +31,8 @@ import (
 	"google.golang.org/grpc/internal"
 	"google.golang.org/grpc/internal/grpctest"
 	"google.golang.org/grpc/internal/stubserver"
-	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/internal/testutils/roundrobin"
+	"google.golang.org/grpc/internal/testutils/stats"
 	"google.golang.org/grpc/orca"
 	"google.golang.org/grpc/peer"
 	"google.golang.org/grpc/resolver"
@@ -224,7 +224,7 @@ func (s) TestWRRMetricsBasic(t *testing.T) {
 	srv := startServer(t, reportCall)
 	sc := svcConfig(t, testMetricsConfig)
 
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	if err := srv.StartClient(grpc.WithDefaultServiceConfig(sc), grpc.WithStatsHandler(tmr)); err != nil {
 		t.Fatalf("Error starting client: %v", err)
 	}

diff --git a/balancer/weightedroundrobin/metrics_test.go b/balancer/weightedroundrobin/metrics_test.go
@@ -24,7 +24,7 @@ import (
 
 	"google.golang.org/grpc/internal/grpctest"
 	iserviceconfig "google.golang.org/grpc/internal/serviceconfig"
-	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/testutils/stats"
 )
 
 type s struct {
@@ -108,7 +108,7 @@ func (s) TestWRR_Metrics_SubConnWeight(t *testing.T) {
 
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {
-			tmr := testutils.NewTestMetricsRecorder()
+			tmr := stats.NewTestMetricsRecorder()
 			wsc := &endpointWeight{
 				metricsRecorder: tmr,
 				weightVal:       3,
@@ -136,7 +136,7 @@ func (s) TestWRR_Metrics_SubConnWeight(t *testing.T) {
 // with no weights. Both of these should emit a count metric for round robin
 // fallback.
 func (s) TestWRR_Metrics_Scheduler_RR_Fallback(t *testing.T) {
-	tmr := testutils.NewTestMetricsRecorder()
+	tmr := stats.NewTestMetricsRecorder()
 	ew := &endpointWeight{
 		metricsRecorder: tmr,
 		weightVal:       0,

diff --git a/internal/stats/metrics_recorder_list_test.go b/internal/stats/metrics_recorder_list_test.go
@@ -36,7 +36,7 @@ import (
 	"google.golang.org/grpc/internal"
 	"google.golang.org/grpc/internal/grpctest"
 	istats "google.golang.org/grpc/internal/stats"
-	"google.golang.org/grpc/internal/testutils"
+	"google.golang.org/grpc/internal/testutils/stats"
 	testgrpc "google.golang.org/grpc/interop/grpc_testing"
 	testpb "google.golang.org/grpc/interop/grpc_testing"
 	"google.golang.org/grpc/resolver"
@@ -146,8 +146,8 @@ func (s) TestMetricsRecorderList(t *testing.T) {
 
 	// Create two stats.Handlers which also implement MetricsRecorder, configure
 	// one as a global dial option and one as a local dial option.
-	mr1 := testutils.NewTestMetricsRecorder()
-	mr2 := testutils.NewTestMetricsRecorder()
+	mr1 := stats.NewTestMetricsRecorder()
+	mr2 := stats.NewTestMetricsRecorder()
 
 	defer internal.ClearGlobalDialOptions()
 	internal.AddGlobalDialOptions.(func(opt ...grpc.DialOption))(grpc.WithStatsHandler(mr1))
@@ -166,7 +166,7 @@ func (s) TestMetricsRecorderList(t *testing.T) {
 	// to record.
 	tsc.UnaryCall(ctx, &testpb.SimpleRequest{})
 
-	mdWant := testutils.MetricsData{
+	mdWant := stats.MetricsData{
 		Handle:    intCountHandle.Descriptor(),
 		IntIncr:   1,
 		LabelKeys: []string{"int counter label", "int counter optional label"},
@@ -179,7 +179,7 @@ func (s) TestMetricsRecorderList(t *testing.T) {
 		t.Fatal(err.Error())
 	}
 
-	mdWant = testutils.MetricsData{
+	mdWant = stats.MetricsData{
 		Handle:    floatCountHandle.Descriptor(),
 		FloatIncr: 2,
 		LabelKeys: []string{"float counter label", "float counter optional label"},
@@ -192,7 +192,7 @@ func (s) TestMetricsRecorderList(t *testing.T) {
 		t.Fatal(err.Error())
 	}
 
-	mdWant = testutils.MetricsData{
+	mdWant = stats.MetricsData{
 		Handle:    intHistoHandle.Descriptor(),
 		IntIncr:   3,
 		LabelKeys: []string{"int histo label", "int histo optional label"},
@@ -205,7 +205,7 @@ func (s) TestMetricsRecorderList(t *testing.T) {
 		t.Fatal(err.Error())
 	}
 
-	mdWant = testutils.MetricsData{
+	mdWant = stats.MetricsData{
 		Handle:    floatHistoHandle.Descriptor(),
 		FloatIncr: 4,
 		LabelKeys: []string{"float histo label", "float histo optional label"},
@@ -217,7 +217,7 @@ func (s) TestMetricsRecorderList(t *testing.T) {
 	if err := mr2.WaitForFloat64Histo(ctx, mdWant); err != nil {
 		t.Fatal(err.Error())
 	}
-	mdWant = testutils.MetricsData{
+	mdWant = stats.MetricsData{
 		Handle:    intGaugeHandle.Descriptor(),
 		IntIncr:   5,
 		LabelKeys: []string{"int gauge label", "int gauge optional label"},

diff --git a/internal/testutils/balancer.go b/internal/testutils/balancer.go
@@ -29,6 +29,8 @@ import (
 	"google.golang.org/grpc/experimental/stats"
 	"google.golang.org/grpc/internal/grpcsync"
 	"google.golang.org/grpc/resolver"
+
+	istats "google.golang.org/grpc/internal/stats"
 )
 
 // TestSubConn implements the SubConn interface, to be used in tests.
@@ -154,9 +156,9 @@ func (tcc *BalancerClientConn) NewSubConn(a []resolver.Address, o balancer.NewSu
 	return sc, nil
 }
 
-// MetricsRecorder returns a no-op MetricsRecorder.
+// MetricsRecorder returns an empty MetricsRecorderList.
 func (*BalancerClientConn) MetricsRecorder() stats.MetricsRecorder {
-	return &NoopMetricsRecorder{}
+	return istats.NewMetricsRecorderList(nil)
 }
 
 // RemoveSubConn is a nop; tests should all be updated to use sc.Shutdown()

diff --git a/internal/testutils/test_metrics_recorder.go → .../testutils/stats/test_metrics_recorder.go b/internal/testutils/test_metrics_recorder.go → .../testutils/stats/test_metrics_recorder.go
@@ -16,7 +16,8 @@
  *
  */
 
-package testutils
+// Package stats implements a TestMetricsRecorder utility.
+package stats
 
 import (
 	"context"
@@ -25,6 +26,7 @@ import (
 
 	"github.com/google/go-cmp/cmp"
 	estats "google.golang.org/grpc/experimental/stats"
+	"google.golang.org/grpc/internal/testutils"
 	"google.golang.org/grpc/stats"
 )
 
@@ -33,11 +35,11 @@ import (
 // have taken place. It also persists metrics data keyed on the metrics
 // descriptor.
 type TestMetricsRecorder struct {
-	intCountCh   *Channel
-	floatCountCh *Channel
-	intHistoCh   *Channel
-	floatHistoCh *Channel
-	intGaugeCh   *Channel
+	intCountCh   *testutils.Channel
+	floatCountCh *testutils.Channel
+	intHistoCh   *testutils.Channel
+	floatHistoCh *testutils.Channel
+	intGaugeCh   *testutils.Channel
 
 	// mu protects data.
 	mu sync.Mutex
@@ -48,11 +50,11 @@ type TestMetricsRecorder struct {
 // NewTestMetricsRecorder returns a new TestMetricsRecorder.
 func NewTestMetricsRecorder() *TestMetricsRecorder {
 	return &TestMetricsRecorder{
-		intCountCh:   NewChannelWithSize(10),
-		floatCountCh: NewChannelWithSize(10),
-		intHistoCh:   NewChannelWithSize(10),
-		floatHistoCh: NewChannelWithSize(10),
-		intGaugeCh:   NewChannelWithSize(10),
+		intCountCh:   testutils.NewChannelWithSize(10),
+		floatCountCh: testutils.NewChannelWithSize(10),
+		intHistoCh:   testutils.NewChannelWithSize(10),
+		floatHistoCh: testutils.NewChannelWithSize(10),
+		intGaugeCh:   testutils.NewChannelWithSize(10),
 
 		data: make(map[string]float64),
 	}

diff --git a/interop/xds/go.mod b/interop/xds/go.mod
@@ -22,7 +22,6 @@ require (
 	github.com/envoyproxy/protoc-gen-validate v1.1.0 // indirect
 	github.com/go-logr/logr v1.4.2 // indirect
 	github.com/go-logr/stdr v1.2.2 // indirect
-	github.com/google/go-cmp v0.6.0 // indirect
 	github.com/google/uuid v1.6.0 // indirect
 	github.com/klauspost/compress v1.17.11 // indirect
 	github.com/munnerz/goautoneg v0.0.0-20191010083416-a7dc8b61c822 // indirect