diff --git a/app/plugins/hapi-pino.plugin.js b/app/plugins/hapi-pino.plugin.js index 670c3e9832..367996e0b0 100644 --- a/app/plugins/hapi-pino.plugin.js +++ b/app/plugins/hapi-pino.plugin.js @@ -13,44 +13,24 @@ const HapiPino = require('hapi-pino') const HapiPinoIgnoreRequestService = require('../services/plugins/hapi-pino-ignore-request.service.js') +const HapiPinoLogInTestService = require('../services/plugins/hapi-pino-log-in-test.service.js') -/** - * Return test configuration options for the logger - * - * When we run our unit tests we don't want the output polluted by noise from the logger. So as a default we set the - * configuration to tell hapi-pino to ignore all events. - * - * But there will be times when trying to diagnose an issue that we will want log output. So using an env var we can - * override the default and tell hapi-pino to log everything as normal. - * - * In both cases using the - * {@link https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Spread_syntax|spread operator} on - * the returned value will allow it to be incorporated with our default hapi-pino options. - */ -const testOptions = logInTest => { - if (process.env.NODE_ENV !== 'test' || logInTest) { - return {} - } - - return { - // Don't log requests etc - logEvents: false, - // Don't log anything tagged with DEBUG or info, for example, req.log(['INFO'], 'User is an admin') - ignoredEventTags: { log: ['DEBUG', 'INFO'], request: ['DEBUG', 'INFO'] } - } -} +const LogConfig = require('../../config/log.config.js') -const HapiPinoPlugin = logInTest => { +const HapiPinoPlugin = () => { return { plugin: HapiPino, options: { // Include our test configuration - ...testOptions(logInTest), + ...HapiPinoLogInTestService.go(LogConfig.logInTest), // When not in the production environment we want a 'pretty' version of the JSON to make it easier to grok what has // happened transport: process.env.NODE_ENV !== 'production' ? { target: 'pino-pretty', options: { colorize: true } } : undefined, // Redact Authorization headers, see https://getpino.io/#/docs/redaction redact: ['req.headers.authorization'], + // Adding this here means it will be passed to HapiPinoIgnoreRequestService.go() within the `options` arg when + // Hapi-pino uses the ignoreFunc property + logAssetRequests: LogConfig.logAssetRequests, // We want our logs to focus on the main requests and not become full of 'noise' from requests for /assets or // pings from the AWS load balancer to /status. We pass this function to hapi-pino to control what gets filtered // https://github.com/pinojs/hapi-pino#optionsignorefunc-options-request--boolean diff --git a/app/server.js b/app/server.js index cada31d5d1..c874a05246 100644 --- a/app/server.js +++ b/app/server.js @@ -14,14 +14,13 @@ const StopPlugin = require('./plugins/stop.plugin.js') const ViewsPlugin = require('./plugins/views.plugin.js') const ServerConfig = require('../config/server.config.js') -const LogConfig = require('../config/log.config.js') const registerPlugins = async (server) => { // Register the remaining plugins await server.register(StopPlugin) await server.register(require('@hapi/inert')) await server.register(RouterPlugin) - await server.register(HapiPinoPlugin(LogConfig.logInTest)) + await server.register(HapiPinoPlugin()) await server.register(AirbrakePlugin) await server.register(ErrorPagesPlugin) await server.register(RequestNotifierPlugin) diff --git a/app/services/plugins/hapi-pino-ignore-request.service.js b/app/services/plugins/hapi-pino-ignore-request.service.js index 6b46e6ce7c..b5b44ad9cc 100644 --- a/app/services/plugins/hapi-pino-ignore-request.service.js +++ b/app/services/plugins/hapi-pino-ignore-request.service.js @@ -5,8 +5,6 @@ * @module HapiPinoIgnoreRequestService */ -const LogConfig = require('../../../config/log.config.js') - /** * Returns true or false whether a request should be loged * @@ -38,8 +36,8 @@ const LogConfig = require('../../../config/log.config.js') * * @returns {boolean} true if the request should be ignored, else false */ -function go (_options, request) { - const staticPaths = ['/', '/status'] +function go (options, request) { + const staticPaths = ['/', '/status', '/favicon.ico'] // If request is a known path ignore it if (staticPaths.includes(request.path)) { @@ -47,7 +45,7 @@ function go (_options, request) { } // If logging asset requests is disabled and the request is for an asset ignore it - if (!LogConfig.logAssetRequests && request.path.startsWith('/assets')) { + if (!options.logAssetRequests && request.path.startsWith('/assets')) { return true } diff --git a/app/services/plugins/hapi-pino-log-in-test.service.js b/app/services/plugins/hapi-pino-log-in-test.service.js new file mode 100644 index 0000000000..147107cfc5 --- /dev/null +++ b/app/services/plugins/hapi-pino-log-in-test.service.js @@ -0,0 +1,34 @@ +'use strict' + +/** + * Used by HapiPinoPlugin to determine which requests to log + * @module HapiPinoLogInTestService + */ + +/** + * Returns test configuration options for the hapi-pino logger + * + * When we run our unit tests we don't want the output polluted by noise from the logger. So as a default we set the + * configuration to tell hapi-pino to ignore all events. + * + * But there will be times when trying to diagnose an issue that we will want log output. So using an env var we can + * override the default and tell hapi-pino to log everything as normal. + * + * @returns {Object} an empty object or one containing Hapi-pino config to tell it not to log events + */ +function go (logInTest) { + if (process.env.NODE_ENV !== 'test' || logInTest) { + return {} + } + + return { + // Don't log requests etc + logEvents: false, + // Don't log anything tagged with DEBUG or info, for example, req.log(['INFO'], 'User is an admin') + ignoredEventTags: { log: ['DEBUG', 'INFO'], request: ['DEBUG', 'INFO'] } + } +} + +module.exports = { + go +} diff --git a/test/services/plugins/hapi-pino-ignore-request.service.test.js b/test/services/plugins/hapi-pino-ignore-request.service.test.js index 1ac210972f..477b9f7a0f 100644 --- a/test/services/plugins/hapi-pino-ignore-request.service.test.js +++ b/test/services/plugins/hapi-pino-ignore-request.service.test.js @@ -3,27 +3,17 @@ // 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 { describe, it } = exports.lab = Lab.script() const { expect } = Code -// Things we need to stub -const LogConfig = require('../../../config/log.config.js') - // Thing under test const HapiPinoIgnoreRequestService = require('../../../app/services/plugins/hapi-pino-ignore-request.service.js') describe('Hapi Pino Ignore Request service', () => { - const _options = {} - - afterEach(() => { - Sinon.restore() - }) - describe("when the request is for the root '/'", () => { it('returns true', () => { - const result = HapiPinoIgnoreRequestService.go(_options, { path: '/' }) + const result = HapiPinoIgnoreRequestService.go({ logAssetRequests: false }, { path: '/' }) expect(result).to.be.true() }) @@ -31,7 +21,7 @@ describe('Hapi Pino Ignore Request service', () => { describe("when the request is for '/status'", () => { it('returns true', () => { - const result = HapiPinoIgnoreRequestService.go(_options, { path: '/status' }) + const result = HapiPinoIgnoreRequestService.go({ logAssetRequests: false }, { path: '/status' }) expect(result).to.be.true() }) @@ -39,24 +29,16 @@ describe('Hapi Pino Ignore Request service', () => { describe('when the request is for an asset', () => { describe('and LOG_ASSET_REQUESTS is false', () => { - beforeEach(() => { - Sinon.replace(LogConfig, 'logAssetRequests', false) - }) - it('returns true', () => { - const result = HapiPinoIgnoreRequestService.go(_options, { path: '/assets/stylesheets/application.css' }) + const result = HapiPinoIgnoreRequestService.go({ logAssetRequests: false }, { path: '/assets/stylesheets/application.css' }) expect(result).to.be.true() }) }) describe('and LOG_ASSET_REQUESTS is true', () => { - beforeEach(() => { - Sinon.replace(LogConfig, 'logAssetRequests', true) - }) - it('returns false', () => { - const result = HapiPinoIgnoreRequestService.go(_options, { path: '/assets/stylesheets/application.css' }) + const result = HapiPinoIgnoreRequestService.go({ logAssetRequests: true }, { path: '/assets/stylesheets/application.css' }) expect(result).to.be.false() }) @@ -65,7 +47,7 @@ describe('Hapi Pino Ignore Request service', () => { describe("when the request is not for '/status' or an asset", () => { it('returns false', () => { - const result = HapiPinoIgnoreRequestService.go(_options, { path: '/bill-run/stuff' }) + const result = HapiPinoIgnoreRequestService.go({ logAssetRequests: false }, { path: '/bill-run/stuff' }) expect(result).to.be.false() }) diff --git a/test/services/plugins/hapi-pino-log-in-test.service.test.js b/test/services/plugins/hapi-pino-log-in-test.service.test.js new file mode 100644 index 0000000000..ad3dc11ebd --- /dev/null +++ b/test/services/plugins/hapi-pino-log-in-test.service.test.js @@ -0,0 +1,61 @@ +'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 + +// Thing under test +const HapiPinoLogInTestService = require('../../../app/services/plugins//hapi-pino-log-in-test.service.js') + +describe('Hapi Pino Log In Test service', () => { + afterEach(() => { + Sinon.restore() + }) + + describe('when unit tests are running', () => { + describe('and we tell it to log events', () => { + it('returns an empty object - hapi-pino is not silenced', () => { + const result = HapiPinoLogInTestService.go(true) + + expect(result).to.equal({}) + }) + }) + + describe('and we tell it not to log events in test', () => { + it('returns an object containing config to silence hapi-pino', () => { + const result = HapiPinoLogInTestService.go(false) + + expect(result).to.equal({ + logEvents: false, + ignoredEventTags: { log: ['DEBUG', 'INFO'], request: ['DEBUG', 'INFO'] } + }) + }) + }) + }) + + describe('when unit tests are not running', () => { + beforeEach(() => { + Sinon.stub(process, 'env').value({ ...process.env, NODE_ENV: 'development' }) + }) + + describe('and we tell it not to log events in test', () => { + it('returns an empty object - hapi-pino is not silenced', () => { + const result = HapiPinoLogInTestService.go(false) + + expect(result).to.equal({}) + }) + }) + + describe('and we tell it to log events in test', () => { + it('returns an empty object - hapi-pino is not silenced', () => { + const result = HapiPinoLogInTestService.go(true) + + expect(result).to.equal({}) + }) + }) + }) +})