Refactor Hapi-pino logger to clean things up

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

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)

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,16 +36,16 @@ 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)) {
     return true
   }
 
   // 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
   }
 

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
+}

test/services/plugins/hapi-pino-ignore-request.service.test.js

@@ -3,60 +3,42 @@
 // 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()
     })
   })
 
   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()
     })
   })
 
   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()
     })

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({})
+      })
+    })
+  })
+})