Skip to content

add logger to OutlierDetectionLoadBalancer#9880

Merged
temawi merged 8 commits intogrpc:masterfrom
s-matyukevich:s-matyukevich/add-outlierdetection-logger
Feb 14, 2023
Merged

add logger to OutlierDetectionLoadBalancer#9880
temawi merged 8 commits intogrpc:masterfrom
s-matyukevich:s-matyukevich/add-outlierdetection-logger

Conversation

@s-matyukevich
Copy link
Copy Markdown
Contributor

@s-matyukevich s-matyukevich commented Feb 7, 2023

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):

[2023-02-07 05:37:12 525] [FINEST ] [Channel<1>: (logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog:50051)] OutlierDetection lb created.

[2023-02-07 05:37:12 532] [FINEST ] [Channel<1>: (logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog:50051)] Received resolution result: ResolvedAddresses{addresses=[[[logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog/10.131.230.154:50051]/{}], [[logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog/10.135.101.119:50051]/{}], [[logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog/10.131.193.5:50051]/{}], [[logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog/10.135.43.140:50051]/{}]], attributes={io.grpc.internal.RetryingNameResolver.RESOLUTION_RESULT_LISTENER_KEY=io.grpc.internal.RetryingNameResolver$ResolutionResultListener@4612c26d}, loadBalancingPolicyConfig=io.grpc.util.OutlierDetectionLoadBalancer$OutlierDetectionLoadBalancerConfig@5c9a2c13} 

[2023-02-07 05:37:22 546] [FINEST ] [Channel<1>: (logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog:50051)] FailurePercentage algorithm detected outlier: AddressTracker{subchannels=[OutlierDetectionSubchannel{addresses=[[[logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog/10.135.101.119:50051]/{}]]}]}

Also, a similar message will be added for the SuccessRate algorithm.

[2023-02-07 05:37:22 546] [FINER  ] [Channel<1>: (logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog:50051)] Subchannel ejected: OutlierDetectionSubchannel{addresses=[[[logs-backend-rpc-test-server.service-discovery.all-clusters.local-dc.fabric.dog/10.135.101.119:50051]/{}]]}

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.

@temawi temawi self-requested a review February 7, 2023 15:56
@temawi temawi added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 7, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 7, 2023
temawi
temawi reviewed Feb 8, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@temawi temawi left a 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.

Comment thread core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java Outdated
temawi
temawi reviewed Feb 9, 2023
View reviewed changes
Comment thread core/src/main/java/io/grpc/util/OutlierDetectionLoadBalancer.java Outdated
double maxFailureRate = ((double)config.failurePercentageEjection.threshold) / 100;
if (tracker.failureRate() > maxFailureRate) {
logger.log(ChannelLogLevel.DEBUG,
"FailurePercentage algorithm detected outlier: {0}", tracker);
Copy link
Copy Markdown
Contributor

@temawi temawi Feb 9, 2023

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.

Copy link
Copy Markdown
Contributor Author

@s-matyukevich s-matyukevich Feb 9, 2023

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.

@s-matyukevich
Copy link
Copy Markdown
Contributor Author

s-matyukevich commented Feb 9, 2023

Right now the CI is failing with javadoc: error - Error fetching URL: https://developers.google.com/protocol-buffers/docs/reference/java/ which I don't think is realted to my change. I rebased on master and got the same error.

@s-matyukevich s-matyukevich force-pushed the s-matyukevich/add-outlierdetection-logger branch from b360b89 to 3f2638c Compare February 9, 2023 20:45
temawi
temawi approved these changes Feb 9, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@temawi temawi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@temawi temawi added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 9, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 9, 2023
@ejona86 ejona86 added the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 14, 2023
@grpc-kokoro grpc-kokoro removed the kokoro:run Add this label to a PR to tell Kokoro the code is safe and tests can be run label Feb 14, 2023
@temawi temawi merged commit 67d6600 into grpc:master Feb 14, 2023
@temawi
Copy link
Copy Markdown
Contributor

temawi commented Feb 14, 2023

Right now the CI is failing with javadoc: error - Error fetching URL: https://developers.google.com/protocol-buffers/docs/reference/java/ which I don't think is realted to my change. I rebased on master and got the same error.

@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.

@s-matyukevich s-matyukevich mentioned this pull request Mar 22, 2023
Merged
@github-actions github-actions Bot locked as resolved and limited conversation to collaborators May 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@temawi temawi temawi approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@s-matyukevich @temawi @ejona86 @grpc-kokoro