From 6ac1328e2137f4273dd272fad4a5ec358d185514 Mon Sep 17 00:00:00 2001 From: Andrew Lee <1517745+andrewrlee@users.noreply.github.com> Date: Fri, 29 Nov 2024 17:10:56 +0000 Subject: [PATCH] Moving to monitoring library --- CHANGELOG.md | 14 ++++- integration_tests/e2e/health.cy.ts | 2 +- package-lock.json | 17 ++++++ package.json | 1 + server/applicationInfo.ts | 2 +- server/config.ts | 2 + server/data/healthCheck.test.ts | 83 -------------------------- server/data/healthCheck.ts | 43 ------------- server/middleware/setUpHealthChecks.ts | 36 ++++------- server/services/healthCheck.test.ts | 81 ------------------------- server/services/healthCheck.ts | 73 ---------------------- 11 files changed, 45 insertions(+), 309 deletions(-) delete mode 100644 server/data/healthCheck.test.ts delete mode 100644 server/data/healthCheck.ts delete mode 100644 server/services/healthCheck.test.ts delete mode 100644 server/services/healthCheck.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index d8c732d9..7dd84b21 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,10 +1,22 @@ # Change log +**November 29th 2024** - Moving to the new monitoring library + +There's a new set of [shared libraries](https://github.com/ministryofjustice/hmpps-typescript-lib) that should allow us to share code more efficiently than current mechanisms. + +The first of these is a [new library](https://github.com/ministryofjustice/hmpps-typescript-lib/tree/main/packages/monitoring) that wraps the 3 common endpoints used for monitoring and strives to ensure that teams have health monitoring of it's dependencies. + +The library will attempt to self install itself by running it via npx: `npx @ministryofjustice/hmpps-monitoring` + +It will then prompt you to perform some manual tasks - if you have stub tests for your health endpoints you might need add some additional stubbing. + +See PR [#479](https://github.com/ministryofjustice/hmpps-template-typescript/pull/479) + **November 18th 2024** - Moving away from csurf and to csrf-sync [csurf](https://www.npmjs.com/package/csurf) has been deprecated for some time and this removes that dependency and implements the [synchronizer token pattern](https://cheatsheetseries.owasp.org/cheatsheets/Cross-Site_Request_Forgery_Prevention_Cheat_Sheet.html#transmissing-csrf-tokens-in-synchronized-patterns) using [csrf-sync](https://www.npmjs.com/package/csrf-sync). -**Note:** Previously csurf used to generate new tokens on every request. The new library generates tokens once per session which is preferrable due to the extra calls to redis that per-request would generate. It is possible to force a refresh/revocation of a token by explicitly calling: `req.csrfToken(true)` +**Note:** Previously csurf used to generate new tokens on every request. The new library generates tokens once per session which is preferrable due to the extra calls to redis that per-request would generate. It is possible to force a refresh/revocation of a token by explicitly calling: `req.csrfToken(true)` See PR [#481](https://github.com/ministryofjustice/hmpps-template-typescript/pull/481) diff --git a/integration_tests/e2e/health.cy.ts b/integration_tests/e2e/health.cy.ts index 7c36be5e..40f5e6f8 100644 --- a/integration_tests/e2e/health.cy.ts +++ b/integration_tests/e2e/health.cy.ts @@ -30,7 +30,7 @@ context('Healthcheck', () => { cy.request({ url: '/health', method: 'GET', failOnStatusCode: false }).then(response => { expect(response.body.components.hmppsAuth.status).to.equal('UP') expect(response.body.components.tokenVerification.status).to.equal('DOWN') - expect(response.body.components.tokenVerification.details).to.contain({ status: 500, retries: 2 }) + expect(response.body.components.tokenVerification.details).to.contain({ status: 500, attempts: 3 }) }) }) diff --git a/package-lock.json b/package-lock.json index 14712400..979984ea 100644 --- a/package-lock.json +++ b/package-lock.json @@ -11,6 +11,7 @@ "dependencies": { "@aws-sdk/client-sqs": "^3.682.0", "@ministryofjustice/frontend": "^3.0.1", + "@ministryofjustice/hmpps-monitoring": "^0.0.1-beta.1", "agentkeepalive": "^4.5.0", "applicationinsights": "^2.9.6", "body-parser": "^1.20.3", @@ -2267,6 +2268,22 @@ "jquery": "^3.6.0" } }, + "node_modules/@ministryofjustice/hmpps-monitoring": { + "version": "0.0.1-beta.1", + "resolved": "https://registry.npmjs.org/@ministryofjustice/hmpps-monitoring/-/hmpps-monitoring-0.0.1-beta.1.tgz", + "integrity": "sha512-J4QiocnHBg1Ey4pIezxT3tY2gnVlEAUhjqaC+J8z0cLCnGBXWLa5wNqKxpB6Dcwido7VHEuwG2Ixj9uD8gT/Gg==", + "license": "MIT", + "bin": { + "hmpps-monitoring": "bin/migrate.sh" + }, + "engines": { + "node": "20 || 22" + }, + "peerDependencies": { + "agentkeepalive": "4.x", + "superagent": "^10.x" + } + }, "node_modules/@nodelib/fs.scandir": { "version": "2.1.5", "resolved": "https://registry.npmjs.org/@nodelib/fs.scandir/-/fs.scandir-2.1.5.tgz", diff --git a/package.json b/package.json index 935c6614..ea52e6e4 100644 --- a/package.json +++ b/package.json @@ -78,6 +78,7 @@ "dependencies": { "@aws-sdk/client-sqs": "^3.682.0", "@ministryofjustice/frontend": "^3.0.1", + "@ministryofjustice/hmpps-monitoring": "^0.0.1-beta.1", "agentkeepalive": "^4.5.0", "applicationinsights": "^2.9.6", "body-parser": "^1.20.3", diff --git a/server/applicationInfo.ts b/server/applicationInfo.ts index 7b7bd496..c28c127b 100644 --- a/server/applicationInfo.ts +++ b/server/applicationInfo.ts @@ -9,7 +9,7 @@ export type ApplicationInfo = { buildNumber: string gitRef: string gitShortHash: string - productId?: string + productId: string branchName: string } diff --git a/server/config.ts b/server/config.ts index 8fd90e60..9d652c4a 100755 --- a/server/config.ts +++ b/server/config.ts @@ -70,6 +70,7 @@ export default { apis: { hmppsAuth: { url: get('HMPPS_AUTH_URL', 'http://localhost:9090/auth', requiredInProduction), + healthPath: '/health/ping', externalUrl: get('HMPPS_AUTH_EXTERNAL_URL', get('HMPPS_AUTH_URL', 'http://localhost:9090/auth')), timeout: { response: Number(get('HMPPS_AUTH_TIMEOUT_RESPONSE', 10000)), @@ -83,6 +84,7 @@ export default { }, tokenVerification: { url: get('TOKEN_VERIFICATION_API_URL', 'http://localhost:8100', requiredInProduction), + healthPath: '/health/ping', timeout: { response: Number(get('TOKEN_VERIFICATION_API_TIMEOUT_RESPONSE', 5000)), deadline: Number(get('TOKEN_VERIFICATION_API_TIMEOUT_DEADLINE', 5000)), diff --git a/server/data/healthCheck.test.ts b/server/data/healthCheck.test.ts deleted file mode 100644 index 84649fe2..00000000 --- a/server/data/healthCheck.test.ts +++ /dev/null @@ -1,83 +0,0 @@ -import nock from 'nock' -import { serviceCheckFactory } from './healthCheck' -import { AgentConfig } from '../config' - -describe('Service healthcheck', () => { - const healthcheck = serviceCheckFactory('externalService', 'http://test-service.com/ping', new AgentConfig(), { - response: 100, - deadline: 150, - }) - - let fakeServiceApi: nock.Scope - - beforeEach(() => { - fakeServiceApi = nock('http://test-service.com') - }) - - afterEach(() => { - nock.abortPendingRequests() - nock.cleanAll() - }) - - describe('Check healthy', () => { - it('Should return data from api', async () => { - fakeServiceApi.get('/ping').reply(200, 'pong') - - const output = await healthcheck() - expect(output).toEqual('UP') - }) - }) - - describe('Check unhealthy', () => { - it('Should throw error from api', async () => { - fakeServiceApi.get('/ping').thrice().reply(500) - - await expect(healthcheck()).rejects.toThrow('Internal Server Error') - }) - }) - - describe('Check healthy retry test', () => { - it('Should retry twice if request fails', async () => { - fakeServiceApi - .get('/ping') - .reply(500, { failure: 'one' }) - .get('/ping') - .reply(500, { failure: 'two' }) - .get('/ping') - .reply(200, 'pong') - - const response = await healthcheck() - expect(response).toEqual('UP') - }) - - it('Should retry twice if request times out', async () => { - fakeServiceApi - .get('/ping') - .delay(10000) // delay set to 10s, timeout to 900/3=300ms - .reply(200, { failure: 'one' }) - .get('/ping') - .delay(10000) - .reply(200, { failure: 'two' }) - .get('/ping') - .reply(200, 'pong') - - const response = await healthcheck() - expect(response).toEqual('UP') - }) - - it('Should fail if request times out three times', async () => { - fakeServiceApi - .get('/ping') - .delay(10000) // delay set to 10s, timeout to 900/3=300ms - .reply(200, { failure: 'one' }) - .get('/ping') - .delay(10000) - .reply(200, { failure: 'two' }) - .get('/ping') - .delay(10000) - .reply(200, { failure: 'three' }) - - await expect(healthcheck()).rejects.toThrow('Response timeout of 100ms exceeded') - }) - }) -}) diff --git a/server/data/healthCheck.ts b/server/data/healthCheck.ts deleted file mode 100644 index c42f2a3c..00000000 --- a/server/data/healthCheck.ts +++ /dev/null @@ -1,43 +0,0 @@ -import superagent from 'superagent' -import Agent, { HttpsAgent } from 'agentkeepalive' -import logger from '../../logger' -import { AgentConfig } from '../config' - -export type ServiceCheck = () => Promise - -export class ServiceTimeout { - response = 1500 - - deadline = 2000 -} - -export function serviceCheckFactory( - name: string, - url: string, - agentOptions: AgentConfig, - serviceTimeout: ServiceTimeout = new ServiceTimeout(), -): ServiceCheck { - const keepaliveAgent = url.startsWith('https') ? new HttpsAgent(agentOptions) : new Agent(agentOptions) - - return () => - new Promise((resolve, reject) => { - superagent - .get(url) - .agent(keepaliveAgent) - .retry(2, (err, res) => { - if (err) logger.info(`Retry handler found API error with ${err.code} ${err.message} when calling ${name}`) - return undefined // retry handler only for logging retries, not to influence retry logic - }) - .timeout(serviceTimeout) - .end((error, result) => { - if (error) { - logger.error(error.stack, `Error calling ${name}`) - reject(error) - } else if (result.status === 200) { - resolve('UP') - } else { - reject(result.status) - } - }) - }) -} diff --git a/server/middleware/setUpHealthChecks.ts b/server/middleware/setUpHealthChecks.ts index b4034c7e..887bdcb9 100644 --- a/server/middleware/setUpHealthChecks.ts +++ b/server/middleware/setUpHealthChecks.ts @@ -1,39 +1,23 @@ import express, { Router } from 'express' -import healthcheck from '../services/healthCheck' +import { monitoringMiddleware, endpointHealthComponent } from '@ministryofjustice/hmpps-monitoring' import type { ApplicationInfo } from '../applicationInfo' +import logger from '../../logger' +import config from '../config' export default function setUpHealthChecks(applicationInfo: ApplicationInfo): Router { const router = express.Router() - router.get('/health', (req, res, next) => { - healthcheck(applicationInfo, result => { - if (result.status !== 'UP') { - res.status(503) - } - res.json(result) - }) - }) + const apiConfig = Object.entries(config.apis) - router.get('/ping', (req, res) => { - res.send({ - status: 'UP', - }) + const middleware = monitoringMiddleware({ + applicationInfo, + healthComponents: apiConfig.map(([name, options]) => endpointHealthComponent(logger, name, options)), }) - router.get('/info', (req, res) => { - res.json({ - git: { - branch: applicationInfo.branchName, - }, - build: { - artifact: applicationInfo.applicationName, - version: applicationInfo.buildNumber, - name: applicationInfo.applicationName, - }, - productId: applicationInfo.productId, - }) - }) + router.get('/health', middleware.health) + router.get('/info', middleware.info) + router.get('/ping', middleware.ping) return router } diff --git a/server/services/healthCheck.test.ts b/server/services/healthCheck.test.ts deleted file mode 100644 index d8548af9..00000000 --- a/server/services/healthCheck.test.ts +++ /dev/null @@ -1,81 +0,0 @@ -import healthCheck from './healthCheck' -import type { ApplicationInfo } from '../applicationInfo' -import type { HealthCheckCallback, HealthCheckService } from './healthCheck' - -describe('Healthcheck', () => { - const testAppInfo: ApplicationInfo = { - applicationName: 'test', - buildNumber: '1', - gitRef: 'long ref', - gitShortHash: 'short ref', - branchName: 'main', - } - - it('Healthcheck reports healthy', done => { - const successfulChecks = [successfulCheck('check1'), successfulCheck('check2')] - - const callback: HealthCheckCallback = result => { - expect(result).toEqual( - expect.objectContaining({ - status: 'UP', - components: { - check1: { - status: 'UP', - details: 'some message', - }, - check2: { - status: 'UP', - details: 'some message', - }, - }, - }), - ) - done() - } - - healthCheck(testAppInfo, callback, successfulChecks) - }) - - it('Healthcheck reports unhealthy', done => { - const successfulChecks = [successfulCheck('check1'), erroredCheck('check2')] - - const callback: HealthCheckCallback = result => { - expect(result).toEqual( - expect.objectContaining({ - status: 'DOWN', - components: { - check1: { - status: 'UP', - details: 'some message', - }, - check2: { - status: 'DOWN', - details: 'some error', - }, - }, - }), - ) - done() - } - - healthCheck(testAppInfo, callback, successfulChecks) - }) -}) - -function successfulCheck(name: string): HealthCheckService { - return () => - Promise.resolve({ - name: `${name}`, - status: 'UP', - message: 'some message', - }) -} - -function erroredCheck(name: string): HealthCheckService { - return () => - Promise.resolve({ - name: `${name}`, - status: 'DOWN', - message: 'some error', - }) -} diff --git a/server/services/healthCheck.ts b/server/services/healthCheck.ts deleted file mode 100644 index 2ac9d0a1..00000000 --- a/server/services/healthCheck.ts +++ /dev/null @@ -1,73 +0,0 @@ -import { serviceCheckFactory } from '../data/healthCheck' -import config from '../config' -import type { AgentConfig } from '../config' -import type { ApplicationInfo } from '../applicationInfo' - -interface HealthCheckStatus { - name: string - status: string - message: unknown -} - -interface HealthCheckResult extends Record { - status: string - components: Record -} - -export type HealthCheckService = () => Promise -export type HealthCheckCallback = (result: HealthCheckResult) => void - -function service(name: string, url: string, agentConfig: AgentConfig): HealthCheckService { - const check = serviceCheckFactory(name, url, agentConfig) - return () => - check() - .then(result => ({ name, status: 'UP', message: result })) - .catch(err => ({ name, status: 'DOWN', message: err })) -} - -function addAppInfo(result: HealthCheckResult, applicationInfo: ApplicationInfo): HealthCheckResult { - const buildInfo = { - uptime: process.uptime(), - build: { - buildNumber: applicationInfo.buildNumber, - gitRef: applicationInfo.gitRef, - }, - version: applicationInfo.buildNumber, - } - - return { ...result, ...buildInfo } -} - -function gatherCheckInfo(aggregateStatus: Record, currentStatus: HealthCheckStatus) { - return { ...aggregateStatus, [currentStatus.name]: { status: currentStatus.status, details: currentStatus.message } } -} - -const apiChecks = [ - service('hmppsAuth', `${config.apis.hmppsAuth.url}/health/ping`, config.apis.hmppsAuth.agent), - ...(config.apis.tokenVerification.enabled - ? [ - service( - 'tokenVerification', - `${config.apis.tokenVerification.url}/health/ping`, - config.apis.tokenVerification.agent, - ), - ] - : []), -] - -export default function healthCheck( - applicationInfo: ApplicationInfo, - callback: HealthCheckCallback, - checks = apiChecks, -): void { - Promise.all(checks.map(fn => fn())).then(checkResults => { - const allOk = checkResults.every(item => item.status === 'UP') ? 'UP' : 'DOWN' - - const result = { - status: allOk, - components: checkResults.reduce(gatherCheckInfo, {}), - } - - callback(addAppInfo(result, applicationInfo)) - }) -}