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

apollo-server-azure-functions: Health checks implementation #5003

Merged

Conversation

vany0114
Copy link
Contributor

@vany0114 vany0114 commented Mar 7, 2021

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.

@apollo-cla
Copy link

@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/

@bennypowers
Copy link

bennypowers commented Mar 9, 2021

I'm implementing in my project with the following patch:

diff
diff --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 function.json

{
  "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"
    }
  ]
}

@glasser
Copy link
Member

glasser commented Mar 25, 2021

@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?

@bennypowers
Copy link

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.

@glasser
Copy link
Member

glasser commented Mar 25, 2021

@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?

@bennypowers
Copy link

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 Co-Authored-By. If this pr is still in flight after Passover I can review OP's tests in light of my patch.

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

@vany0114
Copy link
Contributor Author

vany0114 commented Mar 25, 2021

@beardedtim Oh I see you're using the application/health+json header which makes total sense, I can change it in my PR. On the other hand, I think you're right regarding the function.json file (the "route": "{*segments}" setting) that should be present in the docs.

@vany0114
Copy link
Contributor Author

Done

@glasser
Copy link
Member

glasser commented Apr 6, 2021

Hi, I'm still not sure about what you're saying about the function.json change. Does there need to be an update to docs/source/deployment/azure-functions.md and/or packages/apollo-server-azure-functions/README.md to mention this function.json change?

Also, are you able to resolve conflicts?

@vany0114
Copy link
Contributor Author

vany0114 commented Apr 7, 2021

@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.
Copy link
Contributor Author

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?

Copy link
Member

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.

Copy link
Contributor Author

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!

@vany0114
Copy link
Contributor Author

vany0114 commented May 7, 2021

@glasser any plans to merge this soon?

@glasser
Copy link
Member

glasser commented May 7, 2021

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.

@abbeyciolek
Copy link

@glasser are there any updates on this PR?

@glasser
Copy link
Member

glasser commented Sep 22, 2021

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!

@abbeyciolek
Copy link

@glasser Thank you! Is this change still on track for this week?

@glasser
Copy link
Member

glasser commented Oct 1, 2021

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 main.

@glasser glasser removed the 2021-09 label Oct 1, 2021
@vany0114
Copy link
Contributor Author

vany0114 commented Oct 3, 2021

@glasser I solved the conflicts. I also ran the prettier command however it still shows me an error that does not even make sense, any idea what I'm missing?

image

@glasser glasser self-assigned this Oct 5, 2021
@glasser glasser added the 2021-10 label Oct 5, 2021
@glasser
Copy link
Member

glasser commented Oct 5, 2021

override is a new-ish feature of TypeScript so you probably haven't run npm install enough or something?

@vany0114
Copy link
Contributor Author

vany0114 commented Oct 5, 2021

override is a new-ish feature of TypeScript so you probably haven't run npm install enough or something?

@glasser would you mind running the prettier --write for me? I'm having issues building locally.

@glasser
Copy link
Member

glasser commented Oct 5, 2021

Sure.

@glasser glasser force-pushed the vany0114/azfunctions-healthcheck branch from ee39b84 to 645bc2d Compare October 8, 2021 20:55
if (req.url?.endsWith('/.well-known/apollo/server-health')) {
const successfulResponse = {
body: JSON.stringify({ status: 'pass' }),
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.

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).
@glasser glasser force-pushed the vany0114/azfunctions-healthcheck branch from da8aecc to 7e0eec0 Compare October 8, 2021 23:20
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants