Repository: bentoml/BentoML
Pull request: Added the healthz issue 1060 feature
<!--- Thanks for sending a pull request! Please make sure to read the contribution guidelines, then fill out the blanks below before requesting a code review. -->

## Description
<!--- Describe your changes in detail -->
<!--- Attach screenshots here if appropriate. -->
I have added the healthz endpoint to the YataiService Node app. This endpoint is a convention in building microservices, sometimes a deployed service can get into a bad state and become incapable of handling requests.  

## Motivation and Context
<!--- Why is this change required? What problem does it solve? -->
<!--- If it fixes an open issue, please link to the issue here. -->
<!--- If it is based on a conversation in slack channel, pls quote related messages here -->
This solves a part of the #1060 issue:
https://github.com/bentoml/BentoML/issues/1060

## How Has This Been Tested?
<!--- Please describe in detail how you tested your changes. -->
<!--- Include details of your testing environment, and the tests you ran to -->
<!--- see how your change affects other areas of the code, etc. -->
The endpoint is pretty simple, it returns a 200 status if everything is going well. I have tested this locally and it returned 200 as status.

## Types of changes
<!--- What types of changes does your code introduce? Put an `x` in all the boxes that apply: -->
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [x] New feature and improvements (non-breaking change which adds/improves functionality)
- [ ] Bug fix (non-breaking change which fixes an issue)
- [ ] Code Refactoring (internal change which is not user facing)
- [ ] Documentation
- [ ] Test, CI, or build
<!--- [ ] Others: describe the type of change if it does not fit to categories listed -->

## Component(s) if applicable
- [ ] BentoService (service definition, dependency management, API input/output adapters)
- [ ] Model Artifact (model serialization, multi-framework support)
- [ ] Model Server (mico-batching, dockerisation, logging, OpenAPI, instruments)
- [ ] YataiService gRPC server (model registry, cloud deployment automation)
- [x] YataiService web server (nodejs HTTP server and web UI)
- [ ] Internal (BentoML's own configuration, logging, utility, exception handling)
- [ ] BentoML CLI

## Checklist:
<!--- Go over all the following points, and put an `x` in all the boxes that apply. -->
<!--- If you're unsure about any of these, don't hesitate to ask. We're here to help! -->
<!--- If you plan to update documentation or tests in follow-up, please note -->
- [x] My code follows the bentoml code style, both `./dev/format.sh` and
  `./dev/lint.sh` script have passed
  ([instructions](https://github.com/bentoml/BentoML/blob/master/DEVELOPMENT.md#style-check-and-auto-formatting-your-code)).
- [ ] My change reduces project test coverage and requires unit tests to be added
- [ ] I have added unit tests covering my code change
- [x] My change requires a change to the documentation
- [ ] I have updated the documentation accordingly


Problem statement:
YataiService monitoring and prometheus endpoint
Currently, there is no easy way to monitor a YataiService deployment.

* Add a '/healthz' HTTP endpoint via the node server, in addition to te gRPC health check endpoint
* Add a '/metrics' HTTP endpoint via the node server, and allow expose metrics from the python gRPC server

Hints from issue discussion:
I would love to take this on

Hello @parano, I am currently working on this issue, I just wanted to ask, what does the endpoint /healthz do exactly? Thank you!
Hi @CallmeMehdi - I think @aarnphm  is currently working on this issue as well, maybe you two can sync up and discuss?

"/healthz" endpoint is a convention in building microservices, sometimes a deployed service can get into a bad state and become incapable of handling requests. "/healthz" is a special endpoint that let the monitoring system to discover such failed service instance and do something about it(usually restarting the instance)

Implementing the `healthz` endpoint is usually very straightforward, e.g. https://cloud.ibm.com/docs/node?topic=node-node-healthcheck
@CallmeMehdi @aarnphm Any update on this issue?  Let me know if you are running into any issues, happy to help

Commit message: Added the healthz issue 1060 feature

Patch to review:
diff --git a/bentoml/yatai/web/src/server.ts b/bentoml/yatai/web/src/server.ts
index 2d0bbcb11de..c458ad2b5ee 100644
--- a/bentoml/yatai/web/src/server.ts
+++ b/bentoml/yatai/web/src/server.ts
@@ -5,11 +5,11 @@ import express from "express";
 import { bentoml } from "./generated/bentoml_grpc";
 import { createYataiClient } from "./yatai_client";
 import { getLogger } from "./logger";
+
 const logger = getLogger();
 
 const createRoutes = (app, yataiClient) => {
-  let router = express.Router();
-  router.get("/api/ListBento", async (req: Request, res: Response) => {
+  app.get("/api/ListBento", async (req: Request, res: Response) => {
     const requestQuery: any = Object.assign({}, req.query);
     if (req.query.limit && typeof req.query.limit == "string") {
       requestQuery.limit = Number(req.query.limit);
@@ -40,7 +40,7 @@ const createRoutes = (app, yataiClient) => {
     }
   });
 
-  router.get("/api/GetBento", async (req: Request, res: Response) => {
+  app.get("/api/GetBento", async (req: Request, res: Response) => {
     let verifyError = bentoml.GetBentoRequest.verify(req.query);
     if (verifyError) {
       logger.error({ request: "GetBento", error: verifyError });
@@ -64,7 +64,7 @@ const createRoutes = (app, yataiClient) => {
     }
   });
 
-  router.get("/api/GetDeployment", async (req: Request, res: Response) => {
+  app.get("/api/GetDeployment", async (req: Request, res: Response) => {
     let verifyError = bentoml.GetDeploymentRequest.verify(req.query);
     if (verifyError) {
       logger.error({ request: "GetDeployment", error: verifyError });
@@ -88,7 +88,7 @@ const createRoutes = (app, yataiClient) => {
     }
   });
 
-  router.get("/api/ListDeployments", async (req: Request, res: Response) => {
+  app.get("/api/ListDeployments", async (req: Request, res: Response) => {
     const requestQuery: any = Object.assign({}, req.query);
     if (req.query.limit && typeof req.query.limit == "string") {
       requestQuery.limit = Number(req.query.limit);
@@ -116,7 +116,11 @@ const createRoutes = (app, yataiClient) => {
     }
   });
 
-  router.post("/api/DeleteDeployment", async (req: Request, res: Response) => {
+  app.get("/api/healthz", async (req: Request, res: Response) => {
+    res.json({status: 200});
+  });
+
+  app.post("/api/DeleteDeployment", async (req: Request, res: Response) => {
     let verifyError = bentoml.DeleteDeploymentRequest.verify(req.body);
     if (verifyError) {
       logger.error({ request: "DeleteDeployment", error: verifyError });
@@ -143,7 +147,7 @@ const createRoutes = (app, yataiClient) => {
     }
   });
 
-  router.post("/api/DeleteBento", async (req: Request, res: Response) => {
+  app.post("/api/DeleteBento", async (req: Request, res: Response) => {
     let verifyError = bentoml.DangerouslyDeleteBentoRequest.verify(req.body);
     if (verifyError) {
       logger.error({ request: "DeleteBento", error: verifyError });
@@ -167,7 +171,7 @@ const createRoutes = (app, yataiClient) => {
     }
   });
 
-  router.post("/api/ApplyDeployment", async (req: Request, res: Response) => {
+  app.post("/api/ApplyDeployment", async (req: Request, res: Response) => {
     let verifyError = bentoml.ApplyDeploymentRequest.verify(req.body);
     if (verifyError) {
       logger.error({ request: "ApplyDeployment", error: verifyError });
@@ -193,24 +197,10 @@ const createRoutes = (app, yataiClient) => {
       return res.status(500).json(error);
     }
   });
-
-  return router;
 };
 
-export const getExpressApp = (grpcAddress: string | null, baseURL: string) => {
+export const getExpressApp = (grpcAddress: string | null) => {
   const app = express();
-  const cookieParser = require('cookie-parser');
-  app.use(cookieParser());
-  app.use(function (req, res, next) {
-    var cookie = req.cookies.cookieName;
-    if (cookie === undefined) {
-      res.cookie('baseURLCookie',baseURL, { maxAge: 900000, httpOnly: false });
-      console.log('cookie created successfully');
-    } else {
-      console.log('cookie exists', cookie);
-    }
-    next();
-  });
 
   app.use(express.json());
   app.use(
@@ -218,25 +208,17 @@ export const getExpressApp = (grpcAddress: string | null, baseURL: string) => {
       stream: { write: (message) => logger.info(message.trim()) },
     })
   );
+  app.use(express.static(path.join(__dirname, "../dist/client")));
   const yataiClient = createYataiClient(grpcAddress);
-  const router = createRoutes(app, yataiClient);
-  const apiUrlPrefix = (baseURL=="." ? '/' : '/' + baseURL);
-  app.use(apiUrlPrefix, router);
+  createRoutes(app, yataiClient);
 
   app.get("/*", (req, res) => {
-    let directory = req.path.split("/").slice(-2, -1);
-    let filename = req.path.split("/").pop();
     if (/.js$|.css$/.test(req.path)) {
-      res.sendFile(path.join(__dirname, `../dist/client/static/${directory}/${filename}`));
-    } else if (/favicon.png$|logo192.png$/.test(req.path)) {
+      let filename = req.path.split("/").pop();
       res.sendFile(path.join(__dirname, `../dist/client/${filename}`));
-    } else if (/.png/.test(req.path)) {
-      res.sendFile(path.join(__dirname, `../dist/client/static/${directory}/${filename}`));
     } else {
       res.sendFile(path.join(__dirname, "../dist/client/index.html"));
     }
   });
-
-  app.use(express.static(path.join(__dirname, "../dist/client")));
   return app;
-};
+};
\ No newline at end of file


Code review comments:
diff_hunk: @@ -193,50 +197,28 @@ const createRoutes = (app, yataiClient) => {
       return res.status(500).json(error);
     }
   });
-
-  return router;
 };
 
-export const getExpressApp = (grpcAddress: string | null, baseURL: string) => {
+export const getExpressApp = (grpcAddress: string | null) => {
   const app = express();
-  const cookieParser = require('cookie-parser');
-  app.use(cookieParser());
-  app.use(function (req, res, next) {
-    var cookie = req.cookies.cookieName;
-    if (cookie === undefined) {
-      res.cookie('baseURLCookie',baseURL, { maxAge: 900000, httpOnly: false });
-      console.log('cookie created successfully');
-    } else {
-      console.log('cookie exists', cookie);
-    }
-    next();
-  });
 
   app.use(express.json());
   app.use(
     morgan("combined", {
       stream: { write: (message) => logger.info(message.trim()) },
     })
   );
+  app.use(express.static(path.join(__dirname, "../dist/client")));
   const yataiClient = createYataiClient(grpcAddress);
-  const router = createRoutes(app, yataiClient);
-  const apiUrlPrefix = (baseURL=="." ? '/' : '/' + baseURL);
path: bentoml/yatai/web/src/server.ts
text: @user1:
I think we are passing down the baseURL, for the use-case that the user wants to have a prefix for the yatai server. Let's make sure that still works

@user2:
@user1 do we have a test for that?

@user1:
We don't have a test for that atm.

@user3:
Is this something Mehdi can look into? Is it blocking submission of this PR?

@user1:
@user3 I think it would be great that @ CallmeMehdi can look into adding tests for Yatai web server. I think we can do it either in this PR or another one.

For my initial question, I think removing the prefix code will break the ability for the user-defined prefix path. It'd be great to confirm that feature won't be broken after this PR.