Skip to content

Commit

Permalink
Don't pass t to test metrics recorder
Browse files Browse the repository at this point in the history
  • Loading branch information
@zasweq
zasweq committed Sep 13, 2024
1 parent 01ab48b commit 2b745ab
Show file tree
Hide file tree
Showing 6 changed files with 177 additions and 122 deletions.
39 changes: 29 additions & 10 deletions balancer/rls/cache_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import (

"github.com/google/go-cmp/cmp"
"github.com/google/go-cmp/cmp/cmpopts"
estats "google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/internal/backoff"
"google.golang.org/grpc/internal/testutils/stats"
)
Expand Down Expand Up @@ -251,34 +252,52 @@ func (s) TestDataCache_Metrics(t *testing.T) {
{size: 4},
{size: 5},
}
tmr := stats.NewTestMetricsRecorder(t)
tmr := stats.NewTestMetricsRecorder()
dc := newDataCache(50, nil, tmr, "")

dc.updateRLSServerTarget("rls-server-target")
for i, k := range cacheKeys {
dc.addEntry(k, cacheEntriesMetricsTests[i])
}

const cacheEntriesKey = "grpc.lb.rls.cache_entries"
const cacheSizeKey = "grpc.lb.rls.cache_size"
// 5 total entries which add up to 15 size, so should record that.
tmr.AssertDataForMetric("grpc.lb.rls.cache_entries", 5)
tmr.AssertDataForMetric("grpc.lb.rls.cache_size", 15)
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 5 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 5)
}
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 15 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 15)
}

// Resize down the cache to 2 entries (deterministic as based of LRU).
dc.resize(9)
tmr.AssertDataForMetric("grpc.lb.rls.cache_entries", 2)
tmr.AssertDataForMetric("grpc.lb.rls.cache_size", 9)
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 2 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 2)
}
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 9 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 9)
}

// Update an entry to have size 6. This should reflect in the size metrics,
// which will increase by 1 to 11, while the number of cache entries should
// stay same. This write is deterministic and writes to the last one.
dc.updateEntrySize(cacheEntriesMetricsTests[4], 6)

tmr.AssertDataForMetric("grpc.lb.rls.cache_entries", 2)
tmr.AssertDataForMetric("grpc.lb.rls.cache_size", 10)
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 2 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 2)
}
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 10 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 10)
}

// Delete this scaled up cache key. This should scale down the cache to 2
// Delete this scaled up cache key. This should scale down the cache to 1
// entries, and remove 6 size so cache size should be 4.
dc.deleteAndCleanup(cacheKeys[4], cacheEntriesMetricsTests[4])
tmr.AssertDataForMetric("grpc.lb.rls.cache_entries", 1)
tmr.AssertDataForMetric("grpc.lb.rls.cache_size", 4)
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 1)
}
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 4 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 4)
}
}
43 changes: 31 additions & 12 deletions balancer/rls/picker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
"google.golang.org/grpc/balancer"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/credentials/insecure"
estats "google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/internal/grpcsync"
"google.golang.org/grpc/internal/stubserver"
rlstest "google.golang.org/grpc/internal/testutils/rls"
Expand Down Expand Up @@ -266,7 +267,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 := stats.NewTestMetricsRecorder(t)
tmr := stats.NewTestMetricsRecorder()
cc, err := grpc.Dial(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(tmr))
if err != nil {
t.Fatalf("grpc.Dial() failed: %v", err)
Expand All @@ -278,9 +279,15 @@ func (s) Test_RLSDefaultTargetPicksMetric(t *testing.T) {
defer cancel()
makeTestRPCAndExpectItToReachBackend(ctx, t, cc, defBackendCh)

tmr.AssertDataForMetric("grpc.lb.rls.default_target_picks", 1)
tmr.AssertNoDataForMetric("grpc.lb.rls.failed_picks")
tmr.AssertNoDataForMetric("grpc.lb.rls.target_picks")
if got := tmr.Data[estats.Metric("grpc.lb.rls.default_target_picks")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.rls.default_target_picks", got, 1)
}
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.target_picks")]; ok {
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.target_picks")
}
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.failed_picks")]; ok {
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.failed_picks")
}
}

// Test_RLSTargetPicksMetric tests the target picks metric. It configures an RLS
Expand All @@ -306,7 +313,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 := stats.NewTestMetricsRecorder(t)
tmr := stats.NewTestMetricsRecorder()
// Dial the backend.
cc, err := grpc.Dial(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(tmr))
if err != nil {
Expand All @@ -318,9 +325,15 @@ func (s) Test_RLSTargetPicksMetric(t *testing.T) {
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
defer cancel()
makeTestRPCAndExpectItToReachBackend(ctx, t, cc, testBackendCh)
tmr.AssertDataForMetric("grpc.lb.rls.target_picks", 1)
tmr.AssertNoDataForMetric("grpc.lb.rls.failed_picks")
tmr.AssertNoDataForMetric("grpc.lb.rls.default_target_picks")
if got := tmr.Data[estats.Metric("grpc.lb.rls.target_picks")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.rls.target_picks", got, 1)
}
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.default_target_picks")]; ok {
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.default_target_picks")
}
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.failed_picks")]; ok {
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.failed_picks")
}
}

// Test_RLSFailedPicksMetric tests the failed picks metric. It configures an RLS
Expand All @@ -338,7 +351,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 := stats.NewTestMetricsRecorder(t)
tmr := stats.NewTestMetricsRecorder()
// Dial the backend.
cc, err := grpc.Dial(r.Scheme()+":///", grpc.WithResolvers(r), grpc.WithTransportCredentials(insecure.NewCredentials()), grpc.WithStatsHandler(tmr))
if err != nil {
Expand All @@ -352,9 +365,15 @@ func (s) Test_RLSFailedPicksMetric(t *testing.T) {
defer cancel()
makeTestRPCAndVerifyError(ctx, t, cc, codes.Unavailable, errors.New("RLS response's target list does not contain any entries for key"))

tmr.AssertDataForMetric("grpc.lb.rls.failed_picks", 1)
tmr.AssertNoDataForMetric("grpc.lb.rls.target_picks")
tmr.AssertNoDataForMetric("grpc.lb.rls.default_target_picks")
if got := tmr.Data[estats.Metric("grpc.lb.rls.failed_picks")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.rls.failed_picks", got, 1)
}
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.target_picks")]; ok {
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.target_picks")
}
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.default_target_picks")]; ok {
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.default_target_picks")
}
}

// Test verifies the scenario where there is a matching entry in the data cache
Expand Down
25 changes: 21 additions & 4 deletions balancer/weightedroundrobin/balancer_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import (
"time"

"google.golang.org/grpc"
estats "google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/internal"
"google.golang.org/grpc/internal/grpctest"
"google.golang.org/grpc/internal/stubserver"
Expand Down Expand Up @@ -87,7 +88,7 @@ var (
OOBReportingPeriod: stringp("0.005s"),
BlackoutPeriod: stringp("0s"),
WeightExpirationPeriod: stringp("60s"),
WeightUpdatePeriod: stringp(".050s"),
WeightUpdatePeriod: stringp("30s"),
ErrorUtilizationPenalty: float64p(0),
}
)
Expand Down Expand Up @@ -224,7 +225,7 @@ func (s) TestWRRMetricsBasic(t *testing.T) {
srv := startServer(t, reportCall)
sc := svcConfig(t, testMetricsConfig)

mr := stats.NewTestMetricsRecorder(t)
mr := stats.NewTestMetricsRecorder()
if err := srv.StartClient(grpc.WithDefaultServiceConfig(sc), grpc.WithStatsHandler(mr)); err != nil {
t.Fatalf("Error starting client: %v", err)
}
Expand All @@ -234,12 +235,28 @@ func (s) TestWRRMetricsBasic(t *testing.T) {
t.Fatalf("Error from EmptyCall: %v", err)
}

mr.AssertDataForMetric("grpc.lb.wrr.rr_fallback", 1) // Falls back because only one SubConn.
mr.Mu.Lock()
defer mr.Mu.Unlock()
if got := mr.Data[estats.Metric("grpc.lb.wrr.rr_fallback")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.rr_fallback", got, 1)
}
if got := mr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != 0 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, 0)
}
if got := mr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_not_yet_usable")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_not_yet_usable", got, 1)
}
// Unusable, so no endpoint weight. Due to only one SubConn, this will never
// update the weight. Thus, this will stay 0.
if got := mr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != 0 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, 0)
}
/*mr.AssertDataForMetric("grpc.lb.wrr.rr_fallback", 1) // Falls back because only one SubConn.
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 0) // The endpoint weight has not expired so this is 0 (never emitted).
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 1)
// Unusable, so no endpoint weight. Due to only one SubConn, this will never
// update the weight. Thus, this will stay 0.
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)*/
}

// Tests two addresses with ORCA reporting disabled (should fall back to pure
Expand Down
25 changes: 18 additions & 7 deletions balancer/weightedroundrobin/metrics_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import (
"testing"
"time"

estats "google.golang.org/grpc/experimental/stats"
"google.golang.org/grpc/internal/grpctest"
iserviceconfig "google.golang.org/grpc/internal/serviceconfig"
"google.golang.org/grpc/internal/testutils/stats"
Expand Down Expand Up @@ -108,7 +109,7 @@ func (s) TestWRR_Metrics_SubConnWeight(t *testing.T) {

for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
tmr := stats.NewTestMetricsRecorder(t)
tmr := stats.NewTestMetricsRecorder()
wsc := &weightedSubConn{
metricsRecorder: tmr,
weightVal: 3,
Expand All @@ -117,9 +118,15 @@ func (s) TestWRR_Metrics_SubConnWeight(t *testing.T) {
}
wsc.weight(test.nowTime, test.weightExpirationPeriod, test.blackoutPeriod, true)

tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", test.endpointWeightStaleWant)
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", test.endpointWeightNotYetUsableWant)
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", test.endpointWeightWant)
if got := tmr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != test.endpointWeightStaleWant {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, test.endpointWeightStaleWant)
}
if got := tmr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_not_yet_usable")]; got != test.endpointWeightNotYetUsableWant {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_not_yet_usable", got, test.endpointWeightNotYetUsableWant)
}
if got := tmr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != test.endpointWeightStaleWant {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, test.endpointWeightStaleWant)
}
})
}

Expand All @@ -130,7 +137,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 := stats.NewTestMetricsRecorder(t)
tmr := stats.NewTestMetricsRecorder()
wsc := &weightedSubConn{
metricsRecorder: tmr,
weightVal: 0,
Expand All @@ -147,7 +154,9 @@ func (s) TestWRR_Metrics_Scheduler_RR_Fallback(t *testing.T) {
// There is only one SubConn, so no matter if the SubConn has a weight or
// not will fallback to round robin.
p.regenerateScheduler()
tmr.AssertDataForMetric("grpc.lb.wrr.rr_fallback", 1)
if got := tmr.Data[estats.Metric("grpc.lb.wrr.rr_fallback")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.rr_fallback", got, 1)
}
tmr.ClearMetrics()

// With two SubConns, if neither of them have weights, it will also fallback
Expand All @@ -159,5 +168,7 @@ func (s) TestWRR_Metrics_Scheduler_RR_Fallback(t *testing.T) {
}
p.subConns = append(p.subConns, wsc2)
p.regenerateScheduler()
tmr.AssertDataForMetric("grpc.lb.wrr.rr_fallback", 1)
if got := tmr.Data[estats.Metric("grpc.lb.wrr.rr_fallback")]; got != 1 {
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.rr_fallback", got, 1)
}
}
44 changes: 32 additions & 12 deletions internal/stats/metrics_recorder_list_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 := stats.NewTestMetricsRecorder(t)
mr2 := stats.NewTestMetricsRecorder(t)
mr1 := stats.NewTestMetricsRecorder()
mr2 := stats.NewTestMetricsRecorder()

defer internal.ClearGlobalDialOptions()
internal.AddGlobalDialOptions.(func(opt ...grpc.DialOption))(grpc.WithStatsHandler(mr1))
Expand All @@ -172,43 +172,63 @@ func (s) TestMetricsRecorderList(t *testing.T) {
LabelKeys: []string{"int counter label", "int counter optional label"},
LabelVals: []string{"int counter label val", "int counter optional label val"},
}
mr1.WaitForInt64Count(ctx, mdWant)
mr2.WaitForInt64Count(ctx, mdWant)
if err := mr1.WaitForInt64Count(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 176 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
if err := mr2.WaitForInt64Count(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 179 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}

mdWant = stats.MetricsData{
Handle: floatCountHandle.Descriptor(),
FloatIncr: 2,
LabelKeys: []string{"float counter label", "float counter optional label"},
LabelVals: []string{"float counter label val", "float counter optional label val"},
}
mr1.WaitForFloat64Count(ctx, mdWant)
mr2.WaitForFloat64Count(ctx, mdWant)
if err := mr1.WaitForFloat64Count(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 189 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
if err := mr2.WaitForFloat64Count(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 192 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}

mdWant = stats.MetricsData{
Handle: intHistoHandle.Descriptor(),
IntIncr: 3,
LabelKeys: []string{"int histo label", "int histo optional label"},
LabelVals: []string{"int histo label val", "int histo optional label val"},
}
mr1.WaitForInt64Histo(ctx, mdWant)
mr2.WaitForInt64Histo(ctx, mdWant)
if err := mr1.WaitForInt64Histo(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 202 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
if err := mr2.WaitForInt64Histo(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 205 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}

mdWant = stats.MetricsData{
Handle: floatHistoHandle.Descriptor(),
FloatIncr: 4,
LabelKeys: []string{"float histo label", "float histo optional label"},
LabelVals: []string{"float histo label val", "float histo optional label val"},
}
mr1.WaitForFloat64Histo(ctx, mdWant)
mr2.WaitForFloat64Histo(ctx, mdWant)
if err := mr1.WaitForFloat64Histo(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 215 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
if err := mr2.WaitForFloat64Histo(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 218 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
mdWant = stats.MetricsData{
Handle: intGaugeHandle.Descriptor(),
IntIncr: 5,
LabelKeys: []string{"int gauge label", "int gauge optional label"},
LabelVals: []string{"int gauge label val", "int gauge optional label val"},
}
mr1.WaitForInt64Gauge(ctx, mdWant)
mr2.WaitForInt64Gauge(ctx, mdWant)
if err := mr1.WaitForInt64Gauge(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 227 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
if err := mr2.WaitForInt64Gauge(ctx, mdWant); err != nil {
t.Fatalf(err.Error())

Check failure on line 230 in internal/stats/metrics_recorder_list_test.go

View workflow job for this annotation

GitHub Actions / tests (vet, 1.22) 

printf-style function with dynamic format string and no further arguments should use print-style function instead (SA1006)
}
}

// TestMetricRecorderListPanic tests that the metrics recorder list panics if
Expand Down
Loading

0 comments on commit 2b745ab

Please sign in to comment.