From 87442cf94fcfedab912fb57a8b5f03bb2da56bf9 Mon Sep 17 00:00:00 2001 From: Alan Cruikshanks Date: Thu, 26 Sep 2024 13:49:10 +0100 Subject: [PATCH] Remove import jobs stats from /health/info (#1336) https://eaflood.atlassian.net/browse/WATER-4670 After making some changes to [water-abstraction-import](https://github.com/DEFRA/water-abstraction-import) we found it was no longer completing when run. The issue seemed to be - a step we introduced to let [water-abstraction-system](https://github.com/DEFRA/water-abstraction) see importing licences was overloading the environment because it was acting like a DOS on the system app - each job was taking much longer than realised to complete, and the existing job schedule was overlapping them, again leading to limited system resources We've always suspected that the features the previous team added to improve performance (pg-boss message queues and caching to Redis) were actually impacting it. Initially, we planned to go through all the 'jobs' involved in importing data from NALD and strip out this overhead. We [carried out a spike](https://github.com/DEFRA/water-abstraction-import/pull/1023), and our timings are much, _much_ better, proving our suspicions. However, this was a live issue, so we removed the step and tweaked the times to get the import working again. In the meantime, we'll either complete the spike or [WATER-4535, a replacement for the licence import](https://eaflood.atlassian.net/browse/WATER-4535). Either way, the block of job stats we display on the `/health/info` page will be redundant. In fact, we don't use or look at it anyway, even when there is an issue. So, this removes all logic related to job information from the `/health/info` page in readiness. --- .../health/fetch-import-jobs.service.js | 95 --------------- app/services/health/info.service.js | 31 +---- app/views/info.njk | 63 +++------- test/services/health/info.service.test.js | 115 ------------------ 4 files changed, 24 insertions(+), 280 deletions(-) delete mode 100644 app/services/health/fetch-import-jobs.service.js diff --git a/app/services/health/fetch-import-jobs.service.js b/app/services/health/fetch-import-jobs.service.js deleted file mode 100644 index c8080d9c15..0000000000 --- a/app/services/health/fetch-import-jobs.service.js +++ /dev/null @@ -1,95 +0,0 @@ -'use strict' - -/** - * Fetches details of the pg-boss jobs run by the Import service in the last 24hrs - * @module FetchImportJobs - */ - -const { db } = require('../../../db/db.js') - -/** - * Returns data required to populate the Import tasks of our `/health/info` page. - * - * @returns {object} - query response -*/ -async function go () { - const PGBOSS_JOBS_ARRAY = [ - 'import.bill-runs', - 'import.charge-versions', - 'import.charging-data', - 'import.tracker', - 'licence-import.delete-removed-documents', - 'licence-import.import-company', - 'licence-import.import-licence', - 'licence-import.import-purpose-condition-types', - 'licence-import.queue-companies', - 'licence-import.queue-licences', - 'nald-import.delete-removed-documents', - 'nald-import.import-licence', - 'nald-import.queue-licences', - 'nald-import.s3-download' - ] - const currentDateMinusOneDay = _subtractDaysFromCurrentDate(1) - - return db - .select('name') - .count({ completedCount: db.raw("CASE WHEN state = 'completed' THEN 1 END") }) - .count({ failedCount: db.raw("CASE WHEN state = 'failed' THEN 1 END") }) - .count({ activeCount: db.raw("CASE WHEN state IN ('active', 'created') THEN 1 END") }) - .max('completedon as maxCompletedonDate') - .from( - (db - .select( - 'name', - 'state', - 'completedon' - ) - .from('water_import.job') - .whereIn('state', ['failed', 'completed', 'active', 'created']) - .whereIn('name', PGBOSS_JOBS_ARRAY) - .where((builder) => { - return builder - .where('createdon', '>', currentDateMinusOneDay) - .orWhere('completedon', '>', currentDateMinusOneDay) - }) - .unionAll( - db - .select( - 'name', - 'state', - 'completedon' - ) - .from('water_import.archive') - .whereIn('state', ['failed', 'completed', 'active', 'created']) - .whereIn('name', PGBOSS_JOBS_ARRAY) - .where((builder) => { - return builder - .where('createdon', '>', currentDateMinusOneDay) - .orWhere('completedon', '>', currentDateMinusOneDay) - }) - )) - .as('jobs') - ) - .groupBy('name') -} - -/** - * Calculates the current date minus the number of days passed to it - * - * @param {number} days - The number of days to be deducted from the current date - * - * @returns {Date} The current date minus the number days passed to the function - * - * @private - */ -function _subtractDaysFromCurrentDate (days) { - const date = new Date() - - date.setDate(date.getDate() - days) - - return date -} - -module.exports = { - go -} diff --git a/app/services/health/info.service.js b/app/services/health/info.service.js index a8f8449bf0..8f6090c0ac 100644 --- a/app/services/health/info.service.js +++ b/app/services/health/info.service.js @@ -12,21 +12,23 @@ const exec = util.promisify(ChildProcess.exec) const ChargingModuleRequest = require('../../requests/charging-module.request.js') const CreateRedisClientService = require('./create-redis-client.service.js') -const FetchImportJobsService = require('./fetch-import-jobs.service.js') const FetchSystemInfoService = require('./fetch-system-info.service.js') -const { formatLongDateTime } = require('../../presenters/base.presenter.js') const BaseRequest = require('../../requests/base.request.js') const LegacyRequest = require('../../requests/legacy.request.js') const servicesConfig = require('../../../config/services.config.js') /** + * Checks status and gathers info for each of the services which make up WRLS + * * Returns data required to populate our `/service-status` page, eg. task activity status, virus checker status, service * version numbers, etc. * * Each data set is returned in the format needed to populate the gov.uk table elements ie. an array containing one * array per row, where each row array contains multiple `{ text: '...' }` elements, one for each cell in the row. -*/ + * + * @returns {object} data about each service formatted for the view + */ async function go () { const addressFacadeData = await _getAddressFacadeData() const chargingModuleData = await _getChargingModuleData() @@ -83,7 +85,6 @@ async function _getLegacyAppData () { if (result.succeeded) { service.version = result.response.body.version service.commit = result.response.body.commit - service.jobs = service.name === 'Import' ? await _getImportJobsData() : [] } else { service.version = _parseFailedRequestResult(result) service.commit = '' @@ -103,16 +104,6 @@ async function _getChargingModuleData () { return _parseFailedRequestResult(result) } -async function _getImportJobsData () { - try { - const importJobs = await FetchImportJobsService.go() - - return _mapArrayToTextCells(importJobs) - } catch (error) { - return [] - } -} - async function _getRedisConnectivityData () { let redis @@ -141,18 +132,6 @@ async function _getVirusScannerData () { } } -function _mapArrayToTextCells (rows) { - return rows.map((row) => { - return [ - ...[{ text: row.name }], - ...[{ text: row.completedCount }], - ...[{ text: row.failedCount }], - ...[{ text: row.activeCount }], - ...[{ text: row.maxCompletedonDate ? formatLongDateTime(row.maxCompletedonDate) : '-' }] - ] - }) -} - function _parseFailedRequestResult (result) { if (result.response.statusCode) { return `ERROR: ${result.response.statusCode} - ${result.response.body}` diff --git a/app/views/info.njk b/app/views/info.njk index 31b4ed62a6..6b67d80dca 100644 --- a/app/views/info.njk +++ b/app/views/info.njk @@ -26,55 +26,30 @@

{{ chargingModuleData }}

{% for app in appData %} -
+
-

{{ app.name }}

- {{ govukSummaryList({ - classes: 'govuk-summary-list--no-border', - rows: [ - { - key: { - text: "Version" - }, - value: { - text: app.version - } - }, - { - key: { - text: "Commit hash" - }, - value: { - text: app.commit - } - } - ] - }) }} - - {% if app.jobs.length %} - {{ govukTable({ - firstCellIsHeader: false, - head: [ +

{{ app.name }}

+ {{ govukSummaryList({ + classes: 'govuk-summary-list--no-border', + rows: [ { - text: "Import task name (data for the last 24 hrs)" + key: { + text: "Version" + }, + value: { + text: app.version + } }, { - text: "Completed" - }, - { - text: "Failed" - }, - { - text: "Active" - }, - { - text: "Last job completed" + key: { + text: "Commit hash" + }, + value: { + text: app.commit + } } - ], - rows: app.jobs + ] }) }} - {% endif %} - - {% endfor %} + {% endfor %} {% endblock %} diff --git a/test/services/health/info.service.test.js b/test/services/health/info.service.test.js index b557c765a4..aafacf6233 100644 --- a/test/services/health/info.service.test.js +++ b/test/services/health/info.service.test.js @@ -15,7 +15,6 @@ const servicesConfig = require('../../../config/services.config.js') // Things we need to stub const ChargingModuleRequest = require('../../../app/requests/charging-module.request.js') const CreateRedisClientService = require('../../../app/services/health/create-redis-client.service.js') -const FetchImportJobsService = require('../../../app/services/health/fetch-import-jobs.service.js') const LegacyRequest = require('../../../app/requests/legacy.request.js') const BaseRequest = require('../../../app/requests/base.request.js') @@ -40,32 +39,13 @@ describe('Info service', () => { app: { succeeded: true, response: { statusCode: 200, body: { version: '9.0.99', commit: '99d0e8c' } } } } - const goodFetchImportJobsResults = [ - { - name: 'import.charging-data', - completedCount: 1, - failedCount: 0, - activeCount: 0, - maxCompletedonDate: new Date('2023-09-04T14:00:10.758Z') - }, - { - name: 'licence-import.import-company', - completedCount: 52963, - failedCount: 1, - activeCount: 123, - maxCompletedonDate: new Date('2023-09-04T10:43:44.503Z') - } - ] - let chargingModuleRequestStub - let fetchImportJobsStub let legacyRequestStub let baseRequestStub let redisStub beforeEach(() => { chargingModuleRequestStub = Sinon.stub(ChargingModuleRequest, 'get') - fetchImportJobsStub = Sinon.stub(FetchImportJobsService, 'go') legacyRequestStub = Sinon.stub(LegacyRequest, 'get') baseRequestStub = Sinon.stub(BaseRequest, 'get') redisStub = Sinon.stub(CreateRedisClientService, 'go') @@ -93,7 +73,6 @@ describe('Info service', () => { describe('when all the services are running', () => { beforeEach(async () => { - fetchImportJobsStub.resolves(goodFetchImportJobsResults) redisStub.returns({ ping: Sinon.stub().resolves(), disconnect: Sinon.stub().resolves() }) // In this scenario everything is hunky-dory so we return 2xx responses from these services @@ -136,23 +115,6 @@ describe('Info service', () => { expect(result.appData[0].serviceName).to.equal('import') expect(result.appData[0].version).to.equal('9.0.99') expect(result.appData[0].commit).to.equal('99d0e8c') - expect(result.appData[0].jobs).to.equal([ - [ - { text: 'import.charging-data' }, - { text: 1 }, - { text: 0 }, - { text: 0 }, - { text: '4 September 2023 at 14:00:10' } - ], - [ - { text: 'licence-import.import-company' }, - { text: 52963 }, - { text: 1 }, - { text: 123 }, - { text: '4 September 2023 at 10:43:44' } - ] - ]) - expect(result.appData[1].jobs).to.equal([]) expect(result.redisConnectivityData).to.equal('Up and running') expect(result.virusScannerData).to.equal('ClamAV 9.99.9/26685/Mon Oct 10 08:00:01 2022\n') @@ -161,8 +123,6 @@ describe('Info service', () => { describe('when Redis', () => { beforeEach(async () => { - fetchImportJobsStub.resolves(goodFetchImportJobsResults) - // In these scenarios everything is hunky-dory so we return 2xx responses from these services baseRequestStub .withArgs(`${servicesConfig.addressFacade.url}/address-service/hola`) @@ -201,7 +161,6 @@ describe('Info service', () => { ]) expect(result.appData).to.have.length(10) expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(2) expect(result.redisConnectivityData).to.equal('ERROR: Redis check went boom') expect(result.virusScannerData).to.equal('ClamAV 9.99.9/26685/Mon Oct 10 08:00:01 2022\n') @@ -211,7 +170,6 @@ describe('Info service', () => { describe('when ClamAV', () => { beforeEach(async () => { - fetchImportJobsStub.resolves(goodFetchImportJobsResults) redisStub.returns({ ping: Sinon.stub().resolves(), disconnect: Sinon.stub().resolves() }) // In these scenarios everything is hunky-dory so we return 2xx responses from these services @@ -249,7 +207,6 @@ describe('Info service', () => { ]) expect(result.appData).to.have.length(10) expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(2) expect(result.redisConnectivityData).to.equal('Up and running') expect(result.virusScannerData).to.startWith('ERROR:') @@ -281,7 +238,6 @@ describe('Info service', () => { ]) expect(result.appData).to.have.length(10) expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(2) expect(result.virusScannerData).to.startWith('ERROR:') expect(result.redisConnectivityData).to.equal('Up and running') @@ -289,77 +245,8 @@ describe('Info service', () => { }) }) - describe('when FetchImportJobs service', () => { - beforeEach(async () => { - redisStub.returns({ ping: Sinon.stub().resolves(), disconnect: Sinon.stub().resolves() }) - - // In this scenario everything is hunky-dory so we return 2xx responses from these services - baseRequestStub - .withArgs(`${servicesConfig.addressFacade.url}/address-service/hola`) - .resolves(goodRequestResults.addressFacade) - legacyRequestStub.withArgs('water', 'health/info', null, false).resolves(goodRequestResults.app) - - const execStub = Sinon - .stub() - .withArgs('clamdscan --version') - .resolves({ - stdout: 'ClamAV 9.99.9/26685/Mon Oct 10 08:00:01 2022\n', - stderror: null - }) - const utilStub = { - promisify: Sinon.stub().callsFake(() => { - return execStub - }) - } - - InfoService = Proxyquire('../../../app/services/health/info.service', { util: utilStub }) - }) - - describe('returns no results', () => { - beforeEach(async () => { - fetchImportJobsStub.resolves([]) - }) - - it('jobs are empty and still returns a result for the other services', async () => { - const result = await InfoService.go() - - expect(result).to.include([ - 'virusScannerData', 'redisConnectivityData', 'addressFacadeData', 'chargingModuleData', 'appData' - ]) - expect(result.appData).to.have.length(10) - expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(0) - - expect(result.redisConnectivityData).to.equal('Up and running') - expect(result.virusScannerData).to.equal('ClamAV 9.99.9/26685/Mon Oct 10 08:00:01 2022\n') - }) - }) - - describe('throws an exception', () => { - beforeEach(async () => { - fetchImportJobsStub.throwsException(new Error('FetchImportJobs went boom')) - }) - - it('handles the error and still returns a result for the other services', async () => { - const result = await InfoService.go() - - expect(result).to.include([ - 'virusScannerData', 'redisConnectivityData', 'addressFacadeData', 'chargingModuleData', 'appData' - ]) - expect(result.appData).to.have.length(10) - expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(0) - - expect(result.redisConnectivityData).to.equal('Up and running') - expect(result.virusScannerData).to.equal('ClamAV 9.99.9/26685/Mon Oct 10 08:00:01 2022\n') - }) - }) - }) - describe('when a service we check via http request', () => { beforeEach(async () => { - fetchImportJobsStub.resolves(goodFetchImportJobsResults) - // In these scenarios everything is hunky-dory with clamav and redis. So, we go back to our original stubbing const execStub = Sinon .stub() @@ -397,7 +284,6 @@ describe('Info service', () => { ]) expect(result.appData).to.have.length(10) expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(2) expect(result.addressFacadeData).to.startWith('ERROR:') @@ -424,7 +310,6 @@ describe('Info service', () => { ]) expect(result.appData).to.have.length(10) expect(result.appData[0].version).to.equal('9.0.99') - expect(result.appData[0].jobs).to.have.length(2) expect(result.addressFacadeData).to.startWith('ERROR:')