ML-1213 Move to server event response to trigger send of metrics

.eslintrc

@@ -5,5 +5,8 @@
   },
   "parserOptions": {
     "sourceType": "module"
+  },
+  "rules": {
+    "no-restricted-syntax": ["error", "ForInStatement", "LabeledStatement", "WithStatement"]
   }
 }

.npmignore

@@ -0,0 +1,5 @@
+*
+!lib/**
+!index.js
+!CHANGELOG.md
+!yarn.lock

lib/index.js

@@ -1,5 +1,7 @@
 const DogstatsdClient = require('dog-statsy');
 const Hoek = require('@hapi/hoek');
+const debug = require('debug')('hapi:plugins:dogstatsd');
+const { get, set } = require('lodash');
 
 const defaults = {
     dogstatsdClient: null,
@@ -41,6 +43,18 @@ const tagMap = {
     http_method: getHttpMethod
 };
 
+exports.injectMetricTags = ({ request, tags }) => {
+    const ogTags = get(request, 'plugins.dogstatsd.tags', []);
+    set(request, 'plugins.dogstatsd.tags', [...ogTags, ...tags]);
+    return true;
+};
+
+exports.injectMetrics = ({ request, metrics }) => {
+    const ogMetrics = get(request, 'plugins.dogstatsd.metrics', []);
+    set(request, 'plugins.dogstatsd.metrics', [...ogMetrics, ...metrics]);
+    return true;
+};
+
 exports.register = (server, options) => {
     const settings = Hoek.applyToDefaults(defaults, options || {});
     // Filter list of default tags, removing excludedTags
@@ -55,9 +69,19 @@ exports.register = (server, options) => {
 
     server.decorate('server', 'dogstatsd', dogstatsdClient);
 
-    server.ext('onPreResponse', (request, h) => {
+    // We want to ensure that the metrics are sent after any `onPreResponse` hooks
+    server.events.on('response', (request) => {
+        /*
+          Handle case when client kills the connection
+          https://github.com/hapijs/hapi/blob/master/API.md#-response-event
+        */
+        if (!request.response) {
+            return;
+        }
+
+        // Skip excluded URLs
         if (settings.excludedPaths.indexOf(request.url.pathname) !== -1) {
-            return h.continue;
+            return;
         }
 
         const startDate = new Date(request.info.received);
@@ -68,6 +92,8 @@ exports.register = (server, options) => {
             prev.push(`${tag}:${tagMap[tag](request)}`);
             return prev;
         }, []);
+        const stateTags = get(request, 'plugins.dogstatsd.tags', []);
+        Hoek.merge(localTags, stateTags);
 
         const defaultMetrics = [{
             type: 'incr',
@@ -85,23 +111,15 @@ exports.register = (server, options) => {
             value: ms,
             localTags
         }];
-
-        let stateTags = [];
-        let stateMetrics = [];
-        if (request.plugins.dogstatsd) {
-            stateTags = request.plugins.dogstatsd.tags;
-            stateMetrics = request.plugins.dogstatsd.metrics;
-        }
-
-        Hoek.merge(localTags, stateTags);
+        const stateMetrics = get(request, 'plugins.dogstatsd.metrics', []);
         Hoek.merge(defaultMetrics, stateMetrics);
 
-        defaultMetrics.forEach((metric) => {
+        for (const metric of defaultMetrics) {
             const { type, name, value, tags = [] } = metric;
-            dogstatsdClient[type](name, value, [...localTags, ...tags]);
-        });
-
-        return h.continue;
+            const combinedTags = [...localTags, ...tags];
+            debug({ type, name, value, tags: combinedTags });
+            dogstatsdClient[type](name, value, combinedTags);
+        }
     });
 };
 

package.json

@@ -4,7 +4,10 @@
   "description": "Hapi plugin for publishing route stats to DataDog",
   "main": "index.js",
   "scripts": {
-    "test": "cross-env NODE_ENV=test nyc jasmine",
+    "test": "npm run test:jest -- --coverage",
+    "test:jest": "cross-env NODE_ENV=test jest ",
+    "test:watch": "npm run test:jest -- --watch",
+    "test:results": "open ./coverage/lcov-report/index.html",
     "lint": "eslint .",
     "lint:fix": "eslint --fix .",
     "version": "auto-changelog -p -l false --template keepachangelog && git add CHANGELOG.md"
@@ -57,22 +60,56 @@
     "cache": false,
     "check-coverage": false
   },
+  "jest": {
+    "clearMocks": true,
+    "coverageDirectory": "<rootDir>/coverage",
+    "coverageReporters": [
+      "lcov",
+      "text",
+      "text-summary",
+      "json"
+    ],
+    "collectCoverageFrom": [
+      "lib/**/*.{js,jsx,mjs}",
+      "lib/index.js"
+    ],
+    "setupFiles": [],
+    "testMatch": [
+      "<rootDir>/spec/**/?(*.)(spec|test).{js,jsx,mjs}"
+    ],
+    "testEnvironment": "node",
+    "transform": {},
+    "transformIgnorePatterns": [
+      "[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs)$"
+    ],
+    "moduleNameMapper": {},
+    "modulePaths": [
+      "<rootDir>/lib"
+    ],
+    "moduleFileExtensions": [
+      "web.js",
+      "mjs",
+      "js",
+      "json",
+      "web.jsx",
+      "jsx",
+      "node"
+    ]
+  },
   "devDependencies": {
     "@hapi/hapi": "^18.3.1",
     "auto-changelog": "^1.1.0",
     "cross-env": "^5.0.5",
     "eslint": "^4.8.0",
     "eslint-config-goodway": "^1.3.0",
     "eslint-plugin-import": "^2.7.0",
-    "jasmine": "^3.0.0",
-    "jasmine-core": "^3.0.0",
-    "jasmine-reporters": "^2.2.1",
-    "nock": "^9.0.22",
-    "nyc": "^11.2.1",
+    "jest": "^24.8.0",
     "pre-push": "^0.1.1"
   },
   "dependencies": {
-    "@hapi/hoek": "^7.2.1",
-    "dog-statsy": "^1.2.1"
+    "@hapi/hoek": "^8.2.0",
+    "debug": "^4.1.1",
+    "dog-statsy": "^1.2.1",
+    "lodash": "^4.17.15"
   }
 }

spec/.eslintrc

@@ -1,5 +1,5 @@
 {
   env: {
-    jasmine: true
+    jest: true
   }
 }

spec/hapi-dogstatsd.spec.js

@@ -9,10 +9,10 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
 
         beforeEach(async () => {
             mockStatsdClient = {
-                incr: jasmine.createSpy('incr'),
-                gauge: jasmine.createSpy('gauge'),
-                timer: jasmine.createSpy('timer'),
-                booyah: jasmine.createSpy('booyah')
+                incr: jest.fn(),
+                gauge: jest.fn(),
+                timer: jest.fn(),
+                booyah: jest.fn()
             };
 
             server = new Hapi.Server({
@@ -65,41 +65,41 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
             const tags = ['dns:localhost_8085', 'url_path:/', 'route_path:/', 'status_code:200', 'http_method:GET'];
             await server.inject('/');
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
 
         it('should report stats with path name set explicitly', async () => {
             const tags = ['dns:localhost_8085', 'url_path:/test/path', 'route_path:/test/{param}', 'status_code:200', 'http_method:GET'];
             await server.inject('/test/path');
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
 
         it('should report stats with merging tags from route', async () => {
             const tags = ['dns:localhost_8085', 'url_path:/test/withtags', 'route_path:/test/withtags', 'status_code:200', 'http_method:GET', 'tag1:true', 'tag2:false'];
             await server.inject('/test/withtags');
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
 
         it('should report stats with merging metrics from route', async () => {
             const tags = ['dns:localhost_8085', 'url_path:/test/withmetrics', 'route_path:/test/withmetrics', 'status_code:200', 'http_method:GET'];
             await server.inject('/test/withmetrics');
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
-            expect(mockStatsdClient.booyah).toHaveBeenCalledWith('rick.morty', jasmine.any(Number), [...tags, 'tag:special']);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
+            expect(mockStatsdClient.booyah).toHaveBeenCalledWith('rick.morty', expect.any(Number), [...tags, 'tag:special']);
         });
 
         it('should report proper HTTP status', async () => {
             const tags = ['dns:localhost_8085', 'url_path:/notFound', 'route_path:/{notFound*}', 'status_code:404', 'http_method:GET'];
             await server.inject('/notFound');
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
 
         it('should report report the proper HTTP method', async () => {
@@ -112,17 +112,17 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
                 url: '/'
             });
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
 
         it('should not change the status code of a response', async () => {
             const tags = ['dns:localhost_8085', 'url_path:/throwError', 'route_path:/throwError', 'status_code:500', 'http_method:GET'];
             const res = await server.inject('/throwError');
             expect(res.statusCode).toBe(500);
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
 
         it('should not report stats for /health-check', async () => {
@@ -148,9 +148,9 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
 
         beforeEach(async () => {
             mockStatsdClient = {
-                incr: jasmine.createSpy('incr'),
-                gauge: jasmine.createSpy('gauge'),
-                timer: jasmine.createSpy('timer')
+                incr: jest.fn(),
+                gauge: jest.fn(),
+                timer: jest.fn()
             };
 
             server = new Hapi.Server({
@@ -172,9 +172,9 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
 
         beforeEach(async () => {
             mockStatsdClient = {
-                incr: jasmine.createSpy('incr'),
-                gauge: jasmine.createSpy('gauge'),
-                timer: jasmine.createSpy('timer')
+                incr: jest.fn(),
+                gauge: jest.fn(),
+                timer: jest.fn()
             };
 
             server = new Hapi.Server({
@@ -199,8 +199,8 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
             const tags = ['url_path:/test', 'status_code:200', 'http_method:GET'];
             await server.inject('/test');
             expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
-            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
-            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
+            expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
+            expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
         });
     });