health checks: configure path in apollo-server, doc better #5270

jutaz · 2021-06-03T15:52:51Z

Our health check feature is kind of bizarre. It doesn't actually check
that your GraphQL server can, well, serve GraphQL: it just checks if it
can serve HTTP. And the way it matches path names doesn't work very well
in contexts like Lambda where your whole server is often mounted at a
sub-path. In many cases, it would be better to just run a trivial
GraphQL operation as a health check.

Additionally, it's strange that we let you configure the functionality
of the health check in framework integrations, when it would be just as
easy to define your own handler in your framework.

I suppose it's nice that the health check exists in the
batteries-included apollo-server package, since you do often want to
have a health check (though again, running a GraphQL operation is often
better) and that package doesn't let you tweak HTTP serving directly.
And it's a bit of a shame that you can't set the health check path in
that package.

So, this PR:

Adds a healthCheckPath constructor option to the apollo-server
package. You can also pass null to disable the health check, which
wasn't previously possible with that package. This is based on work by
@jutaz though the PR has been thoroughly reworked since then by @glasser.
Reorganizes the ApolloServer reference docs so that the
apollo-server-specific options are all together, and documents both
the new option and the accidentally-undocumented onHealthCheck.
Rewrites the health check docs to encourage the use of GraphQL-level
health checks, and to separate guidance around HTTP-level health
checks in apollo-server (you can tweak it a bit if you want!) from
HTTP-level health checks in framework integration packages (if you're
doing something fancy just use the framework directly!)

Fixes #3577. Fixes #5731.

apollo-cla · 2021-06-03T15:52:53Z

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

jutaz · 2021-06-03T17:44:28Z

Tagging @abernix for review (can't assign reviewers)

glasser · 2021-06-07T20:46:52Z

Hmm. One thing that's a challenge is that paths in the middlewares are sometimes relative to where the middleware is applied to your server and sometimes not. But I guess just letting you swap out a hardcoded constant for a different one is reasonable.

I really wish that the health check was just a feature of apollo-server and people using a middleware integration just defined their own health check. It has very little to do with serving GraphQL and doesn't really fit with the rest of the project. Because of that, I'm tempted to say this new argument should only be an apollo-server thing, and if you want to customize your health check path with a different library then you can just define it in your own code.

jutaz · 2021-06-08T12:33:17Z

I'm tempted to say this new argument should only be an apollo-server thing, and if you want to customize your health check path with a different library then you can just define it in your own code.

I guess this boils down to - are we viewing apollo-server as the only "batteries included" solution and other vendor-specific packages are "bring your own implementation"? I'm certainly impartial to the solution (personally only care about apollo-server which I'm using), but I wonder if there's a reason to still keep the abstraction for health checks in other packages.

TL;DR - I'll happily change this implementation to whatever maintainers think is best suited implementation, just LMK 👍

glasser · 2021-06-09T18:35:27Z

That's my perspective for sure. So yeah, let's just make this apollo-server-specific?

jutaz · 2021-06-23T06:29:24Z

@glasser Sorry for a slow response on this one - I've changed this PR so that only apollo-server and apollo-server-express (which is required to have this change due to the fact that apollo-server relies on it internally) would expose the healthCheckPath

glasser · 2021-07-02T21:14:57Z

Planning to come back to this once 3.0.0 is released (it's in final release candidate state now).

gidich · 2021-09-24T18:03:42Z

@glasser - is this ready to merge in all checks are still green?

glasser · 2021-09-28T19:54:43Z

@gidich yep, got it on the queue (see label addition)

glasser

(oops meant to send this yesterday)

glasser · 2021-09-29T20:32:16Z

packages/apollo-server-express/src/ApolloServer.ts

@@ -15,6 +15,7 @@ export { GraphQLOptions } from 'apollo-server-core';

 export interface GetMiddlewareOptions {
  path?: string;
+  healthCheckPath?: string;


I don't really want to extend the interface for apollo-server-express; for the framework integration packages I'd rather leave this as a simple not super configurable feature which you can easily reimplement directly in your framework if you need more control. We can give this some sort of __internal_healthCheckPath name to make it clear that this isn't supported and doesn't need to be documented. I can make this change soon if you don't beat me to it.

glasser · 2021-10-01T00:07:00Z

I got a little carried away here and turned this into an opportunity to rewrite the docs and guidance around health checks.

jutaz · 2021-10-01T06:58:33Z

@glasser yup, makes sense - thanks for doing that!

trevor-scheer · 2021-10-04T18:15:37Z

packages/apollo-server-express/src/ApolloServer.ts

@@ -22,6 +22,16 @@ export interface GetMiddlewareOptions {
  bodyParserConfig?: OptionsJson | boolean;
  onHealthCheck?: (req: express.Request) => Promise<any>;
  disableHealthCheck?: boolean;
+  // There's no real point to allowing you to customize the health check path in
+  // an Apollo Server web framework integration package. You're already using
+  // Express --- just define a health check yourself! This option only exists to


Agree with the message but I think the way it's written might trivialize this a bit. Can we link out to some example or something?

I tweaked the wording a little. Not sure an example will help: it's just "make your web server do its normal thing".

@jutaz

Our health check feature is kind of bizarre. It doesn't actually check that your GraphQL server can, well, serve GraphQL: it just checks if it can serve HTTP. And the way it matches path names doesn't work very well in contexts like Lambda where your whole server is often mounted at a sub-path. In many cases, it would be better to just run a trivial GraphQL operation as a health check. Additionally, it's strange that we let you configure the functionality of the health check in framework integrations, when it would be just as easy to define your own handler in your framework. I suppose it's nice that the health check exists in the batteries-included `apollo-server` package, since you do often want to have a health check (though again, running a GraphQL operation is often better) and that package doesn't let you tweak HTTP serving directly. And it's a bit of a shame that you can't set the health check path in that package. So, this PR: - Adds a `healthCheckPath` constructor option to the `apollo-server` package. You can also pass `null` to disable the health check, which wasn't previously possible with that package. This is based on work by @jutaz though the PR has been thoroughly reworked since then. - Reorganizes the `ApolloServer` reference docs so that the `apollo-server`-specific options are all together, and documents both the new option and the accidentally-undocumented `onHealthCheck`. - Rewrites the health check docs to encourage the use of GraphQL-level health checks, and to separate guidance around HTTP-level health checks in `apollo-server` (you can tweak it a bit if you want!) from HTTP-level health checks in framework integration packages (if you're doing something fancy just use the framework directly!) Fixes apollographql#3577. Fixes apollographql#5731.

glasser added the 2021-09 label Sep 22, 2021

glasser reviewed Sep 30, 2021

View reviewed changes

glasser force-pushed the feature/configurable-health-check-path branch from fe2554c to 877b19e Compare October 1, 2021 00:06

glasser changed the title ~~Make the health-check path configurable.~~ health checks: configure path in apollo-server, doc better Oct 1, 2021

glasser assigned trevor-scheer Oct 1, 2021

glasser added 2021-10 and removed 2021-09 labels Oct 1, 2021

trevor-scheer reviewed Oct 4, 2021

View reviewed changes

trevor-scheer approved these changes Oct 4, 2021

View reviewed changes

glasser force-pushed the feature/configurable-health-check-path branch 2 times, most recently from e54fb5d to eb40ae0 Compare October 8, 2021 00:04

glasser force-pushed the feature/configurable-health-check-path branch from eb40ae0 to b006d97 Compare October 8, 2021 00:05

glasser merged commit a0ccf23 into apollographql:main Oct 8, 2021

jutaz deleted the feature/configurable-health-check-path branch June 10, 2022 12:59

github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

health checks: configure path in apollo-server, doc better #5270

health checks: configure path in apollo-server, doc better #5270

jutaz commented Jun 3, 2021 •

edited by glasser

Loading

apollo-cla commented Jun 3, 2021

jutaz commented Jun 3, 2021

glasser commented Jun 7, 2021

jutaz commented Jun 8, 2021

glasser commented Jun 9, 2021

jutaz commented Jun 23, 2021

glasser commented Jul 2, 2021

gidich commented Sep 24, 2021

glasser commented Sep 28, 2021

glasser left a comment

glasser Sep 29, 2021

glasser commented Oct 1, 2021

jutaz commented Oct 1, 2021

trevor-scheer Oct 4, 2021

glasser Oct 8, 2021

health checks: configure path in apollo-server, doc better #5270

health checks: configure path in apollo-server, doc better #5270

Conversation

jutaz commented Jun 3, 2021 • edited by glasser Loading

apollo-cla commented Jun 3, 2021

jutaz commented Jun 3, 2021

glasser commented Jun 7, 2021

jutaz commented Jun 8, 2021

glasser commented Jun 9, 2021

jutaz commented Jun 23, 2021

glasser commented Jul 2, 2021

gidich commented Sep 24, 2021

glasser commented Sep 28, 2021

glasser left a comment

Choose a reason for hiding this comment

glasser Sep 29, 2021

Choose a reason for hiding this comment

glasser commented Oct 1, 2021

jutaz commented Oct 1, 2021

trevor-scheer Oct 4, 2021

Choose a reason for hiding this comment

glasser Oct 8, 2021

Choose a reason for hiding this comment

jutaz commented Jun 3, 2021 •

edited by glasser

Loading