balancer/weightedroundrobin: Add emissions of metrics through metrics registry #7439
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7439 +/- ##
==========================================
+ Coverage 81.39% 81.49% +0.10%
==========================================
Files 354 354
Lines 27053 27076 +23
==========================================
+ Hits 22020 22066 +46
+ Misses 3828 3814 -14
+ Partials 1205 1196 -9
|
| // These fields are built out at creation time and read only after, so no | ||
| // need for synchronization. |
There was a problem hiding this comment.
// The following fields are immutable?
There was a problem hiding this comment.
Sure this is how I see it documented throughout the codebase in other places but it's equivalent to yours so switched for all three structs I added this comment for.
| w.nonEmptySince = time.Time{} | ||
| return 0 | ||
| return |
There was a problem hiding this comment.
Please (almost?) never use bare returns from functions that return values. It's too hard to read. Use return weight or, even better, keep as return 0.
There was a problem hiding this comment.
I'll return weight so reader doesn't need to keep track of two pieces of information (named return value which gets logged and the actual return value).
There was a problem hiding this comment.
IMO this is worse because now you have to go backtrack and figure out what weight might be while reading the code. If it just says return 0 then it's obvious.
Note that named return values are set upon returning. I.e.
func f() (x int) {
x = 5
defer func() {
fmt.Println(x)
}()
return 10
}Will print 10.
There was a problem hiding this comment.
Ah ok I didn't realize named return values were set upon returning. Switched to incorporate all your suggestions.
|
Thanks for the pass :)! Got to all comments! |
| weight = 0 | ||
| if forScheduler { | ||
| defer endpointWeightsHandle.Record(w.metricsRecorder, weight, []string{w.target, w.locality}...) | ||
| weight := float64(0) |
There was a problem hiding this comment.
Named return values are totally fine if you want to keep it. Just don't do a bare return, since it can be confusing (for a minute I actually thought you removed the return value when I saw the bare return, and had to go back to see what was happening).
(But you don't need to initialize them to 0 as in the previous code; Go always initializes everything to zero.)
There was a problem hiding this comment.
Switched to named return value.
| } | ||
|
|
||
| return w.weightVal | ||
| weight = w.weightVal |
There was a problem hiding this comment.
Yeah...I'd prefer to use what you had before. Use the named return value, and don't ever assign to weight at all. Just read it in the defer to find out what was returned.
There was a problem hiding this comment.
Switched to named return value.
This PR adds emissions of weighted round robin metrics defined in A78.
RELEASE NOTES: N/A