onHealthCheck is never called

[5amfung]

I followed https://www.apollographql.com/docs/apollo-server/v2/whats-new.html#Health-checks to add a health check, but in my health check, I reject intentionally just to make sure health check is called. It doesn't seem like the health check was called at all because my logging was never print and the endpoint .well-known/apollo/server-health always returns {"status":"pass"}.

const server = new ApolloServer({
  onHealthCheck: () =>
    new Promise((resolve, reject) => {
      //database check or other asynchronous action
      console.log('health check should failed but this is never called');
      reject('booooo');
    }),
  schema,
});

[rschlaefli]

I had the same problem (using express) and started looking at the code (https://github.com/apollographql/apollo-server/blob/master/packages/apollo-server-express/src/ApolloServer.ts). It seems like onHealthCheck needs to be passed to applyMiddleware instead of passing it into the ApolloServer constructor.

In my case, it ended up working like this:

const server = new ApolloServer({
  ...
});

server.applyMiddleware({
  app: server,
  onHealthCheck: () =>
    new Promise((resolve, reject) => {
      //database check or other asynchronous action
      console.log('health check should failed but this is never called');
      reject('booooo');
    }),
});

Hope this helps. If this is intended behavior, I might add a pull request for the docs?

[5amfung]

That makes sense if I'm using middleware with Express or Hapi. If I'm just using ApolloServer, I don't think I can used applyMiddleware().

[rschlaefli]

The health check functionality is probably only implemented for the integrations, as I can't find any mention of onHealthCheck in the code of core Apollo Server. Not sure if this is intentional..

[onpaws]

Just in case it helps others -- I approached this feature thinking this was a 'self-contained' or 'built-in' feature for Apollo Server, vs an 'out of band' feature.

In other words, I first thought passing this option would cause the the server to regularly call whatever function you pass to onHealthCheck and perhaps throw an exception, or log something, if the health check failed.

After a bit of experimentation tonight turns out that's not how it works. It appears to be intended as a piece of a bigger picture, e.g. a typical orchestration layer.
An outside agent will actually have to make a GET request to .well-known/apollo/server-health for onHealthCheck to fire. This aligns nicely with how e.g. Kubernetes built-in health checks work and is quite sensible in retrospect

Also perhaps of note to newcomers, Apollo Server won't crash or log errors if the health check fails - rather it appears to adjust the status response. It's up to you how to react to the failure state.

As the docs say you can manually check this by browsing to e.g.
http://localhost:4000/.well-known/apollo/server-health

[ryangardner]

The earlier commend and the labels added to this make me wonder if the assumption is that this issue is just a documentation issue?

I think it's more than that - the health checks are documented, they just don't work as documented

https://www.apollographql.com/docs/apollo-server/whats-new.html#Health-checks

const { ApolloServer, gql } = require('apollo-server');

const typeDefs = gql``;
const resolvers = {};

const server = new ApolloServer({
  typeDefs,
  resolvers,
  //optional parameter
  onHealthCheck: () =>
    new Promise((resolve, reject) => {
      //database check or other asynchronous action
    }),
});

server.listen().then(({ url }) => {
  console.log(`🚀 Server ready at ${url}`);
  console.log(
    `Try your health check at: ${url}.well-known/apollo/server-health`,
  );
});

Doesn't work at all - the health check function is not when you hit the .well-known/apollo/server-health endpoint

[gastonovic]

Is there any plans to resolve this issue ?

[abernix]

Fixed by #2672.

[baer]

I'm still having this issue with the latest (see below) version of apollo-server and apollo-server-express

    "apollo-server": "2.6.7",
    "apollo-server-express": "2.6.7",
    "apollo-server-plugin-response-cache": "0.2.4",

new ApolloServer({
  onHealthCheck: () => {
    // Log does not appear
    console.log("Hello out there!")
    // Health does not fail
    return Promise.reject();
  },
  schema: createSchema(enumValues),
  context: ({ req }) => {
    const user = req.user;

    return {
      user,
      models: createModels(dataModelCache)
    };
  },
  plugins: [responseCachePlugin()],
  formatError: error => {
    logger.error(error);
    return error;
  },
  debug: process.env.NODE_ENV !== `production`,
  playground: process.env.NODE_ENV !== `production`
  tracing: process.env.NODE_ENV !== `production`,
  cacheControl: {
    defaultMaxAge: TIME_IN_SECONDS.ONE_HOUR,
    stripFormattedExtensions: false
  }
});

Happy to help repro if needed. For now, I'm just defining a new route in Express to do the job.

[arizonatribe]

Yeah @abernix this wasn't fixed at all in that PR. There wasn't anything added in that PR which affected what Apollo does with a custom onErrorHandler.

In libraries like apollo-server-express for example, the onErrorHandler will only get invoked if we've passed it in through the applyMiddleware() function (the documentation stating it can be passed into the ApolloServer constructor is incorrect).

And even though we can pass in a custom onErrorHandler via applyMiddelware(), code like this is completely ignoring anything we write in a custom onHealthCheck handler:

if (onHealthCheck) {
  onHealthCheck(req)
    .then(() => {
      res.json({ status: 'pass' });
    })
    .catch(() => {
       res.status(503).json({ status: 'fail' });
    });
} else {
  res.json({ status: 'pass' });
}

Regardless if we return a promise in our onHealthCheck handler, there is literally nothing being done with whatever we return in our handler (ie .then(() =>). It simply returns { status: 'pass' } every time.

---

I think the package maintainers probably need to revisit exactly what the goal is for a custom onHealthCheck and then either update the docs to remove the support for a custom onErrorHandler or alter the code to support their decision on the strategy to take.

Should it allow you to create a custom JSON payload that will be returned instead of the default {status: "pass" }?

If so, then you might change it to this:

onHealthCheck(req)
  .then((details = {}) => {
    res.json({ status: 'pass', ...details });
  })
  .catch(() => {
    res.status(503).json({ status: 'fail' });
  });

Or is the goal to allow us to grab the connect middleware's req/res objects and decide what to do on our own.

If so then you might change it to this:

try {
  const result = await onHealthCheck(req, res)
  res.json({ status: 'pass', ...(result || {}) })
} catch (err) {
  res.status(503).json({ status: 'fail' });
}

And from what I can tell, the only reason a custom health check handler even gets looked at in the code is because of this line:

  public applyMiddleware({ app, ...rest }: ServerRegistration) {
    app.use(this.getMiddleware(rest));
  }

Nothing is actually being done in the constructor for the extended ApolloServer (each express, hapi, koa library would need to do this on its own), which is why it isn't being honored when passed into the constructor.

For the Express implementation you might do something like this in the constructor:

        super(config);
        if (config.onHealthCheck) {
          this.onHealthCheck = config.onHealthCheck
        } else {
          this.onHealthCheck = (req, res) => {
            res.json({ status: 'pass' });
          }
        }

and then inside the getMiddleware() method it would look kind of like this:

if (!disableHealthCheck) {
  router.use('/.well-known/apollo/server-health', async (req, res) => {
    res.type('application/health+json');
    try {
      const result = await (onHealthCheck || this.onHealthCheck)(req, res)
      if (!res.headersSent) {
        res.json({ status: 'pass', ...(result || {}) })
      }
    } catch (err) {
      res.status(503).json({ status: 'fail' });
    }
  });
}

You'll have to forgive me because I absolutely hate TypeScript and the kind of inheritance-heavy O.O. programming in the Apollo codebase, so this code would probably need to be altered to fit the desired syntactical "style". But hopefully it communicates the problem and possible solutions adequately.