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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 4 commits into from Apr 25, 2023
Merged

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

merged 4 commits into from Apr 25, 2023

Conversation

s-matyukevich
Copy link
Contributor

@s-matyukevich s-matyukevich commented Mar 22, 2023 •
edited by zasweq

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
Collaborator

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.

zasweq
zasweq requested changes Apr 11, 2023
View reviewed changes
Copy link
Contributor

@zasweq zasweq left a comment •
edited

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.

xds/internal/balancer/outlierdetection/balancer.go Outdated
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.

xds/internal/balancer/outlierdetection/balancer.go Outdated
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 the Type: Feature New features or improvements in behavior label Apr 11, 2023
@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
zasweq
zasweq approved these changes Apr 25, 2023
View reviewed changes
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
dfawley
dfawley reviewed Apr 25, 2023
View reviewed changes
Copy link
Member

@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()

@zasweq zasweq mentioned this pull request Apr 25, 2023
Merged
@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.
Reviewers

@atollena atollena atollena left review comments

@dfawley dfawley dfawley left review comments

@zasweq zasweq zasweq approved these changes

Assignees

@s-matyukevich s-matyukevich

Labels
stale Status: Requires Reporter Clarification Type: Feature New features or improvements in behavior
Projects
None yet
Milestone
1.56 Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@s-matyukevich @atollena @zasweq @dfawley @arvindbr8