Skip to content
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

xds/internal/balancer/outlierdetection: Add Channelz Logger to Outlier Detection LB #6145

Merged

Conversation

s-matyukevich
Copy link
Contributor

@s-matyukevich s-matyukevich commented Mar 22, 2023

This PR replicates grpc/grpc-java#9880 from grpc-java, which adds additional logging to the outlierDetection LB to improve observability.

Here are the additional messages that will be logged (taken from my end-to-end test):

2023-03-22T21:42:53.659526+00:00 DEBUG (outlierdetection/balancer.go:852) - [xds] [Channel #1] FailurePercentage algorithm detected outlier: [[{
  "Addr": "10.135.22.187:3000",
  "ServerName": "",
  "Attributes": null,
  "BalancerAttributes": null,
  "Type": 0,
  "Metadata": null 
}]], failurePercentage=100.000000

And a similar message for the success rate algorithm

2023-03-22T21:42:53.659588+00:00 DEBUG (outlierdetection/balancer.go:868) - [xds] [Channel #1] Subchannel ejected: [{
  "Addr": "10.135.22.187:3000",
  "ServerName": "",
  "Attributes": null,
  "BalancerAttributes": null,
  "Type": 0,
  "Metadata": null 
}]

And a similar message for the unejection event.

It would be nice to use sub-channel logger, but this would require passing sub-channel channelz ID to the balancer, which is currently not possible and requires changing SubConn interface.

RELEASE NOTES:

  • xds/internal/balancer/outlierdetection: Add Channelz Logger to Outlier Detection LB

@s-matyukevich s-matyukevich changed the title Add logger to outlierDetection LB Add logger to the OutlierDetection LB Mar 22, 2023
@s-matyukevich s-matyukevich force-pushed the s-matyukevich/outlier-detection-logger branch from 1b8d781 to 0aad338 Compare March 22, 2023 23:07
@atollena
Copy link
Contributor

cc @zasweq

@zasweq zasweq self-requested a review March 23, 2023 17:20
@zasweq zasweq self-assigned this Mar 23, 2023
@s-matyukevich
Copy link
Contributor Author

@zasweq Any updates on this?

@zasweq
Copy link
Contributor

zasweq commented Apr 11, 2023

Taking a look now. Sorry for the delay.

Copy link
Contributor

@zasweq zasweq 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 the contribution. Sorry for the late pass. Looks great outside of a few nits.

Comment on lines 902 to 910
func (a *addressInfo) string() string {
res := "["
for _, sw := range a.sws {
res += sw.string()
}
res += "]"
return res
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please use a strings.Builder here for efficiency. See #5654, and the second answer here: https://stackoverflow.com/questions/1760757/how-to-efficiently-concatenate-strings-in-go/47798475#47798475.

Comment on lines 821 to 822
channelz.Infof(logger, b.channelzParentID, "SuccessRate algorithm detected outlier: %s. "+
"Parameters: successRate=%f, mean=%f, stddev=%f, requiredSuccessRate=%f",
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you get rid of the + and put this on one line please.

@zasweq zasweq added this to the 1.55 Release milestone Apr 11, 2023
@zasweq zasweq changed the title Add logger to the OutlierDetection LB xds/internal/balancer/outlierdetection: Add Channelz Logger to Outlier Detection LB Apr 11, 2023
@zasweq zasweq assigned s-matyukevich and unassigned zasweq Apr 11, 2023
@arvindbr8 arvindbr8 modified the milestones: 1.55 Release, 1.56 Release Apr 18, 2023
@github-actions
Copy link

This PR is labeled as requiring an update from the reporter, and no update has been received after 6 days. If no update is provided in the next 7 days, this issue will be automatically closed.

@github-actions github-actions bot added the stale label Apr 24, 2023
Copy link
Contributor

@zasweq zasweq left a 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!

@zasweq zasweq merged commit 8628e07 into grpc:master Apr 25, 2023
11 checks passed
Copy link
Contributor

@dfawley dfawley left a comment

Choose a reason for hiding this comment

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

LGTM; @zasweq will follow-up to see if string() should be changed to String()

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants