Skip to content

Commit f1ccbf2

Browse files
clokclok
authored
ML-1213 Move to server event response to trigger send of metrics (#14)
* ML-1213 Move to server event response to trigger send of metrics * convert from jasmine to jest * cleaned up spec dir * added helper methods tests * lunt:fux * Added debug logger for plugin * lint:fix * adjust logging * add protection for client killing connection
1 parent 4120da8 commit f1ccbf2

10 files changed

+3098
-632
lines changed

‎.eslintrc

+3
Original file line numberDiff line numberDiff line change
@@ -5,5 +5,8 @@
55
},
66
"parserOptions": {
77
"sourceType": "module"
8+
},
9+
"rules": {
10+
"no-restricted-syntax": ["error", "ForInStatement", "LabeledStatement", "WithStatement"]
811
}
912
}

‎.npmignore

+5
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
*
2+
!lib/**
3+
!index.js
4+
!CHANGELOG.md
5+
!yarn.lock

‎lib/index.js

+34-16
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
const DogstatsdClient = require('dog-statsy');
22
const Hoek = require('@hapi/hoek');
3+
const debug = require('debug')('hapi:plugins:dogstatsd');
4+
const { get, set } = require('lodash');
35

46
const defaults = {
57
dogstatsdClient: null,
@@ -41,6 +43,18 @@ const tagMap = {
4143
http_method: getHttpMethod
4244
};
4345

46+
exports.injectMetricTags = ({ request, tags }) => {
47+
const ogTags = get(request, 'plugins.dogstatsd.tags', []);
48+
set(request, 'plugins.dogstatsd.tags', [...ogTags, ...tags]);
49+
return true;
50+
};
51+
52+
exports.injectMetrics = ({ request, metrics }) => {
53+
const ogMetrics = get(request, 'plugins.dogstatsd.metrics', []);
54+
set(request, 'plugins.dogstatsd.metrics', [...ogMetrics, ...metrics]);
55+
return true;
56+
};
57+
4458
exports.register = (server, options) => {
4559
const settings = Hoek.applyToDefaults(defaults, options || {});
4660
// Filter list of default tags, removing excludedTags
@@ -55,9 +69,19 @@ exports.register = (server, options) => {
5569

5670
server.decorate('server', 'dogstatsd', dogstatsdClient);
5771

58-
server.ext('onPreResponse', (request, h) => {
72+
// We want to ensure that the metrics are sent after any `onPreResponse` hooks
73+
server.events.on('response', (request) => {
74+
/*
75+
Handle case when client kills the connection
76+
https://github.com/hapijs/hapi/blob/master/API.md#-response-event
77+
*/
78+
if (!request.response) {
79+
return;
80+
}
81+
82+
// Skip excluded URLs
5983
if (settings.excludedPaths.indexOf(request.url.pathname) !== -1) {
60-
return h.continue;
84+
return;
6185
}
6286

6387
const startDate = new Date(request.info.received);
@@ -68,6 +92,8 @@ exports.register = (server, options) => {
6892
prev.push(`${tag}:${tagMap[tag](request)}`);
6993
return prev;
7094
}, []);
95+
const stateTags = get(request, 'plugins.dogstatsd.tags', []);
96+
Hoek.merge(localTags, stateTags);
7197

7298
const defaultMetrics = [{
7399
type: 'incr',
@@ -85,23 +111,15 @@ exports.register = (server, options) => {
85111
value: ms,
86112
localTags
87113
}];
88-
89-
let stateTags = [];
90-
let stateMetrics = [];
91-
if (request.plugins.dogstatsd) {
92-
stateTags = request.plugins.dogstatsd.tags;
93-
stateMetrics = request.plugins.dogstatsd.metrics;
94-
}
95-
96-
Hoek.merge(localTags, stateTags);
114+
const stateMetrics = get(request, 'plugins.dogstatsd.metrics', []);
97115
Hoek.merge(defaultMetrics, stateMetrics);
98116

99-
defaultMetrics.forEach((metric) => {
117+
for (const metric of defaultMetrics) {
100118
const { type, name, value, tags = [] } = metric;
101-
dogstatsdClient[type](name, value, [...localTags, ...tags]);
102-
});
103-
104-
return h.continue;
119+
const combinedTags = [...localTags, ...tags];
120+
debug({ type, name, value, tags: combinedTags });
121+
dogstatsdClient[type](name, value, combinedTags);
122+
}
105123
});
106124
};
107125

‎package.json

+45-8
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,10 @@
44
"description": "Hapi plugin for publishing route stats to DataDog",
55
"main": "index.js",
66
"scripts": {
7-
"test": "cross-env NODE_ENV=test nyc jasmine",
7+
"test": "npm run test:jest -- --coverage",
8+
"test:jest": "cross-env NODE_ENV=test jest ",
9+
"test:watch": "npm run test:jest -- --watch",
10+
"test:results": "open ./coverage/lcov-report/index.html",
811
"lint": "eslint .",
912
"lint:fix": "eslint --fix .",
1013
"version": "auto-changelog -p -l false --template keepachangelog && git add CHANGELOG.md"
@@ -57,22 +60,56 @@
5760
"cache": false,
5861
"check-coverage": false
5962
},
63+
"jest": {
64+
"clearMocks": true,
65+
"coverageDirectory": "<rootDir>/coverage",
66+
"coverageReporters": [
67+
"lcov",
68+
"text",
69+
"text-summary",
70+
"json"
71+
],
72+
"collectCoverageFrom": [
73+
"lib/**/*.{js,jsx,mjs}",
74+
"lib/index.js"
75+
],
76+
"setupFiles": [],
77+
"testMatch": [
78+
"<rootDir>/spec/**/?(*.)(spec|test).{js,jsx,mjs}"
79+
],
80+
"testEnvironment": "node",
81+
"transform": {},
82+
"transformIgnorePatterns": [
83+
"[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs)$"
84+
],
85+
"moduleNameMapper": {},
86+
"modulePaths": [
87+
"<rootDir>/lib"
88+
],
89+
"moduleFileExtensions": [
90+
"web.js",
91+
"mjs",
92+
"js",
93+
"json",
94+
"web.jsx",
95+
"jsx",
96+
"node"
97+
]
98+
},
6099
"devDependencies": {
61100
"@hapi/hapi": "^18.3.1",
62101
"auto-changelog": "^1.1.0",
63102
"cross-env": "^5.0.5",
64103
"eslint": "^4.8.0",
65104
"eslint-config-goodway": "^1.3.0",
66105
"eslint-plugin-import": "^2.7.0",
67-
"jasmine": "^3.0.0",
68-
"jasmine-core": "^3.0.0",
69-
"jasmine-reporters": "^2.2.1",
70-
"nock": "^9.0.22",
71-
"nyc": "^11.2.1",
106+
"jest": "^24.8.0",
72107
"pre-push": "^0.1.1"
73108
},
74109
"dependencies": {
75-
"@hapi/hoek": "^7.2.1",
76-
"dog-statsy": "^1.2.1"
110+
"@hapi/hoek": "^8.2.0",
111+
"debug": "^4.1.1",
112+
"dog-statsy": "^1.2.1",
113+
"lodash": "^4.17.15"
77114
}
78115
}

‎spec/.eslintrc

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
{
22
env: {
3-
jasmine: true
3+
jest: true
44
}
55
}

‎spec/hapi-dogstatsd.spec.js

+27-27
Original file line numberDiff line numberDiff line change
@@ -9,10 +9,10 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
99

1010
beforeEach(async () => {
1111
mockStatsdClient = {
12-
incr: jasmine.createSpy('incr'),
13-
gauge: jasmine.createSpy('gauge'),
14-
timer: jasmine.createSpy('timer'),
15-
booyah: jasmine.createSpy('booyah')
12+
incr: jest.fn(),
13+
gauge: jest.fn(),
14+
timer: jest.fn(),
15+
booyah: jest.fn()
1616
};
1717

1818
server = new Hapi.Server({
@@ -65,41 +65,41 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
6565
const tags = ['dns:localhost_8085', 'url_path:/', 'route_path:/', 'status_code:200', 'http_method:GET'];
6666
await server.inject('/');
6767
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
68-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
69-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
68+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
69+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
7070
});
7171

7272
it('should report stats with path name set explicitly', async () => {
7373
const tags = ['dns:localhost_8085', 'url_path:/test/path', 'route_path:/test/{param}', 'status_code:200', 'http_method:GET'];
7474
await server.inject('/test/path');
7575
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
76-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
77-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
76+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
77+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
7878
});
7979

8080
it('should report stats with merging tags from route', async () => {
8181
const tags = ['dns:localhost_8085', 'url_path:/test/withtags', 'route_path:/test/withtags', 'status_code:200', 'http_method:GET', 'tag1:true', 'tag2:false'];
8282
await server.inject('/test/withtags');
8383
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
84-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
85-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
84+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
85+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
8686
});
8787

8888
it('should report stats with merging metrics from route', async () => {
8989
const tags = ['dns:localhost_8085', 'url_path:/test/withmetrics', 'route_path:/test/withmetrics', 'status_code:200', 'http_method:GET'];
9090
await server.inject('/test/withmetrics');
9191
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
92-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
93-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
94-
expect(mockStatsdClient.booyah).toHaveBeenCalledWith('rick.morty', jasmine.any(Number), [...tags, 'tag:special']);
92+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
93+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
94+
expect(mockStatsdClient.booyah).toHaveBeenCalledWith('rick.morty', expect.any(Number), [...tags, 'tag:special']);
9595
});
9696

9797
it('should report proper HTTP status', async () => {
9898
const tags = ['dns:localhost_8085', 'url_path:/notFound', 'route_path:/{notFound*}', 'status_code:404', 'http_method:GET'];
9999
await server.inject('/notFound');
100100
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
101-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
102-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
101+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
102+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
103103
});
104104

105105
it('should report report the proper HTTP method', async () => {
@@ -112,17 +112,17 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
112112
url: '/'
113113
});
114114
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
115-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
116-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
115+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
116+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
117117
});
118118

119119
it('should not change the status code of a response', async () => {
120120
const tags = ['dns:localhost_8085', 'url_path:/throwError', 'route_path:/throwError', 'status_code:500', 'http_method:GET'];
121121
const res = await server.inject('/throwError');
122122
expect(res.statusCode).toBe(500);
123123
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
124-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
125-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
124+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
125+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
126126
});
127127

128128
it('should not report stats for /health-check', async () => {
@@ -148,9 +148,9 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
148148

149149
beforeEach(async () => {
150150
mockStatsdClient = {
151-
incr: jasmine.createSpy('incr'),
152-
gauge: jasmine.createSpy('gauge'),
153-
timer: jasmine.createSpy('timer')
151+
incr: jest.fn(),
152+
gauge: jest.fn(),
153+
timer: jest.fn()
154154
};
155155

156156
server = new Hapi.Server({
@@ -172,9 +172,9 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
172172

173173
beforeEach(async () => {
174174
mockStatsdClient = {
175-
incr: jasmine.createSpy('incr'),
176-
gauge: jasmine.createSpy('gauge'),
177-
timer: jasmine.createSpy('timer')
175+
incr: jest.fn(),
176+
gauge: jest.fn(),
177+
timer: jest.fn()
178178
};
179179

180180
server = new Hapi.Server({
@@ -199,8 +199,8 @@ describe('lib-hapi-dogstatsd plugin tests', () => {
199199
const tags = ['url_path:/test', 'status_code:200', 'http_method:GET'];
200200
await server.inject('/test');
201201
expect(mockStatsdClient.incr).toHaveBeenCalledWith('route.hits', null, tags);
202-
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', jasmine.any(Number), tags);
203-
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', jasmine.any(Number), tags);
202+
expect(mockStatsdClient.gauge).toHaveBeenCalledWith('route.response_time', expect.any(Number), tags);
203+
expect(mockStatsdClient.timer).toHaveBeenCalledWith('route', expect.any(Number), tags);
204204
});
205205
});
206206

0 commit comments

Comments
 (0)