-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
add logger to OutlierDetectionLoadBalancer #9880
add logger to OutlierDetectionLoadBalancer #9880
Conversation
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.
Thanks for adding this. Generally looks good, I just had a question on one of the log statements.
core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java
Outdated
Show resolved
Hide resolved
@@ -814,6 +854,8 @@ public void ejectOutliers(AddressTrackerMap trackerMap, long ejectionTimeNanos) | |||
// If the failure rate is above the threshold, we should eject... | |||
double maxFailureRate = ((double)config.failurePercentageEjection.threshold) / 100; | |||
if (tracker.failureRate() > maxFailureRate) { | |||
logger.log(ChannelLogLevel.DEBUG, | |||
"FailurePercentage algorithm detected outlier: {0}", tracker); |
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.
Can we please get the parameters for the failure percentage algorithm logged the same way you did with success rate? After that I think we can get this merged.
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. b360b89
I added failureRate parameter to the FailurePercentage algorithm log message, as this is the only dynamically calculated parameter. I also added successRate to the SuccessRate algorithm log message for consistency.
Right now the CI is failing with |
…ection load balancer
b360b89
to
3f2638c
Compare
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.
Thank you!
@ejona pushed an empty commit to have the PR pick up the fix to the build from master. This is merged now, thanks for putting this together. |
Right now there is no visibility at all into what OutlierDetection lb is doing.
This PR is intended to fix this problem by adding ChannelLogger to the load balancer. Here are the different types of message that will be logged (taken from my end-to-end test):
Also, a similar message will be added for the SuccessRate algorithm.
A similar message will be added for the uneject event.
I used FINEST level everywhere except for actual ejection/unejection events, which are logged with FINER.
I also added the list of affected addresses to every log message, because this allows us to easily find misbehaving upstream server.