-
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-azure-functions: Health checks implementation #5003
apollo-server-azure-functions: Health checks implementation #5003
Conversation
@vany0114: 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/ |
I'm implementing in my project with the following patch: diffdiff --git a/node_modules/apollo-server-azure-functions/.DS_Store b/node_modules/apollo-server-azure-functions/.DS_Store
new file mode 100644
index 0000000..c88a062
Binary files /dev/null and b/node_modules/apollo-server-azure-functions/.DS_Store differ
diff --git a/node_modules/apollo-server-azure-functions/dist/ApolloServer.d.ts b/node_modules/apollo-server-azure-functions/dist/ApolloServer.d.ts
index f8712a4..783bbbf 100644
--- a/node_modules/apollo-server-azure-functions/dist/ApolloServer.d.ts
+++ b/node_modules/apollo-server-azure-functions/dist/ApolloServer.d.ts
@@ -1,5 +1,5 @@
import { Context, HttpRequest } from '@azure/functions';
-import { ApolloServerBase } from 'apollo-server-core';
+import { ApolloServerBase, Config as ConfigBase } from 'apollo-server-core';
import { GraphQLOptions } from 'apollo-server-core';
export interface CreateHandlerOptions {
cors?: {
@@ -11,8 +11,13 @@ export interface CreateHandlerOptions {
maxAge?: number;
};
}
+export interface Config extends ConfigBase {
+ onHealthCheck?: (context: Context, request: HttpRequest) => Promise<any>;
+}
+
export declare class ApolloServer extends ApolloServerBase {
protected serverlessFramework(): boolean;
+ constructor(config: Config);
createGraphQLServerOptions(request: HttpRequest, context: Context): Promise<GraphQLOptions>;
createHandler({ cors }?: CreateHandlerOptions): (context: Context, req: HttpRequest) => void;
}
diff --git a/node_modules/apollo-server-azure-functions/dist/ApolloServer.js b/node_modules/apollo-server-azure-functions/dist/ApolloServer.js
index e95c87a..ec4820c 100644
--- a/node_modules/apollo-server-azure-functions/dist/ApolloServer.js
+++ b/node_modules/apollo-server-azure-functions/dist/ApolloServer.js
@@ -10,6 +10,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
};
Object.defineProperty(exports, "__esModule", { value: true });
exports.ApolloServer = void 0;
+const { URL } = require('url');
const apollo_server_core_1 = require("apollo-server-core");
const graphql_playground_html_1 = require("@apollographql/graphql-playground-html");
const azureFunctionApollo_1 = require("./azureFunctionApollo");
@@ -20,6 +21,10 @@ class ApolloServer extends apollo_server_core_1.ApolloServerBase {
createGraphQLServerOptions(request, context) {
return super.graphQLServerOptions({ request, context });
}
+ constructor(config) {
+ super(config);
+ this.onHealthCheck = config && config.onHealthCheck;
+ }
createHandler({ cors } = { cors: undefined }) {
const promiseWillStart = this.willStart();
const corsHeaders = {};
@@ -79,18 +84,55 @@ class ApolloServer extends apollo_server_core_1.ApolloServerBase {
});
return;
}
- if (this.playgroundOptions && req.method === 'GET') {
- const acceptHeader = req.headers['Accept'] || req.headers['accept'];
- if (acceptHeader && acceptHeader.includes('text/html')) {
- const path = req.url || '/';
- const playgroundRenderPageOptions = Object.assign({ endpoint: path }, this.playgroundOptions);
- const body = graphql_playground_html_1.renderPlaygroundPage(playgroundRenderPageOptions);
- context.done(null, {
- body: body,
- status: 200,
- headers: Object.assign({ 'Content-Type': 'text/html' }, corsHeaders),
- });
- return;
+ if (req.method === 'GET') {
+ const url = new URL(req.url);
+ if (url.pathname.endsWith('/.well-known/apollo/server-health')) {
+ // Response follows https://tools.ietf.org/html/draft-inadarei-api-health-check-01
+ if (typeof this.onHealthCheck === 'function') {
+ this.onHealthCheck(context, req)
+ .then(() => {
+ context.done(null, {
+ body: JSON.stringify({ status: 'pass' }),
+ status: 200,
+ headers: {
+ 'Content-Type': 'application/health+json'
+ }
+ });
+ })
+ .catch(() => {
+ context.done(null, {
+ body: JSON.stringify({ status: 'fail' }),
+ status: 503,
+ headers: {
+ 'Content-Type': 'application/health+json'
+ }
+ });
+ });
+ return;
+ } else {
+ context.done(null, {
+ body: JSON.stringify({ status: 'pass' }),
+ status: 200,
+ headers: {
+ 'Content-Type': 'application/health+json'
+ }
+ });
+ return;
+ }
+ }
+ else if (this.playgroundOptions) {
+ const acceptHeader = req.headers['Accept'] || req.headers['accept'];
+ if (acceptHeader && acceptHeader.includes('text/html')) {
+ const path = req.url || '/';
+ const playgroundRenderPageOptions = Object.assign({ endpoint: path }, this.playgroundOptions);
+ const body = graphql_playground_html_1.renderPlaygroundPage(playgroundRenderPageOptions);
+ context.done(null, {
+ body: body,
+ status: 200,
+ headers: Object.assign({ 'Content-Type': 'text/html' }, corsHeaders),
+ });
+ return;
+ }
}
}
const callbackFilter = (error, output) => {
diff --git a/node_modules/apollo-server-azure-functions/src/ApolloServer.ts b/node_modules/apollo-server-azure-functions/src/ApolloServer.ts
index c0ee393..ceaea20 100644
--- a/node_modules/apollo-server-azure-functions/src/ApolloServer.ts
+++ b/node_modules/apollo-server-azure-functions/src/ApolloServer.ts
@@ -1,6 +1,7 @@
import { Context, HttpRequest } from '@azure/functions';
import { HttpResponse } from 'azure-functions-ts-essentials';
-import { ApolloServerBase } from 'apollo-server-core';
+import { ApolloServerBase, Config as ConfigBase } from 'apollo-server-core';
+import { URL } from 'url';
import { GraphQLOptions } from 'apollo-server-core';
import {
renderPlaygroundPage,
@@ -20,11 +21,22 @@ export interface CreateHandlerOptions {
};
}
+export interface Config extends ConfigBase {
+ onHealthCheck?: (context: Context, request: HttpRequest) => Promise<any>;
+}
+
export class ApolloServer extends ApolloServerBase {
protected serverlessFramework(): boolean {
return true;
}
+ private onHealthCheck?: (context: Context, request: HttpRequest) => Promise<any>;
+
+ constructor(config: Config) {
+ super(config);
+ this.onHealthCheck = config && config.onHealthCheck;
+ }
+
// This translates the arguments from the middleware into graphQL options It
// provides typings for the integration specific behavior, ideally this would
// be propagated with a generic to the super class
@@ -110,25 +122,61 @@ export class ApolloServer extends ApolloServerBase {
return;
}
- if (this.playgroundOptions && req.method === 'GET') {
- const acceptHeader = req.headers['Accept'] || req.headers['accept'];
- if (acceptHeader && acceptHeader.includes('text/html')) {
- const path = req.url || '/';
-
- const playgroundRenderPageOptions: PlaygroundRenderPageOptions = {
- endpoint: path,
- ...this.playgroundOptions,
- };
- const body = renderPlaygroundPage(playgroundRenderPageOptions);
- context.done(null, {
- body: body,
- status: 200,
- headers: {
- 'Content-Type': 'text/html',
- ...corsHeaders,
- },
- });
- return;
+ if (req.method === 'GET') {
+ const url = new URL(req.url);
+ if (url.pathname.endsWith('/.well-known/apollo/server-health')) {
+ // Response follows https://tools.ietf.org/html/draft-inadarei-api-health-check-01
+ if (typeof this.onHealthCheck === 'function') {
+ this.onHealthCheck(context, req)
+ .then(() => {
+ context.done(null, {
+ body: JSON.stringify({ status: 'pass' }),
+ status: 200,
+ headers: {
+ 'Content-Type': 'application/health+json'
+ }
+ });
+ })
+ .catch(() => {
+ context.done(null, {
+ body: JSON.stringify({ status: 'fail' }),
+ status: 503,
+ headers: {
+ 'Content-Type': 'application/health+json'
+ }
+ });
+ });
+ return;
+ } else {
+ context.done(null, {
+ body: JSON.stringify({ status: 'pass' }),
+ status: 200,
+ headers: {
+ 'Content-Type': 'application/health+json'
+ }
+ });
+ return;
+ }
+ } else if (this.playgroundOptions) {
+ const acceptHeader = req.headers['Accept'] || req.headers['accept'];
+ if (acceptHeader && acceptHeader.includes('text/html')) {
+ const path = req.url || '/';
+
+ const playgroundRenderPageOptions: PlaygroundRenderPageOptions = {
+ endpoint: path,
+ ...this.playgroundOptions,
+ };
+ const body = renderPlaygroundPage(playgroundRenderPageOptions);
+ context.done(null, {
+ body: body,
+ status: 200,
+ headers: {
+ 'Content-Type': 'text/html',
+ ...corsHeaders,
+ },
+ });
+ return;
+ }
}
} In order for this to work, I had to use the following {
"disabled": false,
"scriptFile": "../dist/server/graphql/index.js",
"bindings": [
{
"authLevel": "anonymous",
"type": "httpTrigger",
"direction": "in",
"name": "req",
"route": "{*segments}",
"methods": [
"get",
"post"
]
},
{
"type": "http",
"direction": "out",
"name": "$return"
}
]
} |
@bennypowers Are you suggesting that your change is better than this PR? Should I be reviewing that as a PR instead of this one? Want to file a PR and explain the tradeoffs between yours and this one? |
I can send over a pr some time in the next two weeks or so. If OP is interested, they are welcome to implement here. |
@bennypowers sure, I'm just curious, does your PR work better than this one for some reason? ie, should I avoid reviewing and merging this one? |
OP has tests, which my patch lacks But my patch has the right Content-Type header In terms of impl details I can't say whether mine is any better than op's. I leave it to reviewers to make that judgement. If parts of my version are adopted in the final merged commit please consider using With either version, i'd also love to see a documentation commit with her necessary changes to function.json, as the az func won't work without it |
@beardedtim Oh I see you're using the |
Done |
Hi, I'm still not sure about what you're saying about the function.json change. Does there need to be an update to Also, are you able to resolve conflicts? |
@glasser I updated the docs as well as resolved the conflicts. |
@@ -112,6 +112,8 @@ It is important to set output binding name to **$return** to work correctly at t | |||
} | |||
``` | |||
|
|||
**Note:** To enable the health checks it's necessary to [modify the route template](../../../packages/apollo-server-azure-functions/README.md#enabling-health-checks) in the `function.json` file. |
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.
@glasser it seems this broke the docs build, do you know how can I link it or should I just replicate the documentation here?
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.
We don't put anything outside of docs
in the docs site, so I'd recommend just linking to the README via a GitHub main branch or npmjs.com link. Replicating sounds good too though.
I'm hoping that as part of the AS3 push we can improve the integration/deployment docs to be more complete and authoritative.
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.
@glasser done, all the checks are passing now!
@glasser any plans to merge this soon? |
It's on my list of incoming PRs to review, but right now I'm heads down on getting Apollo Server 3 out the door. |
@glasser are there any updates on this PR? |
Sorry, there've been a bit of distractions lately (lots of late summer vacations etc) but it's on the queue still and I intend to get a lot of things on that queue done next week! |
@glasser Thank you! Is this change still on track for this week? |
This PR appears to still be based on top of Apollo Server 2. I'd be happy to review it (it does make sense to bring this integration to parity with other integrations) once it's rebased on top of current |
@glasser I solved the conflicts. I also ran the |
|
@glasser would you mind running the |
Sure. |
ee39b84
to
645bc2d
Compare
if (req.url?.endsWith('/.well-known/apollo/server-health')) { | ||
const successfulResponse = { | ||
body: JSON.stringify({ status: 'pass' }), | ||
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.
The existing code in this PR uses status
, not statusCode
. Do both actually work? Changing to status
for consistency.
Change statusCode to status because that appears to be what Azure wants and what we do elsewhere. Make the apollo-server-azure-functions README mostly point to the docs site rather than be another thing that needs to be kept up to date (most other packages have this already).
da8aecc
to
7e0eec0
Compare
This PR implements health checks for
apollo-server-azure-functions
package.Issue: #4925
CC: @glasser
Note: I wasn't able to test this locally since I couldn't build the whole repo, however tests are passing.