-
Notifications
You must be signed in to change notification settings - Fork 2k
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
apollo-server-lambda: Implement onHealthCheck
on createHandler
.
#3458
Conversation
@MrGlenThomas: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
+1 |
1 similar comment
+1 |
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.
Thanks for opening this! I think there's a problem with the HTTP status code that's set during failure conditions, but other than that, thank you for putting this together!
.catch(() => { | ||
return callback(null, { | ||
body: JSON.stringify({ status: 'fail' }), | ||
statusCode: 200, |
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 think this status code should be 503 — do you agree?
statusCode: 200, | |
statusCode: 503, |
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 had thought that the health check should return 200 to signal that the request completed successfully and the body inspected for a result, otherwise the 503 could mean that the health check could not be reached (which isn't really that important to know). Having considered it again I think returning a 503 on failure does make sense, some health checks may work on status code alone and I have seen that this is how it has been implemented in the other Apollo packages.
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 can certainly understand that thinking. The bit about some health check implementations relying on the status code exclusively is the most relevant. Parsing JSON responses is not functionality that all probes/tools support (or it adds too much configuration to make it worthwhile).
For the above reason, and consistency with the other Apollo implementations (and documentation clarity!) I think we should go with 503 here, unless we think that won't play nice with AWS for some reason.
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 that's fine. Route 53 health checks require the endpoint to respond with an HTTP status code of 2xx or 3xx to signal healthy, so that will work.
I've also added a |
onHealthCheck
on createHandler
.
Fixes #3443
I have added the common health check functionality to the apollo-server-lambda package.
When the request path matches
/.well-known/apollo/server-health
then the lambda will execute the supplied onHealthCheck function and return the standard pass/fail response. If no onHealthCheck function is supplied then the lambda will return a default pass response to signal that the server is available.