Skip to content

Commit

Permalink
Refactor Hapi-pino logger to clean things up (#49)
Browse files Browse the repository at this point in the history
The first tweak is around config. We were pulling it into our main `app/server.js` and `HapiPinoIgnoreRequestService`. This meant we also needed to stub the config to get tests working for the `HapiPinoIgnoreRequestService`. By moving the config into the plugin we can use it where it's needed, and ensure it gets passed through as an arg. This means `LogConfig` is now only referenced in one place and we can remove the stubbing in our `HapiPinoIgnoreRequestService` tests because we're passing the critical option through.

The second was a mix of conventions. We were using a separate service to contain the logic of whether a request should be logged but a function in the plugin to cover logging in tests. By moving the logging in tests logic to a service we get consistency _and_ we can write some unit tests to cover it.
  • Loading branch information
Cruikshanks authored Dec 12, 2022
1 parent f5b1d68 commit 11632f2
Show file tree
Hide file tree
Showing 6 changed files with 112 additions and 58 deletions.
34 changes: 7 additions & 27 deletions app/plugins/hapi-pino.plugin.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
3 changes: 1 addition & 2 deletions app/server.js
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
8 changes: 3 additions & 5 deletions app/services/plugins/hapi-pino-ignore-request.service.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@
* @module HapiPinoIgnoreRequestService
*/

const LogConfig = require('../../../config/log.config.js')

/**
* Returns true or false whether a request should be loged
*
Expand Down Expand Up @@ -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
}

Expand Down
34 changes: 34 additions & 0 deletions app/services/plugins/hapi-pino-log-in-test.service.js
Original file line number Diff line number Diff line change
@@ -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
}
30 changes: 6 additions & 24 deletions test/services/plugins/hapi-pino-ignore-request.service.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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()
})
Expand All @@ -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()
})
Expand Down
61 changes: 61 additions & 0 deletions test/services/plugins/hapi-pino-log-in-test.service.test.js
Original file line number Diff line number Diff line change
@@ -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({})
})
})
})
})

0 comments on commit 11632f2

Please sign in to comment.