Skip to content

Commit aa9ea9f

Browse files
zasweqzasweq
committed
Responded to Doug's comments
1 parent 6ff2e00 commit aa9ea9f

File tree

5 files changed

+88
-109
lines changed

5 files changed

+88
-109
lines changed

‎balancer/weightedroundrobin/metrics_test.go‎

Lines changed: 84 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -39,63 +39,94 @@ func Test(t *testing.T) {
3939
// on a weighted SubConn, and expects certain metrics for each of these
4040
// scenarios.
4141
func (s) TestWRR_Metrics_SubConnWeight(t *testing.T) {
42-
tmr := stats.NewTestMetricsRecorder(t, []string{"grpc.lb.wrr.rr_fallback", "grpc.lb.wrr.endpoint_weight_not_yet_usable", "grpc.lb.wrr.endpoint_weight_stale", "grpc.lb.wrr.endpoint_weights"})
43-
44-
wsc := &weightedSubConn{
45-
metricsRecorder: tmr,
46-
weightVal: 3,
42+
tests := []struct {
43+
name string
44+
weightExpirationPeriod time.Duration
45+
blackoutPeriod time.Duration
46+
lastUpdatedSet bool
47+
nonEmptySet bool
48+
nowTime time.Time
49+
endpointWeightStaleWant float64
50+
endpointWeightNotYetUsableWant float64
51+
endpointWeightWant float64
52+
}{
53+
// The weighted SubConn's lastUpdated field hasn't been set, so this
54+
// SubConn's weight is not yet usable. Thus, should emit that endpoint
55+
// weight is not yet usable, and 0 for weight.
56+
{
57+
name: "no weight set",
58+
weightExpirationPeriod: time.Second,
59+
blackoutPeriod: time.Second,
60+
nowTime: time.Now(),
61+
endpointWeightStaleWant: 0,
62+
endpointWeightNotYetUsableWant: 1,
63+
endpointWeightWant: 0,
64+
},
65+
{
66+
name: "weight expiration",
67+
lastUpdatedSet: true,
68+
weightExpirationPeriod: 2 * time.Second,
69+
blackoutPeriod: time.Second,
70+
nowTime: time.Now().Add(100 * time.Second),
71+
endpointWeightStaleWant: 1,
72+
endpointWeightNotYetUsableWant: 0,
73+
endpointWeightWant: 0,
74+
},
75+
{
76+
name: "in blackout period",
77+
lastUpdatedSet: true,
78+
weightExpirationPeriod: time.Minute,
79+
blackoutPeriod: 10 * time.Second,
80+
nowTime: time.Now(),
81+
endpointWeightStaleWant: 0,
82+
endpointWeightNotYetUsableWant: 1,
83+
endpointWeightWant: 0,
84+
},
85+
{
86+
name: "normal weight",
87+
lastUpdatedSet: true,
88+
nonEmptySet: true,
89+
weightExpirationPeriod: time.Minute,
90+
blackoutPeriod: time.Second,
91+
nowTime: time.Now().Add(10 * time.Second),
92+
endpointWeightStaleWant: 0,
93+
endpointWeightNotYetUsableWant: 0,
94+
endpointWeightWant: 3,
95+
},
96+
{
97+
name: "weight expiration takes precdedence over blackout",
98+
lastUpdatedSet: true,
99+
nonEmptySet: true,
100+
weightExpirationPeriod: time.Second,
101+
blackoutPeriod: time.Minute,
102+
nowTime: time.Now().Add(10 * time.Second),
103+
endpointWeightStaleWant: 1,
104+
endpointWeightNotYetUsableWant: 0,
105+
endpointWeightWant: 0,
106+
},
47107
}
48108

49-
// The weighted SubConn's lastUpdated field hasn't been set, so this
50-
// SubConn's weight is not yet usable. Thus, should emit that endpoint
51-
// weight is not yet usable, and 0 for weight.
52-
wsc.weight(time.Now(), time.Second, time.Second, true)
53-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 0) // The endpoint weight has not expired so this is 0.
54-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 1)
55-
// Unusable, so no endpoint weight (i.e. 0).
56-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)
57-
tmr.ClearMetrics()
58-
59-
// Setup a scenario where the SubConn's weight expires. Thus, should emit
60-
// that endpoint weight is stale, and 0 for weight.
61-
wsc.lastUpdated = time.Now()
62-
wsc.weight(time.Now().Add(100*time.Second), 2*time.Second, time.Second, true)
63-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 1)
64-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 0)
65-
// Unusable, so no endpoint weight (i.e. 0).
66-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)
67-
tmr.ClearMetrics()
68-
69-
// Setup a scenario where the SubConn's weight is in the blackout period.
70-
// Thus, should emit that endpoint weight is not yet usable, and 0 for
71-
// weight.
72-
wsc.weight(time.Now(), time.Minute, 10*time.Second, true)
73-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 0)
74-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 1)
75-
// Unusable, so no endpoint weight (i.e. 0).
76-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)
77-
tmr.ClearMetrics()
109+
for _, test := range tests {
110+
t.Run(test.name, func(t *testing.T) {
111+
tmr := stats.NewTestMetricsRecorder(t, []string{"grpc.lb.wrr.rr_fallback", "grpc.lb.wrr.endpoint_weight_not_yet_usable", "grpc.lb.wrr.endpoint_weight_stale", "grpc.lb.wrr.endpoint_weights"})
112+
wsc := &weightedSubConn{
113+
metricsRecorder: tmr,
114+
weightVal: 3,
115+
}
116+
if test.lastUpdatedSet {
117+
wsc.lastUpdated = time.Now()
118+
}
119+
if test.nonEmptySet {
120+
wsc.nonEmptySince = time.Now()
121+
}
122+
wsc.weight(test.nowTime, test.weightExpirationPeriod, test.blackoutPeriod, true)
78123

79-
// Setup a scenario where SubConn's weight is what is persists in weight
80-
// field. This is triggered by last update being past blackout period and
81-
// before weight update period. Should thus emit that endpoint weight is 3,
82-
// and no other metrics.
83-
wsc.nonEmptySince = time.Now()
84-
wsc.weight(time.Now().Add(10*time.Second), time.Minute, time.Second, true)
85-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 0)
86-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 0)
87-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 3)
88-
tmr.ClearMetrics()
124+
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", test.endpointWeightStaleWant)
125+
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", test.endpointWeightNotYetUsableWant)
126+
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", test.endpointWeightWant)
127+
})
128+
}
89129

90-
// Setup a scenario where a SubConn's weight both expires and is within the
91-
// blackout period. In this case, weight expiry should take precedence with
92-
// respect to metrics emitted. Thus, should emit that endpoint weight is not
93-
// yet usable, and 0 for weight.
94-
wsc.weight(time.Now().Add(10*time.Second), time.Second, time.Minute, true)
95-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 1)
96-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 0)
97-
tmr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)
98-
tmr.ClearMetrics()
99130
}
100131

101132
// TestWRR_Metrics_Scheduler_RR_Fallback tests the round robin fallback metric

‎internal/testutils/stats/test_metrics_recorder.go‎

Lines changed: 1 addition & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -79,25 +79,14 @@ func (r *TestMetricsRecorder) AssertDataForMetric(metricName string, wantVal flo
7979
}
8080
}
8181

82-
// AssertEitherDataForMetric asserts either data point is present for metric.
83-
// The zero value in the check is equivalent to unset.
84-
85-
func (r *TestMetricsRecorder) AssertEitherDataForMetric(metricName string, wantVal1 float64, wantVal2 float64) {
86-
r.mu.Lock()
87-
defer r.mu.Unlock()
88-
if r.data[estats.Metric(metricName)] != wantVal1 && r.data[estats.Metric(metricName)] != wantVal2 {
89-
r.t.Fatalf("Unexpected data for metric %v, got: %v, want: %v or %v", metricName, r.data[estats.Metric(metricName)], wantVal1, wantVal2)
90-
}
91-
}
92-
9382
// PollForDataForMetric polls the metric data for the want. Fails if context
9483
// provided expires before data for metric is found.
9584
func (r *TestMetricsRecorder) PollForDataForMetric(ctx context.Context, metricName string, wantVal float64) {
9685
for ; ctx.Err() == nil; <-time.After(time.Millisecond) {
9786
r.mu.Lock()
9887
if r.data[estats.Metric(metricName)] == wantVal {
9988
r.mu.Unlock()
100-
break
89+
return
10190
}
10291
r.mu.Unlock()
10392
}

‎stats/opentelemetry/e2e_test.go‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -473,11 +473,9 @@ func (s) TestWRRMetrics(t *testing.T) {
473473
// scheduler.
474474
receivedExpectedMetrics := grpcsync.NewEvent()
475475
go func() {
476-
for i := 0; i < 100; i++ {
476+
for !receivedExpectedMetrics.HasFired() {
477477
client.EmptyCall(ctx, &testpb.Empty{})
478-
if receivedExpectedMetrics.HasFired() {
479-
break
480-
}
478+
time.Sleep(2 * time.Millisecond)
481479
}
482480
}()
483481

@@ -554,10 +552,6 @@ func (s) TestWRRMetrics(t *testing.T) {
554552
},
555553
}
556554

557-
if ctx.Err() != nil {
558-
t.Fatalf("Timeout waiting for metric %v", eventuallyWantMetric.Name)
559-
}
560-
561555
if err := pollForWantMetrics(ctx, t, reader, []metricdata.Metrics{eventuallyWantMetric}); err != nil {
562556
t.Fatal(err)
563557
}

‎test/xds/xds_client_integration_test.go‎

Lines changed: 0 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -24,16 +24,13 @@ import (
2424
"testing"
2525
"time"
2626

27-
"github.com/google/uuid"
2827
"google.golang.org/grpc"
2928
"google.golang.org/grpc/credentials/insecure"
30-
"google.golang.org/grpc/internal"
3129
"google.golang.org/grpc/internal/grpctest"
3230
"google.golang.org/grpc/internal/stubserver"
3331
"google.golang.org/grpc/internal/testutils"
3432
"google.golang.org/grpc/internal/testutils/xds/e2e"
3533
"google.golang.org/grpc/internal/testutils/xds/e2e/setup"
36-
"google.golang.org/grpc/resolver"
3734

3835
testgrpc "google.golang.org/grpc/interop/grpc_testing"
3936
testpb "google.golang.org/grpc/interop/grpc_testing"
@@ -52,38 +49,6 @@ const (
5249
defaultTestShortTimeout = 10 * time.Millisecond // For events expected to *not* happen.
5350
)
5451

55-
// setupManagementServerAndResolver sets up an xDS management server, creates
56-
// bootstrap configuration pointing to that server and creates an xDS resolver
57-
// using that configuration.
58-
//
59-
// Registers a cleanup function on t to stop the management server.
60-
//
61-
// Returns the following:
62-
// - the xDS management server
63-
// - the node ID to use when talking to this management server
64-
// - bootstrap configuration to use (if creating an xDS-enabled gRPC server)
65-
// - xDS resolver builder (if creating an xDS-enabled gRPC client)
66-
func setupManagementServerAndResolver(t *testing.T) (*e2e.ManagementServer, string, []byte, resolver.Builder) {
67-
// Start an xDS management server.
68-
xdsServer := e2e.StartManagementServer(t, e2e.ManagementServerOptions{AllowResourceSubset: true})
69-
70-
// Create bootstrap configuration pointing to the above management server.
71-
nodeID := uuid.New().String()
72-
bc := e2e.DefaultBootstrapContents(t, nodeID, xdsServer.Address)
73-
74-
// Create an xDS resolver with the above bootstrap configuration.
75-
var r resolver.Builder
76-
var err error
77-
if newResolver := internal.NewXDSResolverWithConfigForTesting; newResolver != nil {
78-
r, err = newResolver.(func([]byte) (resolver.Builder, error))(bc)
79-
if err != nil {
80-
t.Fatalf("Failed to create xDS resolver for testing: %v", err)
81-
}
82-
}
83-
84-
return xdsServer, nodeID, bc, r
85-
}
86-
8752
func (s) TestClientSideXDS(t *testing.T) {
8853
managementServer, nodeID, _, xdsResolver := setup.ManagementServerAndResolver(t)
8954

‎test/xds/xds_client_outlier_detection_test.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,7 @@ func (s) TestOutlierDetectionWithOutlier(t *testing.T) {
243243
// Detection present in the CDS update, but with SuccessRateEjection unset, and
244244
// asserts that Outlier Detection is turned on and ejects upstreams.
245245
func (s) TestOutlierDetectionXDSDefaultOn(t *testing.T) {
246-
managementServer, nodeID, _, xdsResolver := setupManagementServerAndResolver(t)
246+
managementServer, nodeID, _, xdsResolver := setup.ManagementServerAndResolver(t)
247247

248248
// Working backend 1.
249249
backend1 := stubserver.StartTestService(t, nil)

0 commit comments

Comments
 (0)