Skip to content

Commit 5fb24a6

Browse files
zasweqzasweq
committed
Responded to Doug's comments
1 parent 69aadc5 commit 5fb24a6

File tree

5 files changed

+49
-52
lines changed

5 files changed

+49
-52
lines changed

‎balancer/rls/cache_test.go

Lines changed: 8 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424

2525
"github.com/google/go-cmp/cmp"
2626
"github.com/google/go-cmp/cmp/cmpopts"
27-
estats "google.golang.org/grpc/experimental/stats"
2827
"google.golang.org/grpc/internal/backoff"
2928
"google.golang.org/grpc/internal/testutils/stats"
3029
)
@@ -263,19 +262,19 @@ func (s) TestDataCache_Metrics(t *testing.T) {
263262
const cacheEntriesKey = "grpc.lb.rls.cache_entries"
264263
const cacheSizeKey = "grpc.lb.rls.cache_size"
265264
// 5 total entries which add up to 15 size, so should record that.
266-
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 5 {
265+
if got, _ := tmr.Metric(cacheEntriesKey); got != 5 {
267266
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 5)
268267
}
269-
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 15 {
268+
if got, _ := tmr.Metric(cacheSizeKey); got != 15 {
270269
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 15)
271270
}
272271

273272
// Resize down the cache to 2 entries (deterministic as based of LRU).
274273
dc.resize(9)
275-
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 2 {
274+
if got, _ := tmr.Metric(cacheEntriesKey); got != 2 {
276275
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 2)
277276
}
278-
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 9 {
277+
if got, _ := tmr.Metric(cacheSizeKey); got != 9 {
279278
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 9)
280279
}
281280

@@ -284,20 +283,20 @@ func (s) TestDataCache_Metrics(t *testing.T) {
284283
// stay same. This write is deterministic and writes to the last one.
285284
dc.updateEntrySize(cacheEntriesMetricsTests[4], 6)
286285

287-
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 2 {
286+
if got, _ := tmr.Metric(cacheEntriesKey); got != 2 {
288287
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 2)
289288
}
290-
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 10 {
289+
if got, _ := tmr.Metric(cacheSizeKey); got != 10 {
291290
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 10)
292291
}
293292

294293
// Delete this scaled up cache key. This should scale down the cache to 1
295294
// entries, and remove 6 size so cache size should be 4.
296295
dc.deleteAndCleanup(cacheKeys[4], cacheEntriesMetricsTests[4])
297-
if got := tmr.Data[estats.Metric(cacheEntriesKey)]; got != 1 {
296+
if got, _ := tmr.Metric(cacheEntriesKey); got != 1 {
298297
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheEntriesKey, got, 1)
299298
}
300-
if got := tmr.Data[estats.Metric(cacheSizeKey)]; got != 4 {
299+
if got, _ := tmr.Metric(cacheSizeKey); got != 4 {
301300
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", cacheSizeKey, got, 4)
302301
}
303302
}

‎balancer/rls/picker_test.go

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,6 @@ import (
2929
"google.golang.org/grpc/balancer"
3030
"google.golang.org/grpc/codes"
3131
"google.golang.org/grpc/credentials/insecure"
32-
estats "google.golang.org/grpc/experimental/stats"
3332
"google.golang.org/grpc/internal/grpcsync"
3433
"google.golang.org/grpc/internal/stubserver"
3534
rlstest "google.golang.org/grpc/internal/testutils/rls"
@@ -279,13 +278,13 @@ func (s) Test_RLSDefaultTargetPicksMetric(t *testing.T) {
279278
defer cancel()
280279
makeTestRPCAndExpectItToReachBackend(ctx, t, cc, defBackendCh)
281280

282-
if got := tmr.Data[estats.Metric("grpc.lb.rls.default_target_picks")]; got != 1 {
281+
if got, _ := tmr.Metric("grpc.lb.rls.default_target_picks"); got != 1 {
283282
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.rls.default_target_picks", got, 1)
284283
}
285-
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.target_picks")]; ok {
284+
if _, ok := tmr.Metric("grpc.lb.rls.target_picks"); ok {
286285
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.target_picks")
287286
}
288-
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.failed_picks")]; ok {
287+
if _, ok := tmr.Metric("grpc.lb.rls.failed_picks"); ok {
289288
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.failed_picks")
290289
}
291290
}
@@ -325,13 +324,13 @@ func (s) Test_RLSTargetPicksMetric(t *testing.T) {
325324
ctx, cancel := context.WithTimeout(context.Background(), defaultTestTimeout)
326325
defer cancel()
327326
makeTestRPCAndExpectItToReachBackend(ctx, t, cc, testBackendCh)
328-
if got := tmr.Data[estats.Metric("grpc.lb.rls.target_picks")]; got != 1 {
327+
if got, _ := tmr.Metric("grpc.lb.rls.target_picks"); got != 1 {
329328
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.rls.target_picks", got, 1)
330329
}
331-
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.default_target_picks")]; ok {
330+
if _, ok := tmr.Metric("grpc.lb.rls.default_target_picks"); ok {
332331
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.default_target_picks")
333332
}
334-
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.failed_picks")]; ok {
333+
if _, ok := tmr.Metric("grpc.lb.rls.failed_picks"); ok {
335334
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.failed_picks")
336335
}
337336
}
@@ -365,13 +364,13 @@ func (s) Test_RLSFailedPicksMetric(t *testing.T) {
365364
defer cancel()
366365
makeTestRPCAndVerifyError(ctx, t, cc, codes.Unavailable, errors.New("RLS response's target list does not contain any entries for key"))
367366

368-
if got := tmr.Data[estats.Metric("grpc.lb.rls.failed_picks")]; got != 1 {
367+
if got, _ := tmr.Metric("grpc.lb.rls.failed_picks"); got != 1 {
369368
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.rls.failed_picks", got, 1)
370369
}
371-
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.target_picks")]; ok {
370+
if _, ok := tmr.Metric("grpc.lb.rls.target_picks"); ok {
372371
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.target_picks")
373372
}
374-
if _, ok := tmr.Data[estats.Metric("grpc.lb.rls.default_target_picks")]; ok {
373+
if _, ok := tmr.Metric("grpc.lb.rls.default_target_picks"); ok {
375374
t.Fatalf("Data is present for metric %v", "grpc.lb.rls.default_target_picks")
376375
}
377376
}

‎balancer/weightedroundrobin/balancer_test.go

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,6 @@ import (
2828
"time"
2929

3030
"google.golang.org/grpc"
31-
estats "google.golang.org/grpc/experimental/stats"
3231
"google.golang.org/grpc/internal"
3332
"google.golang.org/grpc/internal/grpctest"
3433
"google.golang.org/grpc/internal/stubserver"
@@ -225,8 +224,8 @@ func (s) TestWRRMetricsBasic(t *testing.T) {
225224
srv := startServer(t, reportCall)
226225
sc := svcConfig(t, testMetricsConfig)
227226

228-
mr := stats.NewTestMetricsRecorder()
229-
if err := srv.StartClient(grpc.WithDefaultServiceConfig(sc), grpc.WithStatsHandler(mr)); err != nil {
227+
tmr := stats.NewTestMetricsRecorder()
228+
if err := srv.StartClient(grpc.WithDefaultServiceConfig(sc), grpc.WithStatsHandler(tmr)); err != nil {
230229
t.Fatalf("Error starting client: %v", err)
231230
}
232231
srv.callMetrics.SetQPS(float64(1))
@@ -235,28 +234,22 @@ func (s) TestWRRMetricsBasic(t *testing.T) {
235234
t.Fatalf("Error from EmptyCall: %v", err)
236235
}
237236

238-
mr.Mu.Lock()
239-
defer mr.Mu.Unlock()
240-
if got := mr.Data[estats.Metric("grpc.lb.wrr.rr_fallback")]; got != 1 {
237+
tmr.Mu.Lock()
238+
defer tmr.Mu.Unlock()
239+
if got, _ := tmr.Metric("grpc.lb.wrr.rr_fallback"); got != 1 {
241240
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.rr_fallback", got, 1)
242241
}
243-
if got := mr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != 0 {
244-
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, 0)
242+
if got, ok := tmr.Metric("grpc.lb.wrr.endpoint_weight_stale"); !ok || got != 0 {
243+
t.Fatalf("Unexpected data for metric %v, present: %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", ok, got, 0)
245244
}
246-
if got := mr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_not_yet_usable")]; got != 1 {
245+
if got, _ := tmr.Metric("grpc.lb.wrr.endpoint_weight_not_yet_usable"); got != 1 {
247246
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_not_yet_usable", got, 1)
248247
}
249248
// Unusable, so no endpoint weight. Due to only one SubConn, this will never
250249
// update the weight. Thus, this will stay 0.
251-
if got := mr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != 0 {
252-
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, 0)
250+
if got, ok := tmr.Metric("grpc.lb.wrr.endpoint_weight_stale"); !ok || got != 0 {
251+
t.Fatalf("Unexpected data for metric %v, present: %v, got: %v, want: %v", ok, "grpc.lb.wrr.endpoint_weight_stale", got, 0)
253252
}
254-
/*mr.AssertDataForMetric("grpc.lb.wrr.rr_fallback", 1) // Falls back because only one SubConn.
255-
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_stale", 0) // The endpoint weight has not expired so this is 0 (never emitted).
256-
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weight_not_yet_usable", 1)
257-
// Unusable, so no endpoint weight. Due to only one SubConn, this will never
258-
// update the weight. Thus, this will stay 0.
259-
mr.AssertDataForMetric("grpc.lb.wrr.endpoint_weights", 0)*/
260253
}
261254

262255
// Tests two addresses with ORCA reporting disabled (should fall back to pure

‎balancer/weightedroundrobin/metrics_test.go

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,6 @@ import (
2222
"testing"
2323
"time"
2424

25-
estats "google.golang.org/grpc/experimental/stats"
2625
"google.golang.org/grpc/internal/grpctest"
2726
iserviceconfig "google.golang.org/grpc/internal/serviceconfig"
2827
"google.golang.org/grpc/internal/testutils/stats"
@@ -118,13 +117,13 @@ func (s) TestWRR_Metrics_SubConnWeight(t *testing.T) {
118117
}
119118
wsc.weight(test.nowTime, test.weightExpirationPeriod, test.blackoutPeriod, true)
120119

121-
if got := tmr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != test.endpointWeightStaleWant {
120+
if got, _ := tmr.Metric("grpc.lb.wrr.endpoint_weight_stale"); got != test.endpointWeightStaleWant {
122121
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, test.endpointWeightStaleWant)
123122
}
124-
if got := tmr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_not_yet_usable")]; got != test.endpointWeightNotYetUsableWant {
123+
if got, _ := tmr.Metric("grpc.lb.wrr.endpoint_weight_not_yet_usable"); got != test.endpointWeightNotYetUsableWant {
125124
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_not_yet_usable", got, test.endpointWeightNotYetUsableWant)
126125
}
127-
if got := tmr.Data[estats.Metric("grpc.lb.wrr.endpoint_weight_stale")]; got != test.endpointWeightStaleWant {
126+
if got, _ := tmr.Metric("grpc.lb.wrr.endpoint_weight_stale"); got != test.endpointWeightStaleWant {
128127
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.endpoint_weight_stale", got, test.endpointWeightStaleWant)
129128
}
130129
})
@@ -154,7 +153,7 @@ func (s) TestWRR_Metrics_Scheduler_RR_Fallback(t *testing.T) {
154153
// There is only one SubConn, so no matter if the SubConn has a weight or
155154
// not will fallback to round robin.
156155
p.regenerateScheduler()
157-
if got := tmr.Data[estats.Metric("grpc.lb.wrr.rr_fallback")]; got != 1 {
156+
if got, _ := tmr.Metric("grpc.lb.wrr.rr_fallback"); got != 1 {
158157
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.rr_fallback", got, 1)
159158
}
160159
tmr.ClearMetrics()
@@ -168,7 +167,7 @@ func (s) TestWRR_Metrics_Scheduler_RR_Fallback(t *testing.T) {
168167
}
169168
p.subConns = append(p.subConns, wsc2)
170169
p.regenerateScheduler()
171-
if got := tmr.Data[estats.Metric("grpc.lb.wrr.rr_fallback")]; got != 1 {
170+
if got, _ := tmr.Metric("grpc.lb.wrr.rr_fallback"); got != 1 {
172171
t.Fatalf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.wrr.rr_fallback", got, 1)
173172
}
174173
}

‎internal/testutils/stats/test_metrics_recorder.go

Lines changed: 17 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,10 @@ type TestMetricsRecorder struct {
4444
floatHistoCh *testutils.Channel
4545
intGaugeCh *testutils.Channel
4646

47-
// Mu protects Data.
47+
// Mu protects data.
4848
Mu sync.Mutex
49-
// Data is the most recent update for each metric name.
50-
Data map[estats.Metric]float64
49+
// data is the most recent update for each metric name.
50+
data map[estats.Metric]float64
5151
}
5252

5353
// NewTestMetricsRecorder returns a new TestMetricsRecorder.
@@ -59,15 +59,22 @@ func NewTestMetricsRecorder() *TestMetricsRecorder {
5959
floatHistoCh: testutils.NewChannelWithSize(10),
6060
intGaugeCh: testutils.NewChannelWithSize(10),
6161

62-
Data: make(map[estats.Metric]float64),
62+
data: make(map[estats.Metric]float64),
6363
}
6464
}
6565

66+
// Metric returns the most recent data for a metric, and
67+
// whether this recorder has received data for a metric.
68+
func (r *TestMetricsRecorder) Metric(name string) (float64, bool) {
69+
data, ok := r.data[estats.Metric(name)]
70+
return data, ok
71+
}
72+
6673
// ClearMetrics clears the metrics data store of the test metrics recorder.
6774
func (r *TestMetricsRecorder) ClearMetrics() {
6875
r.Mu.Lock()
6976
defer r.Mu.Unlock()
70-
r.Data = make(map[estats.Metric]float64)
77+
r.data = make(map[estats.Metric]float64)
7178
}
7279

7380
// MetricsData represents data associated with a metric.
@@ -112,7 +119,7 @@ func (r *TestMetricsRecorder) RecordInt64Count(handle *estats.Int64CountHandle,
112119

113120
r.Mu.Lock()
114121
defer r.Mu.Unlock()
115-
r.Data[handle.Name] = float64(incr)
122+
r.data[handle.Name] = float64(incr)
116123
}
117124

118125
// WaitForFloat64Count waits for a float count metric to be recorded and
@@ -144,7 +151,7 @@ func (r *TestMetricsRecorder) RecordFloat64Count(handle *estats.Float64CountHand
144151

145152
r.Mu.Lock()
146153
defer r.Mu.Unlock()
147-
r.Data[handle.Name] = incr
154+
r.data[handle.Name] = incr
148155
}
149156

150157
// WaitForInt64Histo waits for an int histo metric to be recorded and verifies
@@ -175,7 +182,7 @@ func (r *TestMetricsRecorder) RecordInt64Histo(handle *estats.Int64HistoHandle,
175182

176183
r.Mu.Lock()
177184
defer r.Mu.Unlock()
178-
r.Data[handle.Name] = float64(incr)
185+
r.data[handle.Name] = float64(incr)
179186
}
180187

181188
// WaitForFloat64Histo waits for a float histo metric to be recorded and
@@ -206,7 +213,7 @@ func (r *TestMetricsRecorder) RecordFloat64Histo(handle *estats.Float64HistoHand
206213

207214
r.Mu.Lock()
208215
defer r.Mu.Unlock()
209-
r.Data[handle.Name] = incr
216+
r.data[handle.Name] = incr
210217
}
211218

212219
// WaitForInt64Gauge waits for a int gauge metric to be recorded and
@@ -237,7 +244,7 @@ func (r *TestMetricsRecorder) RecordInt64Gauge(handle *estats.Int64GaugeHandle,
237244

238245
r.Mu.Lock()
239246
defer r.Mu.Unlock()
240-
r.Data[handle.Name] = float64(incr)
247+
r.data[handle.Name] = float64(incr)
241248
}
242249

243250
// To implement a stats.Handler, which allows it to be set as a dial option:

0 commit comments

Comments
 (0)