diff --git a/.labrc.js b/.labrc.js index d2e82298fe..7d08bddc82 100644 --- a/.labrc.js +++ b/.labrc.js @@ -15,6 +15,9 @@ module.exports = { '__asyncGenerator', '__asyncDelegator', '__asyncValues', '__makeTemplateObject', '__importStar', '__importDefault', '__classPrivateFieldGet', '__classPrivateFieldSet', // We also ignore globals exposed by global-agent: - 'GLOBAL_AGENT','ROARR' + 'GLOBAL_AGENT','ROARR', + // GlobalNotifier is added by us a global in a server plugin. It's how we make logging available anywhere in the app + // whilst avoiding having to pass it around + 'GlobalNotifier' ].join(',') } diff --git a/app/lib/global-notifier.lib.js b/app/lib/global-notifier.lib.js new file mode 100644 index 0000000000..3f89df9561 --- /dev/null +++ b/app/lib/global-notifier.lib.js @@ -0,0 +1,42 @@ +'use strict' + +/** + * @module GlobalNotifierLib + */ + +const BaseNotifierLib = require('./base-notifier.lib.js') + +/** + * A combined logging and Airbrake (Errbit) notification manager for actions that take place outside of a + * {@link https://hapi.dev/api/?v=20.1.2#request|Hapi request} + * + * Created for use with the `app/plugins/global-notifier.plugin.js`. + */ +class GlobalNotifierLib extends BaseNotifierLib { + constructor (logger, notifier) { + // This is here more to make it clear that we expect these args to be provided. BaseNotifierLib has the built-in + // ability to instantiate them if not provided. But for our use case in global-notifier.plugin.js we want to ensure + // we are using the existing instances. + if (!logger || !notifier) { + throw new Error('new instance of GlobalNotifierLib is missing a required argument') + } + + super(logger, notifier) + } + + _formatLogPacket (message, data) { + return { + message, + ...data + } + } + + _formatNotifyPacket (message, data) { + return { + message, + session: data + } + } +} + +module.exports = GlobalNotifierLib diff --git a/app/lib/request.lib.js b/app/lib/request.lib.js index 348115c143..a0b7ee6891 100644 --- a/app/lib/request.lib.js +++ b/app/lib/request.lib.js @@ -49,6 +49,10 @@ async function get (url, additionalOptions = {}) { result.response = error } + if (!result.succeeded) { + _logFailure('GET', result, url, additionalOptions) + } + return result } @@ -72,6 +76,10 @@ async function post (url, additionalOptions = {}) { result.response = error } + if (!result.succeeded) { + _logFailure('POST', result, url, additionalOptions) + } + return result } @@ -85,6 +93,19 @@ async function _importGot () { return got } +function _logFailure (method, result, url, additionalOptions) { + const data = { + method, + result, + url, + additionalOptions + } + + const severity = result.response instanceof Error ? 'errored' : 'failed' + + global.GlobalNotifier.omfg(`${method} request ${severity}`, data) +} + /** * Combines the custom Got options provided by the caller with our defaults * diff --git a/app/plugins/global-notifier.plugin.js b/app/plugins/global-notifier.plugin.js new file mode 100644 index 0000000000..ec331ad974 --- /dev/null +++ b/app/plugins/global-notifier.plugin.js @@ -0,0 +1,17 @@ +'use strict' + +/** + * Plugin to add a globally available notifier for logging and sending exceptions to Errbit + * @module GlobalNotifierPlugin + */ + +const GlobalNotifierLib = require('../lib/global-notifier.lib.js') + +const GlobalNotifierPlugin = { + name: 'global-notifier', + register: (server, _options) => { + global.GlobalNotifier = new GlobalNotifierLib(server.logger, server.app.airbrake) + } +} + +module.exports = GlobalNotifierPlugin diff --git a/app/server.js b/app/server.js index 79a0971436..2945181579 100644 --- a/app/server.js +++ b/app/server.js @@ -6,6 +6,7 @@ const AirbrakePlugin = require('./plugins/airbrake.plugin.js') const BlippPlugin = require('./plugins/blipp.plugin.js') const ChargingModuleTokenCachePlugin = require('./plugins/charging-module-token-cache.plugin.js') const ErrorPagesPlugin = require('./plugins/error-pages.plugin.js') +const GlobalNotifierPlugin = require('./plugins/global-notifier.plugin.js') const HapiPinoPlugin = require('./plugins/hapi-pino.plugin.js') const RequestNotifierPlugin = require('./plugins/request-notifier.plugin.js') const RouterPlugin = require('./plugins/router.plugin.js') @@ -21,6 +22,7 @@ const registerPlugins = async (server) => { await server.register(RouterPlugin) await server.register(HapiPinoPlugin()) await server.register(AirbrakePlugin) + await server.register(GlobalNotifierPlugin) await server.register(ChargingModuleTokenCachePlugin) await server.register(ErrorPagesPlugin) await server.register(RequestNotifierPlugin) diff --git a/test/lib/global-notifier.lib.test.js b/test/lib/global-notifier.lib.test.js new file mode 100644 index 0000000000..8c5475c16e --- /dev/null +++ b/test/lib/global-notifier.lib.test.js @@ -0,0 +1,80 @@ +'use strict' + +// Test framework dependencies +const Lab = require('@hapi/lab') +const Code = require('@hapi/code') +const Sinon = require('sinon') + +const { describe, it, beforeEach, afterEach } = exports.lab = Lab.script() +const { expect } = Code + +// Things we need to stub +const BaseNotifierLib = require('../../app/lib/base-notifier.lib.js') + +// Thing under test +const GlobalNotifierLib = require('../../app/lib/global-notifier.lib.js') + +describe('GlobalNotifierLib class', () => { + let airbrakeFake + let pinoFake + + beforeEach(async () => { + airbrakeFake = { notify: Sinon.fake.resolves({ id: 1 }), flush: Sinon.fake() } + Sinon.stub(BaseNotifierLib.prototype, '_setNotifier').returns(airbrakeFake) + + pinoFake = { info: Sinon.fake(), error: Sinon.fake() } + Sinon.stub(BaseNotifierLib.prototype, '_setLogger').returns(pinoFake) + }) + + afterEach(() => { + Sinon.restore() + }) + + describe('#constructor', () => { + describe("when the 'logger' argument is not provided", () => { + it('throws an error', () => { + expect(() => new GlobalNotifierLib(null, airbrakeFake)).to.throw() + }) + }) + + describe("when the 'notifier' argument is not provided", () => { + it('throws an error', () => { + expect(() => new GlobalNotifierLib(pinoFake)).to.throw() + }) + }) + }) + + describe('when a log entry is made', () => { + const id = '1234567890' + const message = 'say what test' + + it('formats it as expected', () => { + const expectedArgs = { + message, + id + } + const testNotifier = new GlobalNotifierLib(pinoFake, airbrakeFake) + testNotifier.omg(message, { id }) + + expect(pinoFake.info.calledOnceWith(expectedArgs)).to.be.true() + }) + }) + + describe('when an airbrake notification is sent', () => { + const message = 'hell no test' + const data = { offTheChart: true } + + it('formats it as expected', () => { + const expectedArgs = { + message, + session: { + ...data + } + } + const testNotifier = new GlobalNotifierLib(pinoFake, airbrakeFake) + testNotifier.omfg(message, data) + + expect(airbrakeFake.notify.calledOnceWith(expectedArgs)).to.be.true() + }) + }) +}) diff --git a/test/lib/request.lib.test.js b/test/lib/request.lib.test.js index 5baba8d898..090dc59dd7 100644 --- a/test/lib/request.lib.test.js +++ b/test/lib/request.lib.test.js @@ -17,10 +17,20 @@ const RequestLib = require('../../app/lib/request.lib.js') describe('RequestLib', () => { const testDomain = 'http://example.com' + let notifierStub + + beforeEach(() => { + // RequestLib depends on the GlobalNotifier to have been set. This happens in app/plugins/global-notifier.plugin.js + // when the app starts up and the plugin is registered. As we're not creating an instance of Hapi server in this + // test we recreate the condition by setting it directly with our own stub + notifierStub = { omfg: Sinon.stub() } + global.GlobalNotifier = notifierStub + }) afterEach(() => { Sinon.restore() Nock.cleanAll() + delete global.GlobalNotifier }) describe('#get()', () => { @@ -52,6 +62,12 @@ describe('RequestLib', () => { Nock(testDomain).get('/').reply(500, { data: 'hello world' }) }) + it('records the failure', async () => { + await RequestLib.get(testDomain) + + expect(notifierStub.omfg.calledWith('GET request failed')).to.be.true() + }) + describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { const result = await RequestLib.get(testDomain) @@ -74,6 +90,12 @@ describe('RequestLib', () => { Nock(testDomain).get('/').replyWithError({ code: 'ECONNRESET' }) }) + it('records the error', async () => { + await RequestLib.get(testDomain) + + expect(notifierStub.omfg.calledWith('GET request errored')).to.be.true() + }) + describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { const result = await RequestLib.get(testDomain) @@ -218,6 +240,12 @@ describe('RequestLib', () => { Nock(testDomain).post('/').reply(500, { data: 'hello world' }) }) + it('records the failure', async () => { + await RequestLib.post(testDomain) + + expect(notifierStub.omfg.calledWith('POST request failed')).to.be.true() + }) + describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { const result = await RequestLib.post(testDomain) @@ -240,6 +268,12 @@ describe('RequestLib', () => { Nock(testDomain).post('/').replyWithError({ code: 'ECONNRESET' }) }) + it('records the error', async () => { + await RequestLib.post(testDomain) + + expect(notifierStub.omfg.calledWith('POST request errored')).to.be.true() + }) + describe('the result it returns', () => { it("has a 'succeeded' property marked as false", async () => { const result = await RequestLib.post(testDomain) diff --git a/test/plugins/global-notifier.plugin.test.js b/test/plugins/global-notifier.plugin.test.js new file mode 100644 index 0000000000..65a2d7cccb --- /dev/null +++ b/test/plugins/global-notifier.plugin.test.js @@ -0,0 +1,31 @@ +'use strict' + +// Test framework dependencies +const Lab = require('@hapi/lab') +const Code = require('@hapi/code') + +const { describe, it, beforeEach } = exports.lab = Lab.script() +const { expect } = Code + +// Test helpers +const GlobalNotifierLib = require('../../app/lib/global-notifier.lib.js') + +// For running our service +const { init } = require('../../app/server.js') + +describe('Global Notifier plugin', () => { + beforeEach(async () => { + // Create server before each test + await init() + }) + + describe('Global Notifier Plugin', () => { + describe('when the server is initialised', () => { + it('makes an instance of GlobalNotifierLib available globally', async () => { + const result = global.GlobalNotifier + + expect(result).to.be.an.instanceOf(GlobalNotifierLib) + }) + }) + }) +})