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

Health check implementation (feature request #890) #1056

Merged
merged 15 commits into from
Oct 20, 2021

Conversation

zolkin
Copy link
Contributor

@zolkin zolkin commented Oct 5, 2021

This PR adds the health check functionality to both grpcweb and grpcwebproxy. The implementation is heavily based on existing grpc client health check implementation (see https://github.com/grpc/grpc-go/blob/master/health/client.go for more details)

Changes

Added health.go (exports ClientHealthCheck function) and health_test.go (tests for the new functionality) to grpcweb.
Added implementation of the health check functionality to the grpcwebproxy based on ClientHealthCheck function in grpcweb.
Added enable_health_check_service (default=false), enable_health_endpoint(default=false), health_endpoint_name ( default="_health"), health_service_name, (default="") run parameters for grpcwebproxy to control new health check functionality.

Verification

Apart from added tests manual testing was performed with grpcwebproxy to test whether proxy actually performs health checks and serves the results to the health endpoint.

@improbable-prow-robot improbable-prow-robot added the size/XL Denotes a PR that changes 300-599 lines, ignoring generated files. label Oct 5, 2021
Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

This CI is failing because of a go.mod diff as a result of installing the CI tools. I can fix it up if you give me permission to push to your branch, or you can fix it yourself.

go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
@zolkin
Copy link
Contributor Author

zolkin commented Oct 7, 2021

This CI is failing because of a go.mod diff as a result of installing the CI tools. I can fix it up if you give me permission to push to your branch, or you can fix it yourself.

Invited you as a collaborator, thanks.

evgenymikerinjpmc and others added 2 commits October 8, 2021 17:00
…implementation replaced with existing module, health check function simplified
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
Copy link
Contributor

@johanbrandhorst johanbrandhorst 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 your patience on this, the tests look much better like this, no need for sub tests, but I had a few thoughts still. I think the final step will be changing up main.go a little to improve the ergonomics of calling these little helpers.

go/grpcweb/health_test.go Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Outdated Show resolved Hide resolved
go/grpcwebproxy/main.go Show resolved Hide resolved
@zolkin
Copy link
Contributor Author

zolkin commented Oct 18, 2021

Thanks for your review work and useful suggestions, hopefully latest commits brought us close to merging the PR.

Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

We're very nearly there! Thanks for your patience, lets just get the last few test changes done and we can merge this 👍🏻.

go/grpcweb/health_test.go Outdated Show resolved Hide resolved
go/grpcweb/health_test.go Outdated Show resolved Hide resolved
zolkin and others added 2 commits October 19, 2021 10:58
Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Co-authored-by: Johan Brandhorst-Satzkorn <johan.brandhorst@gmail.com>
Copy link
Contributor

@johanbrandhorst johanbrandhorst left a comment

Choose a reason for hiding this comment

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

The code changes look good to me. The CI job is failing because we need to regenerate the docs for one of the package. Could you find and run generate-docs.sh please? It should hopefully just update a README or something.

@zolkin
Copy link
Contributor Author

zolkin commented Oct 20, 2021

Thanks Johan once again for useful suggestions throughout the review.

@johanbrandhorst
Copy link
Contributor

Seems like our test provider (SauceLabs) is being a bit slow. CC @MarcusLongmuir. I've rerun the jobs twice, hopefully they will all pass now.

@johanbrandhorst johanbrandhorst merged commit 502cb1e into improbable-eng:master Oct 20, 2021
@johanbrandhorst
Copy link
Contributor

Got there in the end, thanks for this contribution @zolkin!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL Denotes a PR that changes 300-599 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants