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

Add Plain HTTP Health Check Endpoint #832

Closed
krapie opened this issue Mar 31, 2024 · 13 comments · Fixed by #952 or #958
Closed

Add Plain HTTP Health Check Endpoint #832

krapie opened this issue Mar 31, 2024 · 13 comments · Fixed by #952 or #958
Assignees
Labels
enhancement 🌟 New feature or request good first issue 🐤 Good for newcomers

Comments

@krapie
Copy link
Member

krapie commented Mar 31, 2024

What would you like to be added:

Currently, our system only has the ConnectRPC health check endpoint grpc.health.v1.Health/Check, which requires an HTTP POST request with an application/json header and an empty body. This type of health checking is not convenient for common uptime checkers, especially free 3rd-party uptime checkers.

To address this limitation, we should implement a simple Plain HTTP health check endpoint that can be accessed with an HTTP HEAD request for easier and more versatile health checking.

Why is this needed:

The current health check method is not user-friendly for common uptime checkers. Introducing a Plain HTTP health check endpoint will make it easier for users to monitor the health status of our system.

@krapie krapie added the enhancement 🌟 New feature or request label Mar 31, 2024
@krapie
Copy link
Member Author

krapie commented Apr 6, 2024

We need to add to also support NCP ALB. @hackerwins
If we don't provide plain http health check endpoint, NCP ALB will exclude target from the load balancing target group.

실패 임계값: 정상 상태의 Target이 실패하면 비정상 상태로 전환되어 부하 분산 Target에서 제외되는 횟수

Reference: https://guide.ncloud-docs.com/docs/loadbalancer-targetgroup-vpc#2-health-check-%EC%84%A4%EC%A0%95

@krapie krapie added the good first issue 🐤 Good for newcomers label Apr 16, 2024
@taeng0204
Copy link
Contributor

Hello:) Could I work on this issue?

@devleejb
Copy link
Member

@taeng0204
Of course. If you have any questions, feel free to ask.

@krapie
Copy link
Member Author

krapie commented Jul 15, 2024

@taeng0204 If you want more context about this issue, please tell me.

@taeng0204
Copy link
Contributor

@krapie Are there any specific requirements or caveats to consider when setting up health checks in the Yorkie system?

@krapie
Copy link
Member Author

krapie commented Jul 16, 2024

@taeng0204 All we need to do is to implement HEAD or GET HTTP health check endpoint, but we want to minimize the change to implement this. Current ConnectRPC's health check endpoint requires POST method with body, but if we can tweak or change the behavior with something like interceptors, it will be great.

For more context, follow: https://bufbuild.slack.com/archives/CRZ680FUH/p1710158028178999

@taeng0204
Copy link
Contributor

@krapie Thanks for the answer, but I thought that link was to the slack comment and it seems to have expired.

@krapie
Copy link
Member Author

krapie commented Jul 16, 2024

@taeng0204 Can you check it again?

@taeng0204
Copy link
Contributor

taeng0204 commented Jul 16, 2024

@krapie Sorry, I tried a few times, but when I click on that link it says I can't see chats before April 17, 2024.
No chats are specifically focused on either.

https://bufbuild.slack.com/archives/CRZ680FUH/p1713373023394579
The chats on this link are the oldest chats I can see.

Can you give me the keywords for those chats? Then I'll do a search.

@krapie
Copy link
Member Author

krapie commented Jul 16, 2024

@taeng0204 Below is the screenshot of the thread.

image

@krapie
Copy link
Member Author

krapie commented Jul 27, 2024

@taeng0204 Any progress on this issue?

@taeng0204
Copy link
Contributor

@krapie
I'm having trouble resolving this issue.

There seem to be two ways to solve this issue.
The first, which I think is the best, is to support the GET method in connectrpc/grpchealth-go itself.
If you check the documentation below, you will see that the GET method is available in the connect protocol.

To do this, we need to make a setting in the proto file like this

Here's an example from connectrpc/grpchealth-go

// before
service Health {
  rpc Check(HealthCheckRequest) returns (HealthCheckResponse);
	...
}

// after
service Health {
  rpc Check(HealthCheckRequest) returns (HealthCheckResponse){
	  option idempotency_level = NO_SIDE_EFFECTS;
  }
	...
}

However, when I reached out to the Buf team via slack, I received the following response.

Do you have a good idea to convince the Buf team?
If not, I'd go with the second method, which is to implement a separate HTTP handler.

@krapie
Copy link
Member Author

krapie commented Aug 2, 2024

@taeng0204 I think it makes sense that gRPC don't have to consider about the HTTP method, because there is no "HTTP Method" concept in gRPC. And it seems like the Buf team is strictly following gRPC proto spec for the gRPC compatibility.

Let's discuss about this in the following weekly-sync session.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request good first issue 🐤 Good for newcomers
Projects
Status: Done
Status: Todo
3 participants