Merge pull request #256 from Unleash/metrics_validation

Add metrics validation to avoid NaN #253

CHANGELOG.md

@@ -5,6 +5,7 @@
 - disable edit of built-in strategies
 - Strip uknown fields in client requests.
 - Disable x-powered-by header
+- Improved client-metrics validation to avoid NaN
 - Add posibility to inject custom logger provider
 
 ## 3.0.0-alpha.1

lib/client-metrics/client-metrics.test.js

@@ -335,3 +335,45 @@ test('should have correct values for lastHour', t => {
     metrics.destroy();
     clock.restore();
 });
+
+test('should not fail when toggle metrics is missing yes/no field', t => {
+    const store = new EventEmitter();
+    const metrics = new UnleashClientMetrics(store);
+    store.emit('metrics', {
+        appName,
+        instanceId,
+        bucket: {
+            start: new Date(),
+            stop: new Date(),
+            toggles: {
+                toggleX: {
+                    yes: 123,
+                    no: 0,
+                },
+            },
+        },
+    });
+
+    metrics.addPayload({
+        appName,
+        instanceId,
+        bucket: {
+            start: new Date(),
+            stop: new Date(),
+            toggles: {
+                toggleX: {
+                    blue: 10,
+                    green: 10,
+                },
+            },
+        },
+    });
+
+    t.is(metrics.globalCount, 123);
+    t.deepEqual(metrics.getTogglesMetrics().lastMinute.toggleX, {
+        yes: 123,
+        no: 0,
+    });
+
+    metrics.destroy();
+});

lib/client-metrics/index.js

@@ -25,7 +25,7 @@ module.exports = class UnleashClientMetrics {
             Object.keys(toggles).forEach(toggleName => {
                 this.lastHourProjection.substract(
                     toggleName,
-                    toggles[toggleName]
+                    this.createCountObject(toggles[toggleName])
                 );
             });
         });
@@ -33,7 +33,7 @@ module.exports = class UnleashClientMetrics {
             Object.keys(toggles).forEach(toggleName => {
                 this.lastMinuteProjection.substract(
                     toggleName,
-                    toggles[toggleName]
+                    this.createCountObject(toggles[toggleName])
                 );
             });
         });
@@ -91,6 +91,12 @@ module.exports = class UnleashClientMetrics {
         return this.apps[appName];
     }
 
+    createCountObject(entry) {
+        const yes = typeof entry.yes == 'number' ? entry.yes : 0;
+        const no = typeof entry.no == 'number' ? entry.no : 0;
+        return { yes, no };
+    }
+
     addBucket(app, bucket) {
         let count = 0;
         // TODO stop should be createdAt
@@ -99,10 +105,10 @@ module.exports = class UnleashClientMetrics {
         const toggleNames = Object.keys(toggles);
 
         toggleNames.forEach(n => {
-            const entry = toggles[n];
-            this.lastHourProjection.add(n, entry);
-            this.lastMinuteProjection.add(n, entry);
-            count += entry.yes + entry.no;
+            const countObj = this.createCountObject(toggles[n]);
+            this.lastHourProjection.add(n, countObj);
+            this.lastMinuteProjection.add(n, countObj);
+            count += countObj.yes + countObj.no;
         });
 
         this.lastHourList.add(toggles, stop);

lib/routes/client-api/metrics-schema.js

@@ -2,13 +2,18 @@
 
 const joi = require('joi');
 
+const countSchema = joi.object().options({ stripUnknown: true }).keys({
+    yes: joi.number().required().min(0).default(0),
+    no: joi.number().required().min(0).default(0),
+});
+
 const clientMetricsSchema = joi.object().options({ stripUnknown: true }).keys({
     appName: joi.string().required(),
     instanceId: joi.string().required(),
     bucket: joi.object().required().keys({
         start: joi.date().required(),
         stop: joi.date().required(),
-        toggles: joi.object(),
+        toggles: joi.object().pattern(/.*/, countSchema),
     }),
 });
 

lib/routes/client-api/metrics.test.js

@@ -31,7 +31,7 @@ test('should validate client metrics', t => {
         .expect(400);
 });
 
-test('should accept client metrics', t => {
+test('should accept empty client metrics', t => {
     t.plan(0);
     const { request } = getSetup();
     return request
@@ -47,3 +47,47 @@ test('should accept client metrics', t => {
         })
         .expect(202);
 });
+
+test('should accept client metrics with yes/no', t => {
+    t.plan(0);
+    const { request } = getSetup();
+    return request
+        .post('/api/client/metrics')
+        .send({
+            appName: 'demo',
+            instanceId: '1',
+            bucket: {
+                start: Date.now(),
+                stop: Date.now(),
+                toggles: {
+                    toggleA: {
+                        yes: 200,
+                        no: 0,
+                    },
+                },
+            },
+        })
+        .expect(202);
+});
+
+test('should not accept client metrics without yes/no', t => {
+    t.plan(0);
+    const { request } = getSetup();
+    return request
+        .post('/api/client/metrics')
+        .send({
+            appName: 'demo',
+            instanceId: '1',
+            bucket: {
+                start: Date.now(),
+                stop: Date.now(),
+                toggles: {
+                    toggleA: {
+                        blue: 200,
+                        green: 0,
+                    },
+                },
+            },
+        })
+        .expect(400);
+});

test/examples/client-register.json

@@ -0,0 +1,8 @@
+{
+    "appName": "appName",
+    "instanceId": "instanceId",
+    "sdkVersion": "test:123",
+    "strategies": ["default", "some-strategy-1"],
+    "started": "2016-11-03T07:16:43.572Z",
+    "interval": 10000
+}