From 9d7faf71cd35f4e51521f8d832e2632d7a3df5e0 Mon Sep 17 00:00:00 2001 From: idantovi Date: Thu, 29 Nov 2018 19:09:30 +0200 Subject: [PATCH 1/4] fix sub route for express framework --- src/metrics-middleware.js | 17 ++-- .../express/middleware-test.js | 81 +++++++++++++++---- .../express/server/express-server.js | 2 + .../express/server/router.js | 31 +++++++ 4 files changed, 110 insertions(+), 21 deletions(-) create mode 100644 test/integration-tests/express/server/router.js diff --git a/src/metrics-middleware.js b/src/metrics-middleware.js index bb751fe..89c9387 100644 --- a/src/metrics-middleware.js +++ b/src/metrics-middleware.js @@ -105,14 +105,19 @@ function _getRoute(req) { let route = req.baseUrl; // express if (req.swagger) { // swagger route = req.swagger.apiPath; - } else if (req.route && route) { // express + } else if (req.route) { // express if (req.route.path !== '/') { route = route + req.route.path; } - } else if (req.url && !route) { // restify - route = req.url; - if (req.route) { - route = req.route.path; + + // In case you have base route and error handler in the root + let routeRegex = route; + routeRegex = routeRegex.replace(/\/:.+/, '/.+'); + routeRegex = routeRegex.replace(/\/:.+\//, '/.+/'); + let regex = new RegExp(routeRegex); + let regexMatch = regex.exec(req.originalUrl); + if (regexMatch && regexMatch.index > 0) { + route = req.originalUrl.substring(0, regexMatch.index) + route; } } @@ -127,7 +132,7 @@ function _getRoute(req) { // express framework and no route was found for the request. if we log this metrics // we'll risk in a memory leak since the route is not a pattern but a hardcoded string. if (!req.route && res && res.statusCode === 404) { - return undefined; + return 'N/A'; } return route; diff --git a/test/integration-tests/express/middleware-test.js b/test/integration-tests/express/middleware-test.js index 56c609a..c1f406f 100644 --- a/test/integration-tests/express/middleware-test.js +++ b/test/integration-tests/express/middleware-test.js @@ -124,6 +124,58 @@ describe('when using express framework', () => { }); }); }); + describe('when calling a GET endpoint with path params and inner router', () => { + before(() => { + return supertest(app) + .get('/v2/hello/200') + .expect(200) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('http_request_duration_seconds_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_response_size_bytes_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_request_size_bytes_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); + }); + }); + }); + describe('when calling a POST endpoint with inner router', () => { + before(() => { + return supertest(app) + .post('/v2/test') + .send({name: 'john'}) + .set('Accept', 'application/json') + .expect(201) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="POST",route="/v2/test",code="201"'); + }); + }); + }); + describe('when calling endpoint and getting an error with inner router', () => { + before(() => { + return supertest(app) + .get('/v2/bad/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/bad/:time",code="500"'); + }); + }); + }); describe('when using custom metrics', () => { before(() => { return supertest(app) @@ -140,22 +192,9 @@ describe('when using express framework', () => { }); }); }); - it('should get metrics as json', () => { - return supertest(app) - .get('/metrics.json') - .expect(200) - .then((res) => { - JSON.parse(res.text); - }); - }); - after(() => { - const Prometheus = require('prom-client'); - Prometheus.register.clear(); - }); describe('when calling not existing endpoint', function() { - let notExistingPath = '/notExistingPath' + Math.floor(Math.random() * 10); - before(() => { + let notExistingPath = '/notExistingPath' + Math.floor(Math.random() * 10); return supertest(app) .get(notExistingPath) .expect(404) @@ -166,10 +205,22 @@ describe('when using express framework', () => { .get('/metrics') .expect(200) .then((res) => { - expect(res.text).to.not.contain("method='GET',route='" + notExistingPath + "',code='404'"); + expect(res.text).to.contain('method="GET",route="N/A",code="404"'); }); }); }); + it('should get metrics as json', () => { + return supertest(app) + .get('/metrics.json') + .expect(200) + .then((res) => { + JSON.parse(res.text); + }); + }); + after(() => { + const Prometheus = require('prom-client'); + Prometheus.register.clear(); + }); }); describe('when start up with unique metric names', () => { let app; diff --git a/test/integration-tests/express/server/express-server.js b/test/integration-tests/express/server/express-server.js index 9f64b1f..1211b79 100644 --- a/test/integration-tests/express/server/express-server.js +++ b/test/integration-tests/express/server/express-server.js @@ -6,6 +6,7 @@ const bodyParser = require('body-parser'); const config = require('./config'); const app = express(); const middleware = require('../../../../src/index.js'); +const router = require('./router'); const checkoutsTotal = Prometheus.register.getSingleMetric('checkouts_total') || new Prometheus.Counter({ name: 'checkouts_total', @@ -15,6 +16,7 @@ const checkoutsTotal = Prometheus.register.getSingleMetric('checkouts_total') || app.use(middleware({ useUniqueHistogramName: config.useUniqueHistogramName })); app.use(bodyParser.json()); +app.use('/v2', router); app.get('/hello', (req, res, next) => { setTimeout(() => { diff --git a/test/integration-tests/express/server/router.js b/test/integration-tests/express/server/router.js new file mode 100644 index 0000000..1ed9070 --- /dev/null +++ b/test/integration-tests/express/server/router.js @@ -0,0 +1,31 @@ +'use strict'; +var express = require('express'); +var router = express.Router(); + +router.route('/').get(bad); +router.route('/bad').get(bad); +router.route('/bad/:time').get(bad); +router.route('/test').post(test); +router.route('/:time').patch(helloTime); +router.route('/hello/:time').get(helloTime); + +function test (req, res, next) { + setTimeout(() => { + res.status(201); + res.json({ message: 'Hello World!' }); + next(); + }, req.body.delay); +}; + +function helloTime (req, res, next) { + setTimeout(() => { + res.json({ message: 'Hello World!' }); + next(); + }, parseInt(req.param.time)); +}; + +function bad (req, res, next) { + next(new Error('My Error')); +}; + +module.exports = router; \ No newline at end of file From f678c8dd335842f690bfa687a14c2af33fd4d5c8 Mon Sep 17 00:00:00 2001 From: idantovi Date: Thu, 29 Nov 2018 20:34:55 +0200 Subject: [PATCH 2/4] add more tests for sub route scenarios and make sure to collect every request even if route not hit --- src/metrics-middleware.js | 25 +++--- .../express/middleware-test.js | 88 ++++++++++++++++++- .../express/server/express-server.js | 6 ++ .../express/server/router.js | 8 ++ 4 files changed, 114 insertions(+), 13 deletions(-) diff --git a/src/metrics-middleware.js b/src/metrics-middleware.js index 89c9387..98416a1 100644 --- a/src/metrics-middleware.js +++ b/src/metrics-middleware.js @@ -110,14 +110,18 @@ function _getRoute(req) { route = route + req.route.path; } - // In case you have base route and error handler in the root - let routeRegex = route; - routeRegex = routeRegex.replace(/\/:.+/, '/.+'); - routeRegex = routeRegex.replace(/\/:.+\//, '/.+/'); - let regex = new RegExp(routeRegex); - let regexMatch = regex.exec(req.originalUrl); - if (regexMatch && regexMatch.index > 0) { - route = req.originalUrl.substring(0, regexMatch.index) + route; + if (route === '') { + route = req.originalUrl; + } else { + // In case you have base route and error handler in the root + let routeRegex = route; + routeRegex = routeRegex.replace(/\/:.+/, '/.+'); + routeRegex = routeRegex.replace(/\/:.+\//, '/.+/'); + const regex = new RegExp(routeRegex); + const regexMatch = regex.exec(req.originalUrl); + if (regexMatch && regexMatch.index > 0) { + route = req.originalUrl.substring(0, regexMatch.index) + route; + } } } @@ -131,8 +135,9 @@ function _getRoute(req) { // this condition will evaluate to true only in // express framework and no route was found for the request. if we log this metrics // we'll risk in a memory leak since the route is not a pattern but a hardcoded string. - if (!req.route && res && res.statusCode === 404) { - return 'N/A'; + if (!route || route === '') { + // if (!req.route && res && res.statusCode === 404) { + route = 'N/A'; } return route; diff --git a/test/integration-tests/express/middleware-test.js b/test/integration-tests/express/middleware-test.js index c1f406f..fb54328 100644 --- a/test/integration-tests/express/middleware-test.js +++ b/test/integration-tests/express/middleware-test.js @@ -124,7 +124,7 @@ describe('when using express framework', () => { }); }); }); - describe('when calling a GET endpoint with path params and inner router', () => { + describe('when calling a GET endpoint with path params and sub router', () => { before(() => { return supertest(app) .get('/v2/hello/200') @@ -142,7 +142,7 @@ describe('when using express framework', () => { }); }); }); - describe('when calling a POST endpoint with inner router', () => { + describe('when calling a POST endpoint with sub router', () => { before(() => { return supertest(app) .post('/v2/test') @@ -160,7 +160,7 @@ describe('when using express framework', () => { }); }); }); - describe('when calling endpoint and getting an error with inner router', () => { + describe('when calling endpoint and getting an error with sub router with 1 variable', () => { before(() => { return supertest(app) .get('/v2/bad/500') @@ -176,6 +176,88 @@ describe('when using express framework', () => { }); }); }); + describe('when calling endpoint and getting an error with sub router with two variables', () => { + before(() => { + return supertest(app) + .get('/v2/bad/500/400') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/bad/:var1/:var2",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router with no variables', () => { + before(() => { + return supertest(app) + .get('/v2/bad') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/bad",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router (root)', () => { + before(() => { + return supertest(app) + .get('/v2') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error from a middleware before route', () => { + before(() => { + return supertest(app) + .get('/hello') + .set('error', 'error') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="N/A",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error from a middleware before sub route', () => { + before(() => { + return supertest(app) + .get('/v2/hello') + .set('error', 'error') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2",code="500"'); + }); + }); + }); describe('when using custom metrics', () => { before(() => { return supertest(app) diff --git a/test/integration-tests/express/server/express-server.js b/test/integration-tests/express/server/express-server.js index 1211b79..ef2d43f 100644 --- a/test/integration-tests/express/server/express-server.js +++ b/test/integration-tests/express/server/express-server.js @@ -16,6 +16,12 @@ const checkoutsTotal = Prometheus.register.getSingleMetric('checkouts_total') || app.use(middleware({ useUniqueHistogramName: config.useUniqueHistogramName })); app.use(bodyParser.json()); +app.use((req, res, next) => { + if (req.headers.error) { + next(new Error('Error')); + } + next(); +}); app.use('/v2', router); app.get('/hello', (req, res, next) => { diff --git a/test/integration-tests/express/server/router.js b/test/integration-tests/express/server/router.js index 1ed9070..8dcdea5 100644 --- a/test/integration-tests/express/server/router.js +++ b/test/integration-tests/express/server/router.js @@ -2,9 +2,17 @@ var express = require('express'); var router = express.Router(); +router.use((req, res, next) => { + if (req.headers.error) { + next(new Error('Error')); + } + next(); +}); + router.route('/').get(bad); router.route('/bad').get(bad); router.route('/bad/:time').get(bad); +router.route('/bad/:var1/:var2').get(bad); router.route('/test').post(test); router.route('/:time').patch(helloTime); router.route('/hello/:time').get(helloTime); From 7ae5d4870c92b002899ddf1f8e5bcb0b32791d29 Mon Sep 17 00:00:00 2001 From: idantovi Date: Mon, 3 Dec 2018 10:27:15 +0200 Subject: [PATCH 3/4] replace regex with string manipulation --- src/metrics-middleware.js | 17 +++++++---------- .../express/middleware-test.js | 16 ++++++++++++++++ test/integration-tests/express/server/router.js | 2 +- 3 files changed, 24 insertions(+), 11 deletions(-) diff --git a/src/metrics-middleware.js b/src/metrics-middleware.js index 98416a1..95d2d8e 100644 --- a/src/metrics-middleware.js +++ b/src/metrics-middleware.js @@ -5,6 +5,7 @@ require('pkginfo')(module, ['name']); const debug = require('debug')(module.exports.name); const utils = require('./utils'); const setupOptions = {}; +const routers = {}; module.exports = (appVersion, projectName) => { return (options = {}) => { @@ -101,7 +102,6 @@ function _handleResponse (req, res) { } function _getRoute(req) { - let res = req.res; let route = req.baseUrl; // express if (req.swagger) { // swagger route = req.swagger.apiPath; @@ -113,15 +113,12 @@ function _getRoute(req) { if (route === '') { route = req.originalUrl; } else { - // In case you have base route and error handler in the root - let routeRegex = route; - routeRegex = routeRegex.replace(/\/:.+/, '/.+'); - routeRegex = routeRegex.replace(/\/:.+\//, '/.+/'); - const regex = new RegExp(routeRegex); - const regexMatch = regex.exec(req.originalUrl); - if (regexMatch && regexMatch.index > 0) { - route = req.originalUrl.substring(0, regexMatch.index) + route; - } + const splittedRoute = route.split('/'); + const splittedUrl = req.originalUrl.split('/'); + const routeIndex = splittedUrl.length - splittedRoute.length + 1; + + const baseUrl = splittedUrl.slice(0, routeIndex).join('/'); + route = baseUrl + route; } } diff --git a/test/integration-tests/express/middleware-test.js b/test/integration-tests/express/middleware-test.js index fb54328..44c9db6 100644 --- a/test/integration-tests/express/middleware-test.js +++ b/test/integration-tests/express/middleware-test.js @@ -160,6 +160,22 @@ describe('when using express framework', () => { }); }); }); + describe('when calling endpoint and getting an error with sub router only variables', () => { + before(() => { + return supertest(app) + .patch('/v2/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="PATCH",route="/v2/:time",code="500"'); + }); + }); + }); describe('when calling endpoint and getting an error with sub router with 1 variable', () => { before(() => { return supertest(app) diff --git a/test/integration-tests/express/server/router.js b/test/integration-tests/express/server/router.js index 8dcdea5..41c5abd 100644 --- a/test/integration-tests/express/server/router.js +++ b/test/integration-tests/express/server/router.js @@ -14,7 +14,7 @@ router.route('/bad').get(bad); router.route('/bad/:time').get(bad); router.route('/bad/:var1/:var2').get(bad); router.route('/test').post(test); -router.route('/:time').patch(helloTime); +router.route('/:time').patch(bad); router.route('/hello/:time').get(helloTime); function test (req, res, next) { From 94945a1e2a6fc9957a8d5b4aa2154afc1d865ba5 Mon Sep 17 00:00:00 2001 From: idantovi Date: Mon, 3 Dec 2018 12:50:31 +0200 Subject: [PATCH 4/4] add more tests --- .../express/middleware-test.js | 520 ++++++++++++++---- .../express/server/router.js | 12 +- .../express/server/sub-router.js | 46 ++ 3 files changed, 461 insertions(+), 117 deletions(-) create mode 100644 test/integration-tests/express/server/sub-router.js diff --git a/test/integration-tests/express/middleware-test.js b/test/integration-tests/express/middleware-test.js index 44c9db6..fb31bd1 100644 --- a/test/integration-tests/express/middleware-test.js +++ b/test/integration-tests/express/middleware-test.js @@ -124,120 +124,425 @@ describe('when using express framework', () => { }); }); }); - describe('when calling a GET endpoint with path params and sub router', () => { - before(() => { - return supertest(app) - .get('/v2/hello/200') - .expect(200) - .then((res) => {}); + describe('sub app', function () { + describe('when calling a GET endpoint with path params', () => { + before(() => { + return supertest(app) + .get('/v2/hello/200') + .expect(200) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('http_request_duration_seconds_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_response_size_bytes_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_request_size_bytes_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('http_request_duration_seconds_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); - expect(res.text).to.contain('http_response_size_bytes_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); - expect(res.text).to.contain('http_request_size_bytes_bucket{le="+Inf",method="GET",route="/v2/hello/:time",code="200"} 1'); - }); + describe('when calling a POST endpoint', () => { + before(() => { + return supertest(app) + .post('/v2/test') + .send({name: 'john'}) + .set('Accept', 'application/json') + .expect(201) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="POST",route="/v2/test",code="201"'); + }); + }); }); - }); - describe('when calling a POST endpoint with sub router', () => { - before(() => { - return supertest(app) - .post('/v2/test') - .send({name: 'john'}) - .set('Accept', 'application/json') - .expect(201) - .then((res) => {}); + describe('when calling endpoint and getting an error only variables', () => { + before(() => { + return supertest(app) + .patch('/v2/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="PATCH",route="/v2/:time",code="500"'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="POST",route="/v2/test",code="201"'); - }); + describe('when calling endpoint and getting an error with 1 variable', () => { + before(() => { + return supertest(app) + .get('/v2/bad/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/bad/:time",code="500"'); + }); + }); }); - }); - describe('when calling endpoint and getting an error with sub router only variables', () => { - before(() => { - return supertest(app) - .patch('/v2/500') - .expect(500) - .then((res) => {}); + describe('when calling endpoint and getting an error with two variables', () => { + before(() => { + return supertest(app) + .get('/v2/bad/500/400') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/bad/:var1/:var2",code="500"'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="PATCH",route="/v2/:time",code="500"'); - }); + describe('when calling endpoint and getting an error with no variables', () => { + before(() => { + return supertest(app) + .get('/v2/bad') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/bad",code="500"'); + }); + }); }); - }); - describe('when calling endpoint and getting an error with sub router with 1 variable', () => { - before(() => { - return supertest(app) - .get('/v2/bad/500') - .expect(500) - .then((res) => {}); + describe('when calling endpoint and getting an error (root)', () => { + before(() => { + return supertest(app) + .get('/v2') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2",code="500"'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="GET",route="/v2/bad/:time",code="500"'); - }); + describe('when calling endpoint and getting an error (error handler in the sub app)', () => { + before(() => { + return supertest(app) + .get('/v2/error/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/error/:var1",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error from a middleware before sub route', () => { + before(() => { + return supertest(app) + .get('/v2/hello') + .set('error', 'error') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2",code="500"'); + }); + }); }); }); - describe('when calling endpoint and getting an error with sub router with two variables', () => { - before(() => { - return supertest(app) - .get('/v2/bad/500/400') - .expect(500) - .then((res) => {}); + describe('sub-sub app with error handler in the sub app', function () { + describe('when calling a GET endpoint with path params and sub router', () => { + before(() => { + return supertest(app) + .get('/v2/v3/hello/200') + .expect(200) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('http_request_duration_seconds_bucket{le="+Inf",method="GET",route="/v2/v3/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_response_size_bytes_bucket{le="+Inf",method="GET",route="/v2/v3/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_request_size_bytes_bucket{le="+Inf",method="GET",route="/v2/v3/hello/:time",code="200"} 1'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="GET",route="/v2/bad/:var1/:var2",code="500"'); - }); + describe('when calling a POST endpoint with sub router', () => { + before(() => { + return supertest(app) + .post('/v2/v3/test') + .send({name: 'john'}) + .set('Accept', 'application/json') + .expect(201) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="POST",route="/v2/v3/test",code="201"'); + }); + }); }); - }); - describe('when calling endpoint and getting an error with sub router with no variables', () => { - before(() => { - return supertest(app) - .get('/v2/bad') - .expect(500) - .then((res) => {}); + describe('when calling endpoint and getting an error with sub router only variables', () => { + before(() => { + return supertest(app) + .patch('/v2/v3/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="PATCH",route="/v2/v3/:time",code="500"'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="GET",route="/v2/bad",code="500"'); - }); + describe('when calling endpoint and getting an error with sub router with 1 variable', () => { + before(() => { + return supertest(app) + .get('/v2/v3/bad/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v3/bad/:time",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router with two variables', () => { + before(() => { + return supertest(app) + .get('/v2/v3/bad/500/400') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v3/bad/:var1/:var2",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router with no variables', () => { + before(() => { + return supertest(app) + .get('/v2/v3/bad') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v3/bad",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router (root)', () => { + before(() => { + return supertest(app) + .get('/v2/v3') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v3",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error from a middleware before sub route', () => { + before(() => { + return supertest(app) + .get('/v2/v3/hello') + .set('error', 'error') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v3",code="500"'); + }); + }); }); }); - describe('when calling endpoint and getting an error with sub router (root)', () => { - before(() => { - return supertest(app) - .get('/v2') - .expect(500) - .then((res) => {}); + describe('sub-sub app with error handler in the sub-sub app', function () { + describe('when calling a GET endpoint with path params and sub router', () => { + before(() => { + return supertest(app) + .get('/v2/v4/hello/200') + .expect(200) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('http_request_duration_seconds_bucket{le="+Inf",method="GET",route="/v2/v4/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_response_size_bytes_bucket{le="+Inf",method="GET",route="/v2/v4/hello/:time",code="200"} 1'); + expect(res.text).to.contain('http_request_size_bytes_bucket{le="+Inf",method="GET",route="/v2/v4/hello/:time",code="200"} 1'); + }); + }); }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="GET",route="/v2",code="500"'); - }); + describe('when calling a POST endpoint with sub router', () => { + before(() => { + return supertest(app) + .post('/v2/v4/test') + .send({name: 'john'}) + .set('Accept', 'application/json') + .expect(201) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="POST",route="/v2/v4/test",code="201"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router only variables', () => { + before(() => { + return supertest(app) + .patch('/v2/v4/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="PATCH",route="/v2/v4/:time",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router with 1 variable', () => { + before(() => { + return supertest(app) + .get('/v2/v4/bad/500') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v4/bad/:time",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router with two variables', () => { + before(() => { + return supertest(app) + .get('/v2/v4/bad/500/400') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v4/bad/:var1/:var2",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router with no variables', () => { + before(() => { + return supertest(app) + .get('/v2/v4/bad') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v4/bad",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error with sub router (root)', () => { + before(() => { + return supertest(app) + .get('/v2/v4') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v4",code="500"'); + }); + }); + }); + describe('when calling endpoint and getting an error from a middleware before sub route', () => { + before(() => { + return supertest(app) + .get('/v2/v4/hello') + .set('error', 'error') + .expect(500) + .then((res) => {}); + }); + it('should add it to the histogram', () => { + return supertest(app) + .get('/metrics') + .expect(200) + .then((res) => { + expect(res.text).to.contain('method="GET",route="/v2/v4",code="500"'); + }); + }); }); }); describe('when calling endpoint and getting an error from a middleware before route', () => { @@ -257,23 +562,6 @@ describe('when using express framework', () => { }); }); }); - describe('when calling endpoint and getting an error from a middleware before sub route', () => { - before(() => { - return supertest(app) - .get('/v2/hello') - .set('error', 'error') - .expect(500) - .then((res) => {}); - }); - it('should add it to the histogram', () => { - return supertest(app) - .get('/metrics') - .expect(200) - .then((res) => { - expect(res.text).to.contain('method="GET",route="/v2",code="500"'); - }); - }); - }); describe('when using custom metrics', () => { before(() => { return supertest(app) diff --git a/test/integration-tests/express/server/router.js b/test/integration-tests/express/server/router.js index 41c5abd..b549772 100644 --- a/test/integration-tests/express/server/router.js +++ b/test/integration-tests/express/server/router.js @@ -1,7 +1,7 @@ 'use strict'; var express = require('express'); var router = express.Router(); - +const subRouter = require('./sub-router'); router.use((req, res, next) => { if (req.headers.error) { next(new Error('Error')); @@ -9,6 +9,8 @@ router.use((req, res, next) => { next(); }); +router.use('/v3', subRouter, errorHandler); +router.use('/v4', subRouter); router.route('/').get(bad); router.route('/bad').get(bad); router.route('/bad/:time').get(bad); @@ -16,6 +18,7 @@ router.route('/bad/:var1/:var2').get(bad); router.route('/test').post(test); router.route('/:time').patch(bad); router.route('/hello/:time').get(helloTime); +router.route('/error/:var1').get(bad, errorHandler); function test (req, res, next) { setTimeout(() => { @@ -36,4 +39,11 @@ function bad (req, res, next) { next(new Error('My Error')); }; +// Error handler +function errorHandler(err, req, res, next) { + res.statusCode = 500; + // Do not expose your error in production + res.json({ error: err.message }); +} + module.exports = router; \ No newline at end of file diff --git a/test/integration-tests/express/server/sub-router.js b/test/integration-tests/express/server/sub-router.js new file mode 100644 index 0000000..ee2a3dd --- /dev/null +++ b/test/integration-tests/express/server/sub-router.js @@ -0,0 +1,46 @@ +'use strict'; +var express = require('express'); +var router = express.Router(); + +router.use((req, res, next) => { + if (req.headers.error) { + next(new Error('Error')); + } + next(); +}); + +router.route('/').get(bad); +router.route('/bad').get(bad); +router.route('/bad/:time').get(bad); +router.route('/bad/:var1/:var2').get(bad); +router.route('/test').post(test); +router.route('/:time').patch(bad); +router.route('/hello/:time').get(helloTime); + +function test (req, res, next) { + setTimeout(() => { + res.status(201); + res.json({ message: 'Hello World!' }); + next(); + }, req.body.delay); +}; + +function helloTime (req, res, next) { + setTimeout(() => { + res.json({ message: 'Hello World!' }); + next(); + }, parseInt(req.param.time)); +}; + +function bad (req, res, next) { + next(new Error('My Error')); +}; + +// Error handler +function errorHandler(err, req, res, next) { + res.statusCode = 500; + // Do not expose your error in production + res.json({ error: err.message }); +} + +module.exports = router; \ No newline at end of file