Closed weeco closed 6 years ago
It's a good idea to provide such functionality out-of-the-box. Do you have any API proposal? :)
thinking about module API, I think it should be more abstract, just accepting list of checkers to be verified in controller route method:
I see 2 basic implementations ideas:
type Checker = { name: string; callback: (...args) => any };
export class HealthCheckerService { private registry: Map<string, Checker> = new Map(); public registerChecker(opts: Checker) { // adding to the registry by hashed-name or just name const nameHash = someHashFunc(opts.name); this.registry.set(nameHash, callback); }
public check() {
const errors: string[] = [];
//await Promise.map(this.registry.values(), async checker => {
try {
await checker.callback();
} catch (err) {
console.error(Executing ${checker.name} failed:
, err);
errors.push(obtainedErrorMessage);
}
});
}
}
@Controller() class HealthCheckController { public constructor(@Inject('checker_token') private readonly healthcheckerService: HealthCheckerService) {} @Get() public async check() { const errors = this.healthcheckerService.check();
if (!!errors.length) {
return InternalServerErrorException(errors); // or more configured low-level response, like failStatusCode or whatever
}
} }
export class HealthCheckModule { static forRoot(opts: { statusEndpointUrl?: string }): DynamicModule { const controllerPath = opts.statusEndpointUrl || "/health"; Reflect.defineMetadata(constants_1.PATH_METADATA, controllerPath, HealthCheckController); // in the same way dynamically set check method route path
return {
module: HealthCheckModule,
controllers: [HealthCheckerController]
components: [HealthCheckerService],
exports: [...opts.checkersProviders],
};
} }
2. More cooler from my POV, but it needs more general nest-pimp-up)) As for me, one of the major features to be added.
https://github.com/inversify/InversifyJS/blob/master/wiki/multi_injection.md
According to that, from `@nest/health` module we can expose some provider token
export const HEALTH_CHECK_PROVIDER_TOKEN = Symbol("health_checker");
expose dynamic module
export class HealthCheckModule { static forRoot(opts: { statusEndpointUrl?: string, checkerProviders: any[] }): DynamicModule { const controllerPath = opts.statusEndpointUrl || "/health"; Reflect.defineMetadata(constants_1.PATH_METADATA, controllerPath, HealthCheckController); // in the same way dynamically set check method route path
return {
module: HealthCheckModule,
controllers: [HealthCheckerController]
components: [...checkerProviders],
exports: [...checkerProviders],
};
} }
let's define interface:
export interface HealthChecker {
public check(): void | Promise
than we can simply inject them into HealthCheckService
export class HealthCheckerService {
public constructor(@MultiInject(HEALTH_CHECK_PROVIDER_TOKEN) private readonly checkers: HealthChecker[])
public check(): string[] {
const errors: string[] = [];
//await Promise.map(this.checkers, async checker => {
try {
await checker.callback();
} catch (err) {
console.error(Executing ${checker.name} failed:
, err);
errors.push(obtainedErrorMessage);
}
});
return errors;
} }
For me, option 2 is much cooler since we can operate with any other implemented checker services with their own injections, avoiding callback closures. One thing I am concerned is about injection hierarchy here.
also, a set of default checkers can be provided by this module, like AppLaunchedChecker
, and etc.
once we agree on API (not exactly proposed by me :rofl: ) I can implement it :muscle:
Why just not to integrate existing solution? https://github.com/CloudNativeJS/cloud-health-connect
@unlight good point, thanks. Idea was to create something generic & extensible with ability to add custom health checks policies. cloud-health-connect can be easily integrated into this module :+1:
Alright even though this sounds like an easy and straight forward endpoint this is a lengthy proposal so that it covers all the best practices. Also I'd like to add that adding the possibility to write and attach custom health indicator may increase the complexity of this feature request and therefore could be added at a later time. However I believe that liveness check should be aware of some nest logic, which means that this route should only respond a 200 status page if all async providers and init stuff successfully ran, all routes were successfully bound and the app is ready to receive traffic. Until then it should respond a 503 page (see below).
HTTP default path: GET "/health" (must be configurable) Success: HTTP response status code: 200 HTTP response body:
{
status: "UP",
"diskSpace": {
"status": "UP",
"free": 56443746,
"threshold": 1345660
},
"customHealthcheck": {
"status": "UP"
}
}
Down: HTTP response status code: 503 HTTP response body:
{
"status":"DOWN",
"reason":"customHealthCheck is DOWN",
"diskSpace":{
"status":"UP",
"free":56443746,
"threshold":1345660
},
"customHealthcheck":{
"status":"DOWN"
}
}
main.ts
import { HealthModule } from "@nestjs/health";
async function bootstrap() {
const app = await NestFactory.create(ApplicationModule);
// exposeDetails: boolean = false; If set to true it will expose details for each health indicator (see above examples), default is set to false where it would only report the status (UP/DOWN)
const options = { exposeDetails: true }
HealthModule.setup('admin/healthcheck', app, options);
await app.listen(3001);
}
bootstrap();
custom-health-check.ts
@Injectable
public class CustomHealthCheck implements HealthIndicator {
// If isHealthy() takes longer than this, consider the check as failed and consider this health indicator as down
protected healthCheckTimeoutMs: number = 1 * 1000;
public async isHealthy(): HealthResponse {
const builder = new HealthBuilder("customHealthCheck"); // Or use class name with applied lower camel case format instead? Then pass the builder as argument, so that you don't need to create an instance
try {
const mysqlTestResponse = await mysqldb.ping();
if (mysqlTestResponse != null) return builder.up();
return builder.down({ reason: "MySQL DB ping response is null or undefined" });
} catch (err) {
// Return down status, optionally along with more information
return builder.down({ host: mysqldb.host, ...err });
}
}
}
main.ts
import { HealthModule } from "@nestjs/health";
import { CustomHealthCheck } from "./mysql/custom-health-check";
async function bootstrap() {
const app = await NestFactory.create(ApplicationModule);
// exposeDetails: boolean = false; If set to true it will expose details for each health indicator (see above examples), default is set to false where it would only report the status (UP/DOWN)
const options = { exposeDetails: true, customHealthChecks: [CustomHealthCheck] }
HealthModule.setup('admin/healthcheck', app, options);
await app.listen(3001);
}
bootstrap();
@weeco here I see a trouble with dependency injection. HealthModule doesn't know anything about your checkers dependencies, so it won't resolve it. correct me if I am wrong, please.
I like @weeco approach, but I think as @unlight mentioned I think we should use an existing solution. I think terminus from godaddy is a good choice which also adds better Kubernetes support in addition. I've started a repository and try to hack around with it from time to time. My proposal would be for a healthCheck:
import { Controller, Get, Inject } from '@nestjs/common';
import { HealthCheck } from '@nestjs/terminus';
import { Connection } from 'typeorm';
@Controller('health')
export class HealthController {
constructor(@Inject('Connection') public connection: Connection) { }
@Get('/db')
@HealthCheck()
public async DbHealthCheck() {
return this.connection.isConnected;
}
}
If @HealthCheck
-decorator is set, it will automatically append it to the Terminus options. It will use the defined route (in this example ${globalPrefix}/health/db
).
In the future we could add predefined health checks like database connection or other things. But I think we should start easy and expand later.
My pending research:
beforeShutdown
onSignal
onShutdown
? Like with the SwaggerModule oustide of the Nest Application module during the bootstrap, or withing an Injectable?timeout
? TerminusModule.forRoot({ timeout: number })
?What is definitely a MUST is that custom health checks have to be injectable. Also, I'm not really convinced if we should use decorators for this case (we shall ensure that we don't follow Spring way imho, decorators hell).
And lifecycle hooks like beforeShutdown
should be within an injectable I believe
@kamilmysliwiec
And lifecycle hooks like
beforeShutdown
should be within an injectable I believe
My proposal for that would be:
import { Injectable } from '@nestjs/common';
import { OnShutdown, OnSignal, BeforeShotdown } from '@nestjs/terminus';
@Injectable()
export class ServerLifecycle {
constructor() { }
@OnShutdown()
async OnServerShutdown() {
console.log('On Shutdown!!');
}
}
But I do not know if this is the best approach. That would mean you can create multiple hook and I do not know yet if this is a good or a bad thing
I think that all this stuff could be easily integrated with Nest using a similar approach to this one that I have described here: https://github.com/nestjs/nest/issues/530. 0 decorators - just expose interfaces for options object, so end-user would know what method should be exposed. Then, all this stuff - lifecycle hooks, custom health checks could be simply managed by the developer.
I hacked around a bit and got a working version (inspired of nest/typeorm).
At the moment you can use it with useClass
. useExisting
, useFactory
to be followed.
import { Module, Injectable } from '@nestjs/common';
import { TerminusModule } from '../../../lib/terminus.module';
import { TerminusOptions } from '../../../lib/interfaces/terminus-options';
@Injectable()
export class TerminusService implements TerminusOptions {
public async onSignal() {
console.log('1. onSignal');
}
public async onShutdown() {
console.log('2. onShutdown');
}
// Fix me: No public attributes
public signal: string = 'SIGTERM';
public logger = console.log;
}
@Module({
imports: [
TerminusModule.forRootAsync({
useClass: TerminusService,
}),
],
})
export class ApplicationModule {}
kill -SIGTERM ${PID}
will print the following output:
1. onSignal
2. onShutdown
It does not work with Express yet. The problem is Express does not expose or even store the internal http server instance (Source). Express would need to add that or @nest/core
should add a constant similar to HTTP_SERVER_REF
but with the http server instance, not just the adapter instance (which includes a express instance).
Getting tracked with #1017
Looks pretty neat so far @BrunnerLivio
Terminus Nest integration works so far :tada:
nest new nest-terminus-example
cd nest-terminus-example/
# JUST USE @brunnerlivio/terminus FOR EXPERIMENTAL USE -- WON'T BE MAINTAINED
npm install --save @nestjs/core@5.3.0 @nestjs/common@5.3.0 @brunnerlivio/terminus @godaddy/terminus
# Add new app module
rm -rf src/app.module.ts
touch src/app.module.ts
cat <<EOT >> src/app.module.ts
import { Module } from '@nestjs/common';
import { AppController } from './app.controller';
import { AppService } from './app.service';
import { TerminusModule } from '@brunnerlivio/terminus';
@Module({
imports: [TerminusModule.forRoot({
healthChecks: {
'/health': async () => true,
},
})],
controllers: [AppController],
providers: [AppService],
})
export class AppModule { }
EOT
npm start
# Open on localhost:3000/health
forRoot
or asynchronous forRootAsync
in combination with useFactory
or useClass
@nestjs/typeorm
name
(if getting merged to @nestjs
org)useExisting
. (Never used this option actually myself, so do not even know how it behaves :sweat_smile: . Do not know if it makes sense in the context of @nestjs/terminus
)Anything to add @kamilmysliwiec ? Can this be a part of the @nestjs
-org?
Can this be a part of the @nest-org?
Obviously!
Will check it very soon, thanks for your hard job 💪
@kamilmysliwiec
Updated my comment.
Please also give me feedback of the Considerable points, if I should implement it in the future or not. Some of these points could also be applied for other modules e.g. @nestjs/typeorm
or @nestjs/swagger
.
@BrunnerLivio
Logging (e.g. when a health route gets registered)
As soon as it would be possible to disable/enable them 👍
useExisting. (Never used this option actually myself, so do not even know how it behaves 😅 . Do not know if it makes sense in the context of @nestjs/terminus)
It makes sense (read here about useExisting: https://docs.nestjs.com/techniques/database)
Continuous deployment to npm (would be nice for other nest modules too!)
it would be awesome for each existing nest module. I didn't have enough time to take care of it so far.
Compodocs implementation: See feature/docs which has automatic doc generation using travis and gets published on Github pages: https://brunnerlivio.github.io/nest-terminus/
Predefined health checks?
not sure what do you mean by that
Alright, add logging, useExisting
and npm deployment asap.
Compodocs implementation: See feature/docs which has automatic doc generation using travis and gets published on Github pages: https://brunnerlivio.github.io/nest-terminus/
Created a PR with a better description. Hope it clarifies it for you. brunnerlivio/nest-terminus#1
Predefined health checks?
As @weeco mentioned:
All initialization stuff is done (asynchronous connections, binding routes, asynchronous modules, etc..) The status of the connections to the infrastructure services (such as a database) is good
I think there should be an easy way to check these out of the box things.
I think there should be an easy way to check these out of the box things.
i'm open to ideas :)
brunnerlivio/nest-terminus
is now nest/terminus. I try to release it on npm by the end of the week!
Lets close this issue and discuss this further on nestjs/terminus. I've also created an issue for the predefined health checks nestjs/terminus#2. Feel free to create more issues if you have more ideas!
@BrunnerLivio Hi, is your package ready to use yet? I don't see @nestjs/terminus
in npm.
@zhenwenc I am currently writing the documentation. Hopefully I'll find some time this week to finish everything from my side for the first release.
If you want to try it, you can currently use my test-npm-package:
npm i @brunnerlivio/terminus
Do not use this package in production! It has not been approved by @kamilmysliwiec yet and the API interface may change. Nonetheless I encourage some feedback on the general architecture of the package.
Here are some examples:
Blockers
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For deploying applications in Kubernetes you should always provide a health and liveness check endpoint, which allows Kubernetes to restart a pod if there is an issue.
Readiness endpoint: Readiness check has to be passed to allow Kubernetes sending traffic to your instance (pod). Therefore it should return an empty response with http status 200 once all initialization stuff is finished and all routes are bound.
Such an endpoint is important for kubernetes deployment because by default it will send traffic to the pod once the container has been started. Instead we want to make sure the app is fully started and ready to serve traffic using a proper readiness endpoint.
Liveness endpoint: The liveness endpoint is supposed to tell kubernetes if the app is alive or dead.
Proposal: Create a module @nest/health which adds a single health endpoint (whose path can be defined). This endpoint starts to respond with status 200 if certain requirements are met: