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

apollo-server-lambda: Implement onHealthCheck on createHandler. #3458

Merged
merged 6 commits into from
Nov 13, 2019
Merged

apollo-server-lambda: Implement onHealthCheck on createHandler. #3458

Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
885b61a
Added health check for apollo-server-lambda
glenthomas Oct 29, 2019
bdb8c85
Set onHealthCheck parameter
glenthomas Oct 29, 2019
7f7d11b
Merge branch 'master' into master
abernix Nov 13, 2019
611cd81
Add CHANGELOG.md for #3458.
abernix Nov 13, 2019
615dc28
Use status code 503 on `onHealthCheck` rejection, rather than 200.
abernix Nov 13, 2019
a330007
Merge branch 'master' into master
abernix Nov 13, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion packages/apollo-server-lambda/src/ApolloServer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export interface CreateHandlerOptions {
credentials?: boolean;
maxAge?: number;
};
onHealthCheck?: (req: APIGatewayProxyEvent) => Promise<any>;
}

export class ApolloServer extends ApolloServerBase {
Expand All @@ -47,7 +48,7 @@ export class ApolloServer extends ApolloServerBase {
return super.graphQLServerOptions({ event, context });
}

public createHandler({ cors }: CreateHandlerOptions = { cors: undefined }) {
public createHandler({ cors, onHealthCheck }: CreateHandlerOptions = { cors: undefined, onHealthCheck: undefined }) {
// We will kick off the `willStart` event once for the server, and then
// await it before processing any requests by incorporating its `await` into
// the GraphQLServerOptions function which is called before each request.
Expand Down Expand Up @@ -159,6 +160,35 @@ export class ApolloServer extends ApolloServerBase {
});
}

if (event.path === '/.well-known/apollo/server-health') {
const successfulResponse = {
body: JSON.stringify({ status: 'pass' }),
statusCode: 200,
headers: {
'Content-Type': 'application/json',
...requestCorsHeadersObject,
},
};
if (onHealthCheck) {
onHealthCheck(event)
.then(() => {
return callback(null, successfulResponse);
})
.catch(() => {
return callback(null, {
body: JSON.stringify({ status: 'fail' }),
statusCode: 200,
Copy link
Member

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?

Suggested change
statusCode: 200,
statusCode: 503,

Copy link
Contributor

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.

Copy link
Member

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.

Copy link
Contributor

@Glen-Moonpig Glen-Moonpig Nov 13, 2019 •
edited
Loading

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.

headers: {
'Content-Type': 'application/json',
...requestCorsHeadersObject,
},
});
});
} else {
return callback(null, successfulResponse);
}
}

if (this.playgroundOptions && event.httpMethod === 'GET') {
const acceptHeader = event.headers['Accept'] || event.headers['accept'];
if (acceptHeader && acceptHeader.includes('text/html')) {
Expand Down