balancer/weightedroundrobin: Add recording point for endpoint weight not yet usable and add metrics tests #7466
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #7466 +/- ##
==========================================
- Coverage 81.57% 81.53% -0.05%
==========================================
Files 354 358 +4
Lines 27076 27278 +202
==========================================
+ Hits 22088 22240 +152
- Misses 3798 3824 +26
- Partials 1190 1214 +24
|
zasweq
force-pushed
the
add-tests-wrr-metrics
branch
2 times, most recently
from
270a26d to
37e724c
Compare
zasweq
force-pushed
the
add-tests-wrr-metrics
branch
from
37e724c to
f8aed9e
Compare
dfawley
left a comment
dfawley
left a comment
There was a problem hiding this comment.
Sending comments discussed offline
stats/opentelemetry/e2e_test.go
Outdated
| // load report, giving that SubConn a weight which will eventually expire. | ||
| // Two backends needed as for only one backend, WRR does not recompute the | ||
| // scheduler. | ||
| for i := 0; i < 100; i++ { |
There was a problem hiding this comment.
Use a map to guarantee you're hitting both backends. Or make RPCs simultaneously with checking metrics.
There was a problem hiding this comment.
Decided to just set weights on both of them, and do the latter. Even if I fork a goroutine here, I don't think there's a guarantee it doesn't just hit backend 2 over and over again 100 times even if I have a metrics emission (will emit endpoint weight metric even if doesn't have a weight).
stats/opentelemetry/e2e_test.go
Outdated
| rm := &metricdata.ResourceMetrics{} | ||
| reader.Collect(ctx, rm) | ||
| gotMetrics := map[string]metricdata.Metrics{} | ||
| for _, sm := range rm.ScopeMetrics { | ||
| for _, m := range sm.Metrics { | ||
| gotMetrics[m.Name] = m | ||
| } | ||
| } |
zasweq
force-pushed
the
add-tests-wrr-metrics
branch
from
ac5b190 to
f50574a
Compare
zasweq
force-pushed
the
add-tests-wrr-metrics
branch
from
f50574a to
6ff2e00
Compare
| // ClearMetrics clears the metrics data stores of the test metrics recorder by | ||
| // setting all the data to 0. |
There was a problem hiding this comment.
Why set everything to zero instead of r.data = make(...)?
There was a problem hiding this comment.
That is what I do in my constructor, but it seems like it doesn't matter because the writes in RecordSomething() will write a new data type. Should I get rid of that logic in constructor too and here?
There was a problem hiding this comment.
Oh I see.. Yes, I would just create an empty map unless there's a need to do otherwise (and then...question whether that need is truly necessary). The zero value as the default should make everything "just work".
There was a problem hiding this comment.
Yeah fair. Switched to just making an empty map, and letting new map write take care of it. Made no sense to configure with metrics name list I guess.
zasweq
force-pushed
the
add-tests-wrr-metrics
branch
from
5e8a314 to
aa9ea9f
Compare
zasweq
force-pushed
the
add-tests-wrr-metrics
branch
from
aa9ea9f to
761b729
Compare
| wsc := &weightedSubConn{ | ||
| metricsRecorder: tmr, | ||
| weightVal: 3, | ||
| } | ||
| if test.lastUpdatedSet { | ||
| wsc.lastUpdated = time.Now() | ||
| } | ||
| if test.nonEmptySet { | ||
| wsc.nonEmptySince = time.Now() | ||
| } |
There was a problem hiding this comment.
Consider instead to have lastUpdated and nonEmptySince as fields in the test cases struct as time.Times, and just set them to either time.Time{} or time.Now(). Then there's less logic which should make things easier to read & understand in the future.
There was a problem hiding this comment.
Ah right sounds good. Switched.
…not yet usable and add metrics tests (grpc#7466)
2 participants
This PR adds unit test assertions for Weighted Roudn Robin Metrics and an e2e test with exact OpenTelemetry metrics atoms expected.
It also fixes the "startup" case for WeightedSubConns when a WeightedSubConn has not received a load report yet. Previously, it would record an endpoint weight stale metric, but I added logic to make this case record on endpoint weight not yet usable instead.
RELEASE NOTES: N/A