From cdfba8f7b15e723faa21d6f22775c95ba500789b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ivar=20Conradi=20=C3=98sthus?= Date: Thu, 17 Dec 2020 19:22:30 +0100 Subject: [PATCH] feat: Adds last-seen dat on toggles When an application updates metrics for a toggle we now stores the timestamp on the toggle when it was last seen used by an application. This will make it much easier to detect toggles not in use anymore. closes #642 --- docs/api/oas/openapi.yaml | 3 ++ docs/database-schema.md | 1 + lib/db/feature-toggle-store.js | 15 ++++++- lib/db/feature-type-store.js | 2 +- lib/routes/admin-api/feature.js | 6 +-- lib/routes/admin-api/metrics.js | 8 +--- lib/routes/admin-api/metrics.test.js | 8 ++-- lib/routes/client-api/metrics.js | 8 +++- lib/routes/client-api/metrics.test.js | 7 ++- lib/server-impl.test.js | 2 +- .../client-metrics/client-metrics.test.js | 44 +++++++++---------- lib/{ => services}/client-metrics/index.js | 4 +- lib/{ => services}/client-metrics/list.js | 0 .../client-metrics/list.test.js | 0 .../client-metrics/projection.js | 0 .../client-metrics/projection.test.js | 0 lib/{ => services}/client-metrics/ttl-list.js | 0 .../client-metrics/ttl-list.test.js | 0 lib/services/index.js | 2 + ...0201216140726-add-last-seen-to-features.js | 10 +++++ package.json | 2 +- test/e2e/helpers/test-helper.js | 27 +++++------- test/fixtures/fake-feature-toggle-store.js | 1 + yarn.lock | 8 ++-- 24 files changed, 95 insertions(+), 63 deletions(-) rename lib/{ => services}/client-metrics/client-metrics.test.js (87%) rename lib/{ => services}/client-metrics/index.js (97%) rename lib/{ => services}/client-metrics/list.js (100%) rename lib/{ => services}/client-metrics/list.test.js (100%) rename lib/{ => services}/client-metrics/projection.js (100%) rename lib/{ => services}/client-metrics/projection.test.js (100%) rename lib/{ => services}/client-metrics/ttl-list.js (100%) rename lib/{ => services}/client-metrics/ttl-list.test.js (100%) create mode 100644 migrations/20201216140726-add-last-seen-to-features.js diff --git a/docs/api/oas/openapi.yaml b/docs/api/oas/openapi.yaml index 33de729b1a..8d152502ca 100644 --- a/docs/api/oas/openapi.yaml +++ b/docs/api/oas/openapi.yaml @@ -1497,6 +1497,9 @@ components: createdAt: type: string minLength: 1 + lastSeenAt: + type: string + minLength: 1 x-tags: - Responses 200-events: diff --git a/docs/database-schema.md b/docs/database-schema.md index f3964847f2..60b6b2bc6a 100644 --- a/docs/database-schema.md +++ b/docs/database-schema.md @@ -45,6 +45,7 @@ Used by db-migrate module to keep track of migrations. | archived | int4 | 10 | 1 | 0 | | | strategies | json | 2147483647 | 1 | (null) | | | type | varchar | 2147483647 | 1 | release | | +| last_seen_at | timestamp | 29 | 1 | (null) | | ## Table: _client_strategies_ diff --git a/lib/db/feature-toggle-store.js b/lib/db/feature-toggle-store.js index ab43e3ee1b..7b681b000f 100644 --- a/lib/db/feature-toggle-store.js +++ b/lib/db/feature-toggle-store.js @@ -22,13 +22,14 @@ const FEATURE_COLUMNS = [ 'strategies', 'variants', 'created_at', + 'last_seen_at', ]; const TABLE = 'features'; class FeatureToggleStore { constructor(db, eventStore, eventBus, getLogger) { this.db = db; - this.getLogger = getLogger('feature-toggle-store.js'); + this.logger = getLogger('feature-toggle-store.js'); this.timer = action => metricsHelper.wrapTimer(eventBus, DB_TIME, { @@ -115,6 +116,17 @@ class FeatureToggleStore { return rows.map(this.rowToFeature); } + async lastSeenToggles(togleNames) { + const now = new Date(); + try { + await this.db(TABLE) + .whereIn('name', togleNames) + .update({ last_seen_at: now }); + } catch (err) { + this.logger.error('Could not update lastSeen, error: ', err); + } + } + rowToFeature(row) { if (!row) { throw new NotFoundError('No feature toggle found'); @@ -129,6 +141,7 @@ class FeatureToggleStore { strategies: row.strategies, variants: row.variants, createdAt: row.created_at, + lastSeenAt: row.last_seen_at, }; } diff --git a/lib/db/feature-type-store.js b/lib/db/feature-type-store.js index 3b6c85e7ac..c7396a17a4 100644 --- a/lib/db/feature-type-store.js +++ b/lib/db/feature-type-store.js @@ -6,7 +6,7 @@ const TABLE = 'feature_types'; class FeatureToggleStore { constructor(db, getLogger) { this.db = db; - this.getLogger = getLogger('feature-type-store.js'); + this.logger = getLogger('feature-type-store.js'); } async getAll() { diff --git a/lib/routes/admin-api/feature.js b/lib/routes/admin-api/feature.js index ad83c18718..78927d9162 100644 --- a/lib/routes/admin-api/feature.js +++ b/lib/routes/admin-api/feature.js @@ -1,5 +1,3 @@ -'use strict'; - const Controller = require('../controller'); const { @@ -159,12 +157,12 @@ class FeatureController extends Controller { const feature = await this.featureToggleStore.getFeature( featureName, ); - feature[field] = value; + const validFeature = await featureShema.validateAsync(feature); await this.eventStore.store({ type: FEATURE_UPDATED, createdBy: userName, - data: feature, + data: validFeature, }); res.json(feature).end(); } catch (error) { diff --git a/lib/routes/admin-api/metrics.js b/lib/routes/admin-api/metrics.js index e0efc1b9ba..15046dabbb 100644 --- a/lib/routes/admin-api/metrics.js +++ b/lib/routes/admin-api/metrics.js @@ -1,23 +1,19 @@ -'use strict'; - const Controller = require('../controller'); -const ClientMetrics = require('../../client-metrics'); const schema = require('./metrics-schema'); const { UPDATE_APPLICATION } = require('../../permissions'); class MetricsController extends Controller { - constructor(config) { + constructor(config, { clientMetricsService }) { super(config); this.logger = config.getLogger('/admin-api/metrics.js'); const { - clientMetricsStore, clientInstanceStore, clientApplicationsStore, strategyStore, featureToggleStore, } = config.stores; - this.metrics = new ClientMetrics(clientMetricsStore); + this.metrics = clientMetricsService; this.clientInstanceStore = clientInstanceStore; this.clientApplicationsStore = clientApplicationsStore; this.strategyStore = strategyStore; diff --git a/lib/routes/admin-api/metrics.test.js b/lib/routes/admin-api/metrics.test.js index 1854f4959b..8059f2cb40 100644 --- a/lib/routes/admin-api/metrics.test.js +++ b/lib/routes/admin-api/metrics.test.js @@ -8,20 +8,22 @@ const permissions = require('../../../test/fixtures/permissions'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); const { UPDATE_APPLICATION } = require('../../permissions'); +const { createServices } = require('../../services'); const eventBus = new EventEmitter(); function getSetup() { const stores = store.createStores(); const perms = permissions(); - const app = getApp({ + const config = { baseUriPath: '', - stores, eventBus, extendedPermissions: true, preRouterHook: perms.hook, getLogger, - }); + }; + const services = createServices(stores, config); + const app = getApp({ ...config, stores }, services); return { request: supertest(app), diff --git a/lib/routes/client-api/metrics.js b/lib/routes/client-api/metrics.js index 6e5303e79d..529756a1fd 100644 --- a/lib/routes/client-api/metrics.js +++ b/lib/routes/client-api/metrics.js @@ -4,11 +4,15 @@ const Controller = require('../controller'); const { clientMetricsSchema } = require('./metrics-schema'); class ClientMetricsController extends Controller { - constructor({ clientMetricsStore, clientInstanceStore }, getLogger) { + constructor( + { clientMetricsStore, clientInstanceStore, featureToggleStore }, + getLogger, + ) { super(); this.logger = getLogger('/api/client/metrics'); this.clientMetricsStore = clientMetricsStore; this.clientInstanceStore = clientInstanceStore; + this.featureToggleStore = featureToggleStore; this.post('/', this.registerMetrics); } @@ -25,6 +29,8 @@ class ClientMetricsController extends Controller { } try { + const toggleNames = Object.keys(value.bucket.toggles); + await this.featureToggleStore.lastSeenToggles(toggleNames); await this.clientMetricsStore.insert(value); await this.clientInstanceStore.insert({ appName: value.appName, diff --git a/lib/routes/client-api/metrics.test.js b/lib/routes/client-api/metrics.test.js index 0e176c8a79..b1c8e3cca4 100644 --- a/lib/routes/client-api/metrics.test.js +++ b/lib/routes/client-api/metrics.test.js @@ -7,17 +7,20 @@ const store = require('../../../test/fixtures/store'); const getLogger = require('../../../test/fixtures/no-logger'); const getApp = require('../../app'); const { clientMetricsSchema } = require('./metrics-schema'); +const { createServices } = require('../../services'); const eventBus = new EventEmitter(); function getSetup() { const stores = store.createStores(); - const app = getApp({ + const config = { baseUriPath: '', stores, eventBus, getLogger, - }); + }; + const services = createServices(stores, config); + const app = getApp(config, services); return { request: supertest(app), diff --git a/lib/server-impl.test.js b/lib/server-impl.test.js index 6d80259e81..b9256bb158 100644 --- a/lib/server-impl.test.js +++ b/lib/server-impl.test.js @@ -38,7 +38,7 @@ const serverImpl = proxyquire('./server-impl', { return { db: { destroy: cb => cb() }, clientInstanceStore: { destroy: noop }, - clientMetricsStore: { destroy: noop }, + clientMetricsStore: { destroy: noop, on: noop }, eventStore, settingStore, }; diff --git a/lib/client-metrics/client-metrics.test.js b/lib/services/client-metrics/client-metrics.test.js similarity index 87% rename from lib/client-metrics/client-metrics.test.js rename to lib/services/client-metrics/client-metrics.test.js index be58a42369..f43f84f953 100644 --- a/lib/client-metrics/client-metrics.test.js +++ b/lib/services/client-metrics/client-metrics.test.js @@ -11,8 +11,8 @@ const appName = 'appName'; const instanceId = 'instanceId'; test('should work without state', t => { - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); t.truthy(metrics.getAppsWithToggles()); t.truthy(metrics.getTogglesMetrics()); @@ -23,8 +23,8 @@ test('should work without state', t => { test.cb('data should expire', t => { const clock = lolex.install(); - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); metrics.addPayload({ appName, @@ -64,9 +64,9 @@ test.cb('data should expire', t => { }); test('should listen to metrics from store', t => { - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); - store.emit('metrics', { + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); + clientMetricsStore.emit('metrics', { appName, instanceId, bucket: { @@ -122,9 +122,9 @@ test('should listen to metrics from store', t => { }); test('should build up list of seend toggles when new metrics arrives', t => { - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); - store.emit('metrics', { + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); + clientMetricsStore.emit('metrics', { appName, instanceId, bucket: { @@ -158,15 +158,15 @@ test('should build up list of seend toggles when new metrics arrives', t => { }); test('should handle a lot of toggles', t => { - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); const toggleCounts = {}; for (let i = 0; i < 100; i++) { toggleCounts[`toggle${i}`] = { yes: i, no: i }; } - store.emit('metrics', { + clientMetricsStore.emit('metrics', { appName, instanceId, bucket: { @@ -185,8 +185,8 @@ test('should handle a lot of toggles', t => { test('should have correct values for lastMinute', t => { const clock = lolex.install(); - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); const now = new Date(); const input = [ @@ -228,7 +228,7 @@ test('should have correct values for lastMinute', t => { ]; input.forEach(bucket => { - store.emit('metrics', { + clientMetricsStore.emit('metrics', { appName, instanceId, bucket, @@ -257,8 +257,8 @@ test('should have correct values for lastMinute', t => { test('should have correct values for lastHour', t => { const clock = lolex.install(); - const store = new EventEmitter(); - const metrics = new UnleashClientMetrics(store); + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); const now = new Date(); const input = [ @@ -293,7 +293,7 @@ test('should have correct values for lastHour', t => { ]; input.forEach(bucket => { - store.emit('metrics', { + clientMetricsStore.emit('metrics', { appName, instanceId, bucket, @@ -337,9 +337,9 @@ test('should have correct values for lastHour', t => { }); 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', { + const clientMetricsStore = new EventEmitter(); + const metrics = new UnleashClientMetrics({ clientMetricsStore }); + clientMetricsStore.emit('metrics', { appName, instanceId, bucket: { diff --git a/lib/client-metrics/index.js b/lib/services/client-metrics/index.js similarity index 97% rename from lib/client-metrics/index.js rename to lib/services/client-metrics/index.js index 67869c22ec..0f72b9a378 100644 --- a/lib/client-metrics/index.js +++ b/lib/services/client-metrics/index.js @@ -5,8 +5,8 @@ const Projection = require('./projection.js'); const TTLList = require('./ttl-list.js'); -module.exports = class UnleashClientMetrics { - constructor(clientMetricsStore) { +module.exports = class ClientMetricsService { + constructor({ clientMetricsStore }) { this.globalCount = 0; this.apps = {}; diff --git a/lib/client-metrics/list.js b/lib/services/client-metrics/list.js similarity index 100% rename from lib/client-metrics/list.js rename to lib/services/client-metrics/list.js diff --git a/lib/client-metrics/list.test.js b/lib/services/client-metrics/list.test.js similarity index 100% rename from lib/client-metrics/list.test.js rename to lib/services/client-metrics/list.test.js diff --git a/lib/client-metrics/projection.js b/lib/services/client-metrics/projection.js similarity index 100% rename from lib/client-metrics/projection.js rename to lib/services/client-metrics/projection.js diff --git a/lib/client-metrics/projection.test.js b/lib/services/client-metrics/projection.test.js similarity index 100% rename from lib/client-metrics/projection.test.js rename to lib/services/client-metrics/projection.test.js diff --git a/lib/client-metrics/ttl-list.js b/lib/services/client-metrics/ttl-list.js similarity index 100% rename from lib/client-metrics/ttl-list.js rename to lib/services/client-metrics/ttl-list.js diff --git a/lib/client-metrics/ttl-list.test.js b/lib/services/client-metrics/ttl-list.test.js similarity index 100% rename from lib/client-metrics/ttl-list.test.js rename to lib/services/client-metrics/ttl-list.test.js diff --git a/lib/services/index.js b/lib/services/index.js index 8703e247f0..a99c025dab 100644 --- a/lib/services/index.js +++ b/lib/services/index.js @@ -1,7 +1,9 @@ const ProjectService = require('./project-service'); const StateService = require('./state-service'); +const ClientMetricsService = require('./client-metrics'); module.exports.createServices = (stores, config) => ({ projectService: new ProjectService(stores, config), stateService: new StateService(stores, config), + clientMetricsService: new ClientMetricsService(stores, config), }); diff --git a/migrations/20201216140726-add-last-seen-to-features.js b/migrations/20201216140726-add-last-seen-to-features.js new file mode 100644 index 0000000000..747621f8d7 --- /dev/null +++ b/migrations/20201216140726-add-last-seen-to-features.js @@ -0,0 +1,10 @@ +exports.up = function(db, callback) { + db.runSql( + 'ALTER TABLE features ADD "last_seen_at" TIMESTAMP WITH TIME ZONE;', + callback, + ); +}; + +exports.down = function(db, cb) { + return db.removeColumn('features', 'last_seen_at', cb); +}; diff --git a/package.json b/package.json index 5563b48569..ea3dad8be0 100644 --- a/package.json +++ b/package.json @@ -89,7 +89,7 @@ "prom-client": "^12.0.0", "response-time": "^2.3.2", "serve-favicon": "^2.5.0", - "unleash-frontend": "3.8.2", + "unleash-frontend": "3.8.3", "yargs": "^16.0.3" }, "devDependencies": { diff --git a/test/e2e/helpers/test-helper.js b/test/e2e/helpers/test-helper.js index 521b8abc7f..d8e75fa1b0 100644 --- a/test/e2e/helpers/test-helper.js +++ b/test/e2e/helpers/test-helper.js @@ -7,26 +7,23 @@ const supertest = require('supertest'); const { EventEmitter } = require('events'); const getApp = require('../../../lib/app'); const getLogger = require('../../fixtures/no-logger'); -const StateService = require('../../../lib/services/state-service'); +const { createServices } = require('../../../lib/services'); const eventBus = new EventEmitter(); function createApp(stores, adminAuthentication = 'none', preHook) { - const services = { - stateService: new StateService(stores, { getLogger }), + const config = { + stores, + eventBus, + preHook, + adminAuthentication, + secret: 'super-secret', + sessionAge: 4000, + getLogger, }; - return getApp( - { - stores, - eventBus, - preHook, - adminAuthentication, - secret: 'super-secret', - sessionAge: 4000, - getLogger, - }, - services, - ); + const services = createServices(stores, config); + // TODO: use create from server-impl instead? + return getApp(config, services); } module.exports = { diff --git a/test/fixtures/fake-feature-toggle-store.js b/test/fixtures/fake-feature-toggle-store.js index c7f1056928..bfbc33fd1c 100644 --- a/test/fixtures/fake-feature-toggle-store.js +++ b/test/fixtures/fake-feature-toggle-store.js @@ -26,5 +26,6 @@ module.exports = () => { addFeature: feature => _features.push(feature), getArchivedFeatures: () => Promise.resolve(_archive), addArchivedFeature: feature => _archive.push(feature), + lastSeenToggles: () => {}, }; }; diff --git a/yarn.lock b/yarn.lock index 5f8946f644..60ad241fb1 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5715,10 +5715,10 @@ universalify@^0.1.0: resolved "https://registry.yarnpkg.com/universalify/-/universalify-0.1.2.tgz#b646f69be3942dabcecc9d6639c80dc105efaa66" integrity sha512-rBJeI5CXAlmy1pV+617WB9J63U6XcazHHF2f2dbJix4XzpUF0RS3Zbj0FGIOCAva5P/d/GBOYaACQ1w+0azUkg== -unleash-frontend@3.8.2: - version "3.8.2" - resolved "https://registry.yarnpkg.com/unleash-frontend/-/unleash-frontend-3.8.2.tgz#a33c7fa58b98071c77c06aa36a87c45fefc58c6e" - integrity sha512-ieuDsF7WMZnnIaT4g9Df0oxvV37HxWdcx9QkBzK9ykQDkCIMYOideu82lXrZjfnI8oUbh/ZBYTRiE60+t7RC4A== +unleash-frontend@3.8.3: + version "3.8.3" + resolved "https://registry.yarnpkg.com/unleash-frontend/-/unleash-frontend-3.8.3.tgz#18827b4f2a48af85ccc53a53c6ab05099326bcac" + integrity sha512-Q+EfvwLzYkfecKp+FMnXKqWz8fLRboCugX54uN6VIWPh7FBdcasCwY4od494HobRC8lFoBxiLtuTkvSglxZ4EA== unpipe@1.0.0, unpipe@~1.0.0: version "1.0.0"