-
Notifications
You must be signed in to change notification settings - Fork 4.6k
outlierdetection: add metrics specified in gRFC A91 #8644
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
Co-authored-by: eshitachandwani <[email protected]>
Co-authored-by: eshitachandwani <[email protected]>
Co-authored-by: eshitachandwani <[email protected]>
… `""` * adds tests to verify the unenforced ejection metrics are recorded correctly when outliers are detected but not enforced. Signed-off-by: sotiris <[email protected]>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8644 +/- ##
==========================================
+ Coverage 79.45% 81.81% +2.36%
==========================================
Files 415 417 +2
Lines 41339 40801 -538
==========================================
+ Hits 32844 33383 +539
+ Misses 6621 6045 -576
+ Partials 1874 1373 -501
🚀 New features to boost your workflow:
|
Thank you for your contribution. Since PR titles and descriptions become part of our git commit history, please follow these recommendations: https://github.com/grpc/grpc-go/blob/master/CONTRIBUTING.md#pr-descriptions |
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.
Haven't reviewed the tests yet.
Signed-off-by: sotiris <[email protected]>
@davinci26 : Please do not mark comments as resolved. The process that we follow involves the reviewer marking them as resolved. When the author does that, the reviewer will have to click on each of those resolved comments to see what the original comment was and whether it was sufficiently addressed. Thanks for understanding. |
@easwars sorry, habit from a different process. Will keep them open and let the reviewer close them |
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.
I'm still looking at the test. Thanks.
Signed-off-by: sotiris <[email protected]>
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.
LGTM modulo minor nits in the test
Signed-off-by: sotiris <[email protected]>
Assigning to @mbissa for second set of eyes. |
@davinci26 : Thank you for your contribution! |
thank you and I will try to follow with adding the optional label. Do we want to open an issue for that since we are not 100% compliant right now? |
Thank you for offering. We have to wait for #8637 to be merged and I believe @mbissa is going to be out for a little while. We usually wait till we get two approvals on PRs from contributors who are not on the grpc team. And @mbissa is marked as the second reviewer. We might be able to get a different reviewer to approve this and have this merged. So, if that happens, you could just wait for the backend_service PR to get merged and add that optional label in a separate PR. We don't technically need an issue, but feel free to file one if you feel like it. |
Signed-off-by: sotiris <[email protected]>
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.
LGTM, thanks for the contribution!
Implements gRFC A91: https://github.com/grpc/proposal/blob/master/A91-outlier-detection-metrics.md
Notable implementation detals
grpc.lb.backend_service
is not implemented yet (marked as optional in the gRFC)enforced
/unenforced
without repeating the test setup.RELEASE NOTES: