From 7d97e0305ea53ecc288f9c7b631ef5b214b16e5a Mon Sep 17 00:00:00 2001 From: Arjan Bal Date: Mon, 27 Jan 2025 16:00:25 +0530 Subject: [PATCH] Use an empty MetricsRecorderList instead of a NoOpMetricsRecorder in TestClientConn --- .../pickfirst/pickfirstleaf/metrics_test.go | 5 ++-- .../pickfirstleaf/pickfirstleaf_ext_test.go | 7 +++--- balancer/rls/cache_test.go | 14 +++++------ balancer/rls/picker_test.go | 8 +++---- balancer/weightedroundrobin/balancer_test.go | 4 ++-- balancer/weightedroundrobin/metrics_test.go | 6 ++--- internal/stats/metrics_recorder_list_test.go | 16 ++++++------- internal/testutils/balancer.go | 6 +++-- .../{ => stats}/test_metrics_recorder.go | 24 ++++++++++--------- interop/xds/go.mod | 1 - 10 files changed, 48 insertions(+), 43 deletions(-) rename internal/testutils/{ => stats}/test_metrics_recorder.go (95%) diff --git a/balancer/pickfirst/pickfirstleaf/metrics_test.go b/balancer/pickfirst/pickfirstleaf/metrics_test.go index c4e14e693205..90beca6adc42 100644 --- 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 index 8ff3c35e47bb..33970efb2e99 100644 --- 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 index 88be32268381..52239deb77d6 100644 --- 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 index b77eb9217d80..a0bdbc827921 100644 --- 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 index a741ee49a8c6..5e369780764e 100644 --- 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 index 7866ebd17541..79e4d0a145a0 100644 --- 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 index ec938118835e..fef3d1dff1e0 100644 --- 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 index ff24c30218dd..13490b5e3cd9 100644 --- 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 b/internal/testutils/stats/test_metrics_recorder.go similarity index 95% rename from internal/testutils/test_metrics_recorder.go rename to internal/testutils/stats/test_metrics_recorder.go index ca2a04c3431f..e1a03b8d8008 100644 --- a/internal/testutils/test_metrics_recorder.go +++ b/internal/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 index 7663b85476dc..99fe9a0fb584 100644 --- 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