-
Notifications
You must be signed in to change notification settings - Fork 113
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
Node.js: Adding health check endpoints for a Kubernetes environment #749
Conversation
I'm curious why you need these specific paths exposed in the samples. Can you expand on that? The samples already have health endpoints. They're also pretty simple so liveness/readiness are really the same thing. Adding more paths and options makes the sample apps more complicated. Just trying to weigh the pros/cons here. Thanks |
nodejs/no-package-manager/server.js
Outdated
</body> | ||
</html>`); | ||
} else if ( | ||
request.url === "/actuator/health/readiness" && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just wondering how you chose those endpoints. I was looking at https://nodeshift.dev/nodejs-reference-architecture/operations/healthchecks/#endpoints and from that we might use /readyz and /livez
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess maybe it is to match the Java examples?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, to match the routes from the java example
The node.js examples do not have a readiness or a liveness endpoints. |
That's unnecessary though. As it says in the K8s docs...
So you could configure your probes like:
or you could just point them to the same place, all that matters is that you point them to an actual endpoint in the application. Since the sample apps don't treat liveness/readiness different, because of their simplicity, I'm not sure I see why we should introduce additional endpoints.
If you would like to put example configurations for K8s into the READMEs that would be great. Showing people how to configure K8s for the samples would be awesome, but I don't personally see a need for adding additional endpoints to the apps. |
At one point I thought that some of the examples (the Java ones) already had health endpoints but that turns out not to be the case. In that context I don't have a strong feeling one way or the other in terms of adding them. It could make it easier/better to use the samples in the context of deployment to Kubernetes environments, but its pretty easy to copy the examples when that is needed. |
I will add though, that if we do want to show configuring the endpoints then I don't think we should be pointing them to the app endpoint and we'd want to use different endpoints for readiness and liveness. |
I see/agree @dmikusa, doesn't make sense to add more routes to the Java example than is unnecessary once you can configure it at a Kubernetes level and also breaks the consistency across the rest of the Java examples For the node apps it would be nice to have a +1 for adding the kubernetes configuration to the docs. Thank you for your feedback :) |
That sounds good to me. Making the samples more consistent across languages is 👍 |
765df1d
to
8b6b29c
Compare
Adding the single endpoint to the Node.js one to be consistent with the Java example sounds good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@dmikusa It has three approvals, should we merge it? |
Summary
This PR adds the
/actuator/health
health check endpoint on the Node.js applications.This PR adds also a logging function for the Node.js apps to log any incoming requests, which is helpful to know whether the request has been received by the server.
Use Cases
Checklist