From 84e0810d64cb653940bd2f01a3b55f6a3924d14e Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Thu, 1 Dec 2016 17:15:55 +0100 Subject: [PATCH] Some Metrics API cleanups --- docs/api/metrics-api.md | 2 +- lib/routes/metrics.js | 26 +++--- lib/routes/route-utils.js | 8 ++ .../fixtures/fake-client-instance-store.js | 1 + .../fixtures/fake-client-strategy-store.js | 1 + .../routes/fixtures/fake-metrics-store.js | 17 +++- test/unit/routes/fixtures/store.js | 4 +- test/unit/routes/metrics.test.js | 93 +++++++++++++++++++ 8 files changed, 129 insertions(+), 23 deletions(-) create mode 100644 lib/routes/route-utils.js diff --git a/docs/api/metrics-api.md b/docs/api/metrics-api.md index fce484bbc9..e15e4b3dd6 100644 --- a/docs/api/metrics-api.md +++ b/docs/api/metrics-api.md @@ -96,7 +96,7 @@ will in most cases remember seen-toggles for applications longer ### Feature-Toggles metrics -`GET http://unleash.host.com/api/metrics/feature-toggles` +`GET http://unleash.host.com/api/client/metrics/feature-toggles` This endpoint gives _last minute_ and _last hour_ metrics for all active toggles. This is based on metrics reported by client applications. Yes is the number of times a given feature toggle diff --git a/lib/routes/metrics.js b/lib/routes/metrics.js index 36d393ee2b..1529f50458 100644 --- a/lib/routes/metrics.js +++ b/lib/routes/metrics.js @@ -5,13 +5,8 @@ const ClientMetrics = require('../client-metrics'); const joi = require('joi'); const { clientMetricsSchema, clientRegisterSchema } = require('./metrics-schema'); const { CLIENT_REGISTER, CLIENT_METRICS } = require('../events'); -/* -* TODO: -* - always catch errors and always return a response to client! -* - clean up and document uri endpoint -* - always json response (middleware?) -* - fix failing tests -*/ +const { catchLogAndSendErrorResponse } = require('./route-utils'); + module.exports = function (app, config) { const { clientMetricsStore, @@ -28,7 +23,7 @@ module.exports = function (app, config) { res.json(seenAppToggles); }); - app.get('/metrics/feature-toggles', (req, res) => { + app.get('/client/metrics/feature-toggles', (req, res) => { res.json(metrics.getTogglesMetrics()); }); @@ -50,7 +45,7 @@ module.exports = function (app, config) { instanceId: cleaned.instanceId, clientIp, })) - .catch(e => logger.error('Error inserting metrics data', e)); + .catch(err => catchLogAndSendErrorResponse(err, res)); res.status(202).end(); }); @@ -74,8 +69,9 @@ module.exports = function (app, config) { instanceId: cleaned.instanceId, clientIp, })) - .then(() => logger.info('New client registered!')) - .catch((error) => logger.error('Error registering client', error)); + .then(() => logger.info(`New client registered with + appName=${cleaned.appName} and instanceId=${cleaned.instanceId}`)) + .catch(err => catchLogAndSendErrorResponse(err, res)); res.status(202).end(); }); @@ -86,11 +82,11 @@ module.exports = function (app, config) { if(appName) { clientStrategyStore.getByAppName(appName) .then(data => res.json(data)) - .catch(err => logger.error(err)); + .catch(err => catchLogAndSendErrorResponse(err, res)); } else { clientStrategyStore.getAll() .then(data => res.json(data)) - .catch(err => logger.error(err)); + .catch(err => catchLogAndSendErrorResponse(err, res)); } }); @@ -105,7 +101,7 @@ module.exports = function (app, config) { })) res.json({applications}) }) - .catch(err => logger.error(err)); + .catch(err => catchLogAndSendErrorResponse(err, res)); }); app.get('/client/applications/:appName', (req, res) => { @@ -116,6 +112,6 @@ module.exports = function (app, config) { clientStrategyStore.getByAppName(appName) ]) .then(([instances, strategies]) => res.json({appName, instances, strategies, seenToggles})) - .catch(err => logger.error(err)); + .catch(err => catchLogAndSendErrorResponse(err, res)); }); }; diff --git a/lib/routes/route-utils.js b/lib/routes/route-utils.js new file mode 100644 index 0000000000..8ab3aa05b5 --- /dev/null +++ b/lib/routes/route-utils.js @@ -0,0 +1,8 @@ +const logger = require('../logger'); + +const catchLogAndSendErrorResponse = (err, res) => { + logger.error(err); + res.status(500).end(); +} + +module.exports = { catchLogAndSendErrorResponse }; \ No newline at end of file diff --git a/test/unit/routes/fixtures/fake-client-instance-store.js b/test/unit/routes/fixtures/fake-client-instance-store.js index 1b2ea2a50a..b8d48b78e6 100644 --- a/test/unit/routes/fixtures/fake-client-instance-store.js +++ b/test/unit/routes/fixtures/fake-client-instance-store.js @@ -2,4 +2,5 @@ module.exports = () => ({ insert: () => Promise.resolve(), + getApplications: () => Promise.resolve([]), }); diff --git a/test/unit/routes/fixtures/fake-client-strategy-store.js b/test/unit/routes/fixtures/fake-client-strategy-store.js index 1b2ea2a50a..2666aed691 100644 --- a/test/unit/routes/fixtures/fake-client-strategy-store.js +++ b/test/unit/routes/fixtures/fake-client-strategy-store.js @@ -2,4 +2,5 @@ module.exports = () => ({ insert: () => Promise.resolve(), + getAll: () => Promise.resolve([]) }); diff --git a/test/unit/routes/fixtures/fake-metrics-store.js b/test/unit/routes/fixtures/fake-metrics-store.js index 2f2e35fb50..c19033ca5b 100644 --- a/test/unit/routes/fixtures/fake-metrics-store.js +++ b/test/unit/routes/fixtures/fake-metrics-store.js @@ -1,7 +1,14 @@ 'use strict'; -module.exports = () => ({ - getMetricsLastHour: () => Promise.resolve([]), - insert: () => Promise.resolve(), - on: () => {} -}); +const { EventEmitter } = require('events'); + +class FakeMetricsStore extends EventEmitter { + getMetricsLastHour () { + return Promise.resolve([]); + } + insert () { + return Promise.resolve(); + } +} + +module.exports = FakeMetricsStore; \ No newline at end of file diff --git a/test/unit/routes/fixtures/store.js b/test/unit/routes/fixtures/store.js index 3b2ee9563f..3e24840288 100644 --- a/test/unit/routes/fixtures/store.js +++ b/test/unit/routes/fixtures/store.js @@ -1,6 +1,6 @@ 'use strict'; -const clientMetricsStore = require('./fake-metrics-store'); +const ClientMetricsStore = require('./fake-metrics-store'); const clientStrategyStore = require('./fake-client-strategy-store'); const clientInstanceStore = require('./fake-client-instance-store'); const featureToggleStore = require('./fake-feature-toggle-store'); @@ -19,7 +19,7 @@ module.exports = { return { db, - clientMetricsStore: clientMetricsStore(), + clientMetricsStore: new ClientMetricsStore(), clientStrategyStore: clientStrategyStore(), clientInstanceStore: clientInstanceStore(), featureToggleStore: featureToggleStore(), diff --git a/test/unit/routes/metrics.test.js b/test/unit/routes/metrics.test.js index 610f47594b..07bd55d436 100644 --- a/test/unit/routes/metrics.test.js +++ b/test/unit/routes/metrics.test.js @@ -23,6 +23,7 @@ function getSetup () { return { request: supertest(app), + stores }; } @@ -77,3 +78,95 @@ test('should accept client metrics', () => { }) .expect(202); }); + +test('should return seen toggles even when there is nothing', t => { + const { request } = getSetup(); + return request + .get('/api/client/seen-toggles') + .expect(200) + .expect((res) => { + t.true(res.body.length === 0); + }); +}); + +test('should return list of seen-toggles per app', t => { + const { request, stores } = getSetup(); + const appName = 'asd!23' + stores.clientMetricsStore.emit('metrics', { + appName, + instanceId: 'instanceId', + bucket: { + start: new Date(), + stop: new Date(), + toggles: { + toggleX: {yes: 123,no: 0}, + toggleY: {yes: 123,no: 0} + }, + }, + }); + + return request + .get('/api/client/seen-toggles') + .expect(200) + .expect((res) => { + const seenAppsWithToggles = res.body; + t.true(seenAppsWithToggles.length === 1); + t.true(seenAppsWithToggles[0].appName === appName); + t.true(seenAppsWithToggles[0].seenToggles.length === 2); + }); +}); + +test('should return feature-toggles metrics even when there is nothing', t => { + const { request } = getSetup(); + return request + .get('/api/client/metrics/feature-toggles') + .expect(200) +}); + +test('should return metrics for all toggles', t => { + const { request, stores } = getSetup(); + const appName = 'asd!23' + stores.clientMetricsStore.emit('metrics', { + appName, + instanceId: 'instanceId', + bucket: { + start: new Date(), + stop: new Date(), + toggles: { + toggleX: {yes: 123,no: 0}, + toggleY: {yes: 123,no: 0} + }, + }, + }); + + return request + .get('/api/client/metrics/feature-toggles') + .expect(200) + .expect((res) => { + + const metrics = res.body; + t.true(metrics.lastHour !== undefined); + t.true(metrics.lastMinute !== undefined); + }); +}); + + +test('should return list of client strategies', t => { + const { request, stores } = getSetup(); + return request + .get('/api/client/strategies') + .expect(200) + .expect((res) => { + t.true(res.body.length === 0); + }); +}); + +test('should return list of client applications', t => { + const { request, stores } = getSetup(); + return request + .get('/api/client/applications') + .expect(200) + .expect((res) => { + t.true(res.body.applications.length === 0); + }); +});