From 10c2493e28aa0a532250b90fd70cd2605fdf5d08 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Fri, 4 Aug 2017 11:24:58 +0200 Subject: [PATCH 1/3] Add metrics validation to avoid NaN #253 --- lib/client-metrics/client-metrics.test.js | 42 +++++++++++++++++++++ lib/client-metrics/index.js | 18 ++++++--- lib/routes/client-api/metrics-schema.js | 7 +++- lib/routes/client-api/metrics.test.js | 46 ++++++++++++++++++++++- test/examples/client-register.json | 8 ++++ 5 files changed, 113 insertions(+), 8 deletions(-) create mode 100644 test/examples/client-register.json diff --git a/lib/client-metrics/client-metrics.test.js b/lib/client-metrics/client-metrics.test.js index d972b11666..36c056657d 100644 --- a/lib/client-metrics/client-metrics.test.js +++ b/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.truthy(metrics.globalCount === 123); + t.deepEqual(metrics.getTogglesMetrics().lastMinute.toggleX, { + yes: 123, + no: 0, + }); + + metrics.destroy(); +}); diff --git a/lib/client-metrics/index.js b/lib/client-metrics/index.js index dfb911cee4..a2e7feacd6 100644 --- a/lib/client-metrics/index.js +++ b/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); diff --git a/lib/routes/client-api/metrics-schema.js b/lib/routes/client-api/metrics-schema.js index 4c25ade26a..96ac7e4c33 100644 --- a/lib/routes/client-api/metrics-schema.js +++ b/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), }), }); diff --git a/lib/routes/client-api/metrics.test.js b/lib/routes/client-api/metrics.test.js index 78578c626c..0f95366667 100644 --- a/lib/routes/client-api/metrics.test.js +++ b/lib/routes/client-api/metrics.test.js @@ -36,7 +36,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 @@ -52,3 +52,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); +}); diff --git a/test/examples/client-register.json b/test/examples/client-register.json new file mode 100644 index 0000000000..d13c06e995 --- /dev/null +++ b/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 +} From 13cf218ccfca7c68d9f829bea8a44bbf2842a278 Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Tue, 8 Aug 2017 16:51:26 +0200 Subject: [PATCH 2/3] Fix assert --- lib/client-metrics/client-metrics.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/client-metrics/client-metrics.test.js b/lib/client-metrics/client-metrics.test.js index 36c056657d..3db556efab 100644 --- a/lib/client-metrics/client-metrics.test.js +++ b/lib/client-metrics/client-metrics.test.js @@ -369,7 +369,7 @@ test('should not fail when toggle metrics is missing yes/no field', t => { }, }); - t.truthy(metrics.globalCount === 123); + t.is(metrics.globalCount, 123); t.deepEqual(metrics.getTogglesMetrics().lastMinute.toggleX, { yes: 123, no: 0, From 997c5da5acd2746c8424efd3392d8f75b9f5842e Mon Sep 17 00:00:00 2001 From: ivaosthu Date: Tue, 8 Aug 2017 16:51:37 +0200 Subject: [PATCH 3/3] Update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a2a3e0887..6e0a7a0a67 100644 --- a/CHANGELOG.md +++ b/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 ## 3.0.0-alpha.1 - upgrade unleash-frontend to 3.0.0-alpha.1