From 022573a07259878b94b73ca188a7e884ff7b51ce Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 1 Nov 2018 22:07:58 -0700 Subject: [PATCH 1/6] Move jsonWithDefault to a util --- ui/app/services/system.js | 6 +----- ui/app/utils/json-with-default.js | 10 ++++++++++ 2 files changed, 11 insertions(+), 5 deletions(-) create mode 100644 ui/app/utils/json-with-default.js diff --git a/ui/app/services/system.js b/ui/app/services/system.js index 7d6d21792435..b8a5a1a91ac6 100644 --- a/ui/app/services/system.js +++ b/ui/app/services/system.js @@ -1,13 +1,9 @@ import Service, { inject as service } from '@ember/service'; import { computed } from '@ember/object'; -import { copy } from '@ember/object/internals'; import PromiseObject from '../utils/classes/promise-object'; import PromiseArray from '../utils/classes/promise-array'; import { namespace } from '../adapters/application'; - -// When the request isn't ok (e.g., forbidden) handle gracefully -const jsonWithDefault = defaultResponse => res => - res.ok ? res.json() : copy(defaultResponse, true); +import jsonWithDefault from '../utils/json-with-default'; export default Service.extend({ token: service(), diff --git a/ui/app/utils/json-with-default.js b/ui/app/utils/json-with-default.js new file mode 100644 index 000000000000..d71fa93ba9b5 --- /dev/null +++ b/ui/app/utils/json-with-default.js @@ -0,0 +1,10 @@ +import { copy } from '@ember/object/internals'; + +// Used with fetch. +// Fetch only goes into the promise catch if there is a network error. +// This means that handling a 4xx or 5xx error is the responsibility +// of the developer. +const jsonWithDefault = defaultResponse => res => + res.ok ? res.json() : copy(defaultResponse, true); + +export default jsonWithDefault; From 9f98029898970985f41144272643287ab51f87b0 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 1 Nov 2018 22:08:57 -0700 Subject: [PATCH 2/6] Gracefully handle response errors in stat trackers 1. Check if the response is a 4xx/5xx 2. If it is, skip the append step and track a frame miss 3. If enough frame misses occur in a row, treat it as a pause A "pause" is when a null data frame is added, which shows up as a gap in line charts. --- .../utils/classes/abstract-stats-tracker.js | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/ui/app/utils/classes/abstract-stats-tracker.js b/ui/app/utils/classes/abstract-stats-tracker.js index 08a8f03f5568..58e784685f93 100644 --- a/ui/app/utils/classes/abstract-stats-tracker.js +++ b/ui/app/utils/classes/abstract-stats-tracker.js @@ -1,12 +1,20 @@ import Mixin from '@ember/object/mixin'; import { assert } from '@ember/debug'; import { task, timeout } from 'ember-concurrency'; +import jsonWithDefault from 'nomad-ui/utils/json-with-default'; export default Mixin.create({ url: '', + // The max number of data points tracked. Once the max is reached, + // data points at the head of the list are removed in favor of new + // data appended at the tail bufferSize: 500, + // The number of consecutive request failures that can occur before an + // empty frame is appended + maxFrameMisses: 5, + fetch() { assert('StatsTrackers need a fetch method, which should have an interface like window.fetch'); }, @@ -23,6 +31,25 @@ export default Mixin.create({ ); }, + frameMisses: 0, + + handleResponse(frame) { + if (frame.error) { + this.incrementProperty('frameMisses'); + if (this.get('frameMisses') >= this.get('maxFrameMisses')) { + // Missing enough data consecutively is effectively a pause + this.pause(); + this.set('frameMisses', 0); + } + return; + } else { + this.set('frameMisses', 0); + + // Only append non-error frames + this.append(frame); + } + }, + // Uses EC as a form of debounce to prevent multiple // references to the same tracker from flooding the tracker, // but also avoiding the issue where different places where the @@ -36,8 +63,8 @@ export default Mixin.create({ assert('Url must be defined', url); yield this.get('fetch')(url) - .then(res => res.json()) - .then(frame => this.append(frame)); + .then(jsonWithDefault({ error: true })) + .then(frame => this.handleResponse(frame)); } catch (error) { throw new Error(error); } From b0a1a360ba598b60e12ff7458e9cf593fe265eb7 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 1 Nov 2018 22:11:56 -0700 Subject: [PATCH 3/6] Improve client stat simulation and add a chance for requests to error --- ui/mirage/config.js | 12 +++++++++++- ui/mirage/factories/client-stats.js | 4 ++-- ui/mirage/factories/node.js | 4 +++- 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/ui/mirage/config.js b/ui/mirage/config.js index d0b834a42949..c98588b5ce6c 100644 --- a/ui/mirage/config.js +++ b/ui/mirage/config.js @@ -307,7 +307,17 @@ export default function() { this.get('/client/fs/logs/:allocation_id', clientAllocationLog); this.get('/client/stats', function({ clientStats }, { queryParams }) { - return this.serialize(clientStats.find(queryParams.node_id)); + const seed = Math.random(); + if (seed > 0.8) { + const stats = clientStats.find(queryParams.node_id); + stats.update({ + timestamp: Date.now() * 1000000, + CPUTicksConsumed: stats.CPUTicksConsumed + (Math.random() * 20 - 10), + }); + return this.serialize(stats); + } else { + return new Response(500, {}, null); + } }); // TODO: in the future, this hack may be replaceable with dynamic host name diff --git a/ui/mirage/factories/client-stats.js b/ui/mirage/factories/client-stats.js index 6924f07f6418..8d7b27edd1ba 100644 --- a/ui/mirage/factories/client-stats.js +++ b/ui/mirage/factories/client-stats.js @@ -23,7 +23,7 @@ export default Factory.extend({ })), ], - CPUTicksConsumed: 1000000, + CPUTicksConsumed: faker.random.number({ min: 100, max: 1000 }), diskStats: () => [ Array(faker.random.number({ min: 1, max: 5 })) @@ -46,6 +46,6 @@ export default Factory.extend({ Used: 10000000000, }), - timestamp: 149000000000, + timestamp: Date.now() * 1000000, uptime: 193838, }); diff --git a/ui/mirage/factories/node.js b/ui/mirage/factories/node.js index e8ec62a9d910..a7c66cd3ad21 100644 --- a/ui/mirage/factories/node.js +++ b/ui/mirage/factories/node.js @@ -1,6 +1,6 @@ import { Factory, faker, trait } from 'ember-cli-mirage'; import { provide } from '../utils'; -import { DATACENTERS, HOSTS } from '../common'; +import { DATACENTERS, HOSTS, generateResources } from '../common'; import moment from 'moment'; const UUIDS = provide(100, faker.random.uuid.bind(faker.random)); @@ -65,6 +65,8 @@ export default Factory.extend({ drivers: makeDrivers, + resources: generateResources, + attributes() { // TODO add variability to these return { From 333e76ba73d10a1734fb3743d8e9182912f6515f Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 1 Nov 2018 22:12:44 -0700 Subject: [PATCH 4/6] Test coverage for frame misses --- .../utils/allocation-stats-tracker-test.js | 25 +++++ .../behaviors/stats-tracker-frame-missing.js | 97 +++++++++++++++++++ .../unit/utils/node-stats-tracker-test.js | 23 +++++ 3 files changed, 145 insertions(+) create mode 100644 ui/tests/unit/utils/behaviors/stats-tracker-frame-missing.js diff --git a/ui/tests/unit/utils/allocation-stats-tracker-test.js b/ui/tests/unit/utils/allocation-stats-tracker-test.js index cbfa181fa4f9..5af885a8054c 100644 --- a/ui/tests/unit/utils/allocation-stats-tracker-test.js +++ b/ui/tests/unit/utils/allocation-stats-tracker-test.js @@ -6,6 +6,7 @@ import sinon from 'sinon'; import Pretender from 'pretender'; import AllocationStatsTracker, { stats } from 'nomad-ui/utils/classes/allocation-stats-tracker'; import fetch from 'nomad-ui/utils/fetch'; +import statsTrackerFrameMissingBehavior from './behaviors/stats-tracker-frame-missing'; module('Unit | Util | AllocationStatsTracker'); @@ -453,3 +454,27 @@ test('changing the value of the allocationProp constructs a new AllocationStatsT 'Changing the value of alloc results in creating a new AllocationStatsTracker instance' ); }); + +statsTrackerFrameMissingBehavior({ + resourceName: 'allocation', + ResourceConstructor: MockAllocation, + TrackerConstructor: AllocationStatsTracker, + mockFrame, + compileResources(frame) { + const timestamp = makeDate(frame.Timestamp); + const cpu = frame.ResourceUsage.CpuStats.TotalTicks; + const memory = frame.ResourceUsage.MemoryStats.RSS; + return [ + { + timestamp, + used: cpu, + percent: cpu / 200, + }, + { + timestamp, + used: memory, + percent: memory / 1024 / 1024 / 512, + }, + ]; + }, +}); diff --git a/ui/tests/unit/utils/behaviors/stats-tracker-frame-missing.js b/ui/tests/unit/utils/behaviors/stats-tracker-frame-missing.js new file mode 100644 index 000000000000..679143848c1f --- /dev/null +++ b/ui/tests/unit/utils/behaviors/stats-tracker-frame-missing.js @@ -0,0 +1,97 @@ +import { resolve } from 'rsvp'; +import wait from 'ember-test-helpers/wait'; +import { test } from 'ember-qunit'; +import sinon from 'sinon'; + +const MockResponse = json => ({ + ok: true, + json() { + return resolve(json); + }, +}); + +export default function statsTrackerFrameMissing({ + resourceName, + TrackerConstructor, + ResourceConstructor, + mockFrame, + compileResources, +}) { + test('a bad response from a fetch request is handled gracefully', function(assert) { + const frame = mockFrame(1); + const [compiledCPU, compiledMemory] = compileResources(frame); + + let shouldFail = false; + const fetch = () => { + return resolve(shouldFail ? { ok: false } : new MockResponse(frame)); + }; + + const resource = ResourceConstructor(); + const tracker = TrackerConstructor.create({ fetch, [resourceName]: resource }); + + tracker.get('poll').perform(); + + return wait() + .then(() => { + assert.deepEqual(tracker.get('cpu'), [compiledCPU], 'One frame of cpu'); + assert.deepEqual(tracker.get('memory'), [compiledMemory], 'One frame of memory'); + + shouldFail = true; + tracker.get('poll').perform(); + return wait(); + }) + .then(() => { + assert.deepEqual(tracker.get('cpu'), [compiledCPU], 'Still one frame of cpu'); + assert.deepEqual(tracker.get('memory'), [compiledMemory], 'Still one frame of memory'); + assert.equal(tracker.get('frameMisses'), 1, 'Frame miss is tracked'); + + shouldFail = false; + tracker.get('poll').perform(); + return wait(); + }) + .then(() => { + assert.deepEqual(tracker.get('cpu'), [compiledCPU, compiledCPU], 'Still one frame of cpu'); + assert.deepEqual( + tracker.get('memory'), + [compiledMemory, compiledMemory], + 'Still one frame of memory' + ); + assert.equal(tracker.get('frameMisses'), 0, 'Frame misses is reset'); + }); + }); + + test('enough bad responses from fetch consecutively (as set by maxFrameMisses) results in a pause', function(assert) { + const fetch = () => { + return resolve({ ok: false }); + }; + + const resource = ResourceConstructor(); + const tracker = TrackerConstructor.create({ + fetch, + [resourceName]: resource, + maxFrameMisses: 3, + pause: sinon.spy(), + }); + + tracker.get('poll').perform(); + return wait() + .then(() => { + assert.equal(tracker.get('frameMisses'), 1, 'Tick misses'); + assert.notOk(tracker.pause.called, 'Pause not called yet'); + + tracker.get('poll').perform(); + return wait(); + }) + .then(() => { + assert.equal(tracker.get('frameMisses'), 2, 'Tick misses'); + assert.notOk(tracker.pause.called, 'Pause still not called yet'); + + tracker.get('poll').perform(); + return wait(); + }) + .then(() => { + assert.equal(tracker.get('frameMisses'), 0, 'Misses reset'); + assert.ok(tracker.pause.called, 'Pause called now that frameMisses == maxFrameMisses'); + }); + }); +} diff --git a/ui/tests/unit/utils/node-stats-tracker-test.js b/ui/tests/unit/utils/node-stats-tracker-test.js index 5dfbb19ff411..cfcf1426dda0 100644 --- a/ui/tests/unit/utils/node-stats-tracker-test.js +++ b/ui/tests/unit/utils/node-stats-tracker-test.js @@ -6,6 +6,7 @@ import sinon from 'sinon'; import Pretender from 'pretender'; import NodeStatsTracker, { stats } from 'nomad-ui/utils/classes/node-stats-tracker'; import fetch from 'nomad-ui/utils/fetch'; +import statsTrackerFrameMissingBehavior from './behaviors/stats-tracker-frame-missing'; module('Unit | Util | NodeStatsTracker'); @@ -221,3 +222,25 @@ test('changing the value of the nodeProp constructs a new NodeStatsTracker', fun 'Changing the value of the node results in creating a new NodeStatsTracker instance' ); }); + +statsTrackerFrameMissingBehavior({ + resourceName: 'node', + ResourceConstructor: MockNode, + TrackerConstructor: NodeStatsTracker, + mockFrame, + compileResources(frame) { + const timestamp = makeDate(frame.Timestamp); + return [ + { + timestamp, + used: frame.CPUTicksConsumed, + percent: frame.CPUTicksConsumed / 2000, + }, + { + timestamp, + used: frame.Memory.Used, + percent: frame.Memory.Used / 1024 / 1024 / 4096, + }, + ]; + }, +}); From c95821e84e06612523d266e4c15b33ca043ca8fb Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Thu, 1 Nov 2018 22:12:57 -0700 Subject: [PATCH 5/6] =?UTF-8?q?Make=20your=20tests=20orders=20of=20magnitu?= =?UTF-8?q?de=20faster=20with=20One=20Neat=20Trick=E2=84=A2?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ui/app/utils/classes/abstract-stats-tracker.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ui/app/utils/classes/abstract-stats-tracker.js b/ui/app/utils/classes/abstract-stats-tracker.js index 58e784685f93..68f2b5e56fdb 100644 --- a/ui/app/utils/classes/abstract-stats-tracker.js +++ b/ui/app/utils/classes/abstract-stats-tracker.js @@ -1,3 +1,4 @@ +import Ember from 'ember'; import Mixin from '@ember/object/mixin'; import { assert } from '@ember/debug'; import { task, timeout } from 'ember-concurrency'; @@ -69,12 +70,12 @@ export default Mixin.create({ throw new Error(error); } - yield timeout(2000); + yield timeout(Ember.testing ? 0 : 2000); }).drop(), signalPause: task(function*() { // wait 2 seconds - yield timeout(2000); + yield timeout(Ember.testing ? 0 : 2000); // if no poll called in 2 seconds, pause this.pause(); }).drop(), From b5348aacf08140f623c3299e6488641bf93fe650 Mon Sep 17 00:00:00 2001 From: Michael Lange Date: Fri, 9 Nov 2018 14:25:32 -0800 Subject: [PATCH 6/6] Add missing module dependencies to the node adapter tests --- ui/tests/unit/adapters/node-test.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ui/tests/unit/adapters/node-test.js b/ui/tests/unit/adapters/node-test.js index 67e0e8c30d3c..214864c027eb 100644 --- a/ui/tests/unit/adapters/node-test.js +++ b/ui/tests/unit/adapters/node-test.js @@ -12,6 +12,8 @@ moduleForAdapter('node', 'Unit | Adapter | Node', { 'model:node-driver', 'model:node-event', 'model:evaluation', + 'model:resources', + 'model:network', 'model:job', 'serializer:application', 'serializer:node', @@ -21,6 +23,7 @@ moduleForAdapter('node', 'Unit | Adapter | Node', { 'service:watchList', 'transform:fragment', 'transform:fragment-array', + 'transform:array', ], beforeEach() { this.server = startMirage();