Some Metrics API cleanups

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 

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));
     });
 };

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 };

test/unit/routes/fixtures/fake-client-instance-store.js

@@ -2,4 +2,5 @@
 
 module.exports = () => ({
     insert: () => Promise.resolve(),
+    getApplications: () => Promise.resolve([]),
 });

test/unit/routes/fixtures/fake-client-strategy-store.js

@@ -2,4 +2,5 @@
 
 module.exports = () => ({
     insert: () => Promise.resolve(),
+    getAll: () => Promise.resolve([])
 });

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;

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(),

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);
+        });
+});