7.0.0 proposal - Removal of `@godaddy/terminus`

BrunnerLivio commented 5 years ago

Limitations

At the moment @nestjs/terminus is underlying some harsh limitations.

Some of which are:

Swagger compatibility #32
Compatibility with middlewares #33
OnApplicationShutdown hook #312
Get Health Status programmatically #9

The named issues are hardly or impossible to fix because of restrictions of the underlying @godaddy/terminus library. Surely, it would be possible to create pull requests to the underlying library, so the named issues would be possible. Though in that case there would not be much benefit of relying on the library at all.

Proposal

What I am proposing is the removal of the dependent @godaddy/terminus library.

The main points which is currently handled by @godaddy/terminus and would need to take over to the codebase of @nestjs/terminus:

Register the health check route to the http server (enabler of #32, #33) @godaddy/terminus registers health checks directly to the underlying http instance. The problem with that approach is, NestJS "does not know" these routes exist. Currently it is not possible to let NestJS know about these routes, and therefore there is no way to add Swagger, Middleware or else compatibility. Related issue nestjs/nest#1438.
Handle life-cycle management (enabler of #312) @godaddy/terminus registers listeners to system signals. Unfortunately NestJS should need to handle the system signals handling by itself, using onApplicationShutdown or beforeApplicationShutdown.
Handle the health indicator execution manager (enabler of #9) Currently @godaddy/terminus does not allow to compute the current outcome of the health status, without requesting the health check url.
Handle response codes on failure / succcess. No benefit in handling it ourselves, but it still allows for flexibility in the future.

Breaking changes

Most users most probably won't need to migrate. The only thing which would be beneficial for all users is to remove the @godaddy/terminus so your project does not have an unneeded dependency (npm uninstall @godaddy/terminus).

For all users who have built custom health indicators will need to change how you had to handle unhealthy response codes. Essentially it boils down to changing the following line:

import { HealthCheckError } from '@godaddy/terminus';

to

import { HealthCheckError } from '@nestjs/terminus';

and that's it.

Since this is a breaking change, I want to align with you before-hand. Please let me know whether my strategy is sufficient for your project. Maybe you have a better idea? Let me know :)

iangregsondev commented 5 years ago

@BrunnerLivio I am using 2 custom indicators that i created (using the doc recipes), so doing the above seems fine for me.

Here is an example of an Indicator right now

@Injectable()
export class ExchangeRateRedisIndicator extends HealthIndicator {
  constructor(private readonly serverSharedExchangeRateRedisService: ServerSharedExchangeRateRedisService) {
    super()
  }
  async isHealthy(): Promise<HealthIndicatorResult> {
    const healthy = !this.serverSharedExchangeRateRedisService.hasConnectionError()
    let message: string | undefined = undefined
    if (!healthy) {
      message = this.serverSharedExchangeRateRedisService.connectionErrorMessage()
    }
    const result = this.getStatus("exchange_rate_redis_connection", healthy, { message })

    if (healthy) {
      return result
    }
    throw new HealthCheckError("ExchangeRateRedisIndicator failed", result)
  }
}

I am not using anything special apart from what is discussed in the recipes.

Thanks.

eropple commented 5 years ago

I like the idea of ditching @godaddy/terminus, but given the breaking changes you suggest I'd suggest not calling this terminus at all--that may lead to confusion. @nestjs/health-checks, perhaps?

eropple commented 5 years ago

Also, pursuant to these changes I'd like to work with you on surfacing information from this library into @eropple/nestjs-openapi3; please feel free to reach out to me via email or Discord (Ed#7871).

BrunnerLivio commented 4 years ago

@eropple

given the breaking changes you suggest I'd suggest not calling this terminus at all--that may lead to confusion. @nestjs/health-checks, perhaps?

@nestjs/terminus does more than just provide health checks in the future. It will provide a service or similar for handling graceful shutdown. In my opinion we should keep the name to minimize the migration steps.

To keep you aligned with the strategy & status of this proposal

Supposedly no blockers present (beforeApplicationShutdown-feature has been merged on nestjs/nest, so that would enable this proposal)
To be aligned with the other packages from NestJS we will release this major change once 7.0.0 has been release on nestjs/nest. Unfortunately we do not have a timeline for this yet, but most probably it will happen soon, since we already have proposed breaking changes on nestjs/nest and nestjs/swagger.
Because there is no timeline for 7.0.0, I will try to push forward on nestjs/terminus and release a prerelease which runs on 6.x.x, but already includes the breaking changes. (e.g. @nestjs/terminus@next)

Currently I am fully consumed by my day-to-day job unfortunately. I try to take a day off, or work on weekends to make this happening though. Feel free to contact me on Discord, if you want to contribute to this proposal, but I think the easiest way to implement the initial steps for this proposal would require me.

I'll try to be as transparent as possible with this breaking change, so it hopefully runs down smoothly. :beer:

BrunnerLivio commented 4 years ago

Resolved in 7.0.0 :tada:

nestjs / terminus