balancer/rls: Add picker and cache unit tests for RLS Metrics #7614
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7614 +/- ##
==========================================
- Coverage 81.93% 81.84% -0.09%
==========================================
Files 361 361
Lines 27816 27814 -2
==========================================
- Hits 22790 22764 -26
- Misses 3837 3851 +14
- Partials 1189 1199 +10
|
zasweq
force-pushed
the
rls-metrics-cache-picker-test
branch
from
dc4efec
to
29ea040
Compare
| r.mu.Lock() | ||
| defer r.mu.Unlock() | ||
| if _, ok := r.data[estats.Metric(metricName)]; ok { | ||
| r.t.Fatalf("Data is present for metric %v", metricName) |
There was a problem hiding this comment.
This is following an anti-pattern: https://google.github.io/styleguide/go/decisions#assert
Now that we have more time, let's address this.
There was a problem hiding this comment.
Didn't pass t to this component anymore, did all the assertions inline.
balancer/rls/cache_test.go
Outdated
| @@ -242,3 +242,50 @@ func (s) TestDataCache_ResetBackoffState(t *testing.T) { | |||
| t.Fatalf("unexpected diff in backoffState for cache entry after dataCache.resetBackoffState(): %s", diff) | |||
| } | |||
| } | |||
|
|
|||
| var cacheEntriesMetricsTests = []*cacheEntry{ | |||
There was a problem hiding this comment.
Please move inside test function so it's not global.
There was a problem hiding this comment.
Ah good point. Done. No need to clear back to original state too as I do later.
balancer/rls/cache_test.go
Outdated
| // Resize down the cache to 3 entries. This should scale down the cache to 3 | ||
| // entries with 3 bytes each, so should record 3 entries and 9 size. | ||
| dc.resize(9) | ||
| tmr.AssertDataForMetric("grpc.lb.rls.cache_entries", 3) | ||
| tmr.AssertDataForMetric("grpc.lb.rls.cache_size", 9) |
There was a problem hiding this comment.
This relies on having the same size for every entry? Or does order matter?
There was a problem hiding this comment.
Yeah it relies. I think operations are deterministic if I tweak the numbers, but I don't know if we'll have that guarantee in the future. Let me play around with this some.
balancer/rls/cache_test.go
Outdated
| {size: 3}, | ||
| {size: 3}, | ||
| {size: 3}, | ||
| {size: 3}, |
There was a problem hiding this comment.
It seems like these should vary to have better coverage.
There was a problem hiding this comment.
Switched to 12345, and scaled the assertions to follow this. Luckily all the cache operations are deterministic.
zasweq
force-pushed
the
rls-metrics-cache-picker-test
branch
from
2b745ab
to
0ff3022
Compare
zasweq
force-pushed
the
rls-metrics-cache-picker-test
branch
3 times, most recently
from
5fb24a6
to
e8ff69d
Compare
zasweq
force-pushed
the
rls-metrics-cache-picker-test
branch
from
b2338c5
to
2c3d2f3
Compare
| intCountCh *testutils.Channel | ||
| floatCountCh *testutils.Channel | ||
| intHistoCh *testutils.Channel | ||
| floatHistoCh *testutils.Channel | ||
| intGaugeCh *testutils.Channel | ||
|
|
||
| // Mu protects Data. | ||
| // Mu protects data. | ||
| Mu sync.Mutex |
There was a problem hiding this comment.
The main goal of making a getter was so that mu didn't need to be exported and to make access simpler, since mutexes are generally internal state and complicate things.
Seems like you need something more complex if you need an atomic read of multiple values (though this wasn't supported previously?). func Metrics(name []string) (*float64)???
There was a problem hiding this comment.
Hmmmmm I just deleted the grab of it in balancer_test. I was worried due to the low time for weight updates (when time.Ticker goes off and reupdates scheduler) but I made it 30 seconds so should never race (it'll fail much earlier than that). So mutex still grabbed sometimes in this component in exported helpers.
This PR adds picker and cache unit tests to test RLS Metrics.
RELEASE NOTES: N/A