-
Notifications
You must be signed in to change notification settings - Fork 4.6k
stats/otel: Add subchannel metrics (A94) #8738
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #8738 +/- ##
==========================================
+ Coverage 83.22% 83.37% +0.15%
==========================================
Files 419 418 -1
Lines 32454 32425 -29
==========================================
+ Hits 27009 27034 +25
+ Misses 4057 4010 -47
+ Partials 1388 1381 -7
🚀 New features to boost your workflow:
|
|
A94 states the following: How are we currently handling this in the pickfirst metrics? |
|
A94 states the following: Can you please ensure that we have an issue filed to track the removal of the old metrics and that it captures the correct release where it needs to be removed. |
Here is how pickfirst handles this:
|
Issue filed - #8752 |
Handled as per PR |
balancer/pickfirst/metrics_test.go
Outdated
| t.Errorf("Unexpected data for metric %v, got: %v, want: %v", "grpc.lb.pick_first.disconnections", got, 0) | ||
| } | ||
|
|
||
| //Checking for subchannel metrics as well |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Space between the // and the start of the comment, and please terminate comment sentences with a period.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and elsewhere. There are still a bunch of places with comments that are not terminated with periods. See: go/go-style/decisions#comment-sentences
| // Wait for the SUCCESS metric to ensure recording logic has processed. | ||
| waitForMetric(ctx, t, tmr, "grpc.subchannel.connection_attempts_succeeded") | ||
|
|
||
| // Verify Success: Exactly 1 (The Winner). | ||
| if got, _ := tmr.Metric("grpc.subchannel.connection_attempts_succeeded"); got != 1 { | ||
| t.Errorf("Unexpected data for metric %v, got: %v, want: 1", "grpc.subchannel.connection_attempts_succeeded", got) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this actually ensure that we check the value of the metric after the first connection attempt is completely processed? We do call holds[0].Resume(), but does that guarantee that the subchannel code sees the connection being successful, but drops it since the subchannel has been deleted by the LB policy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are waiting for the metric to be emitted. Connection attempt success will only be emitted if there is a successful connection. In case of cancellation of attempt - it will not be successful and in case of disconnection after establishing connection, it will still be recorded as a disconnection. In both scenarios, the attempts succeeded will always be 1.
easwars
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good mostly. Just some minor nits.
Can you also please update the PR description to note that the disconnection_reason will be plumbed in a follow-up PR. Thanks.
| _, ok := tmr.Metric(metricName) | ||
| if ok { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: The assignment and the conditional can be moved into a single line:
if _, ok := tmr.Metric(metricName); ok {
...
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
done. |
Addresses : https://github.com/grpc/proposal/blob/master/A94-subchannel-otel-metrics.md
This PR adds subchannel metrics with applicable labels as per the RFC proposal.
disconnection_reasonwill be added as a follow up PR.RELEASE NOTES: