From 8d8a975c6c63b445933c6f94d209290b5c20a375 Mon Sep 17 00:00:00 2001 From: Mateusz Kwasniewski Date: Mon, 23 Oct 2023 15:22:30 +0200 Subject: [PATCH] Client instance service (#5126) --- .../tests/client-feature-toggle.e2e.test.ts | 9 --- src/lib/routes/admin-api/config.test.ts | 9 --- src/lib/routes/admin-api/context.test.ts | 9 --- src/lib/routes/admin-api/metrics.test.ts | 9 --- .../routes/admin-api/public-signup.test.ts | 9 --- src/lib/routes/admin-api/strategy.test.ts | 10 --- src/lib/routes/admin-api/tag.test.ts | 8 --- src/lib/routes/backstage.test.ts | 1 - src/lib/routes/client-api/metrics.test.ts | 13 ++-- src/lib/routes/client-api/register.test.ts | 6 -- src/lib/routes/health-check.test.ts | 6 -- src/lib/routes/public-invite.test.ts | 9 --- .../client-metrics/instance-service.test.ts | 62 +++---------------- .../client-metrics/instance-service.ts | 24 ------- src/lib/services/index.ts | 12 ++++ .../services/public-signup-token-service.ts | 7 --- .../client-metrics-service.e2e.test.ts | 9 +-- 17 files changed, 32 insertions(+), 180 deletions(-) diff --git a/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts b/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts index 5b817dc51b28..c2ec3f4150f7 100644 --- a/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts +++ b/src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts @@ -24,9 +24,6 @@ async function getSetup() { base, clientFeatureToggleStore: stores.clientFeatureToggleStore, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } @@ -43,7 +40,6 @@ const callGetAll = async (controller: FeatureController) => { let base; let request; -let destroy; let flagResolver; @@ -51,7 +47,6 @@ beforeEach(async () => { const setup = await getSetup(); base = setup.base; request = setup.request; - destroy = setup.destroy; flagResolver = { isEnabled: () => { return false; @@ -59,10 +54,6 @@ beforeEach(async () => { }; }); -afterEach(() => { - destroy(); -}); - test('should get empty getFeatures via client', () => { expect.assertions(1); return request diff --git a/src/lib/routes/admin-api/config.test.ts b/src/lib/routes/admin-api/config.test.ts index 99a14438ccbc..a73025a88771 100644 --- a/src/lib/routes/admin-api/config.test.ts +++ b/src/lib/routes/admin-api/config.test.ts @@ -28,25 +28,16 @@ async function getSetup() { return { base, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let request; let base; -let destroy; beforeEach(async () => { const setup = await getSetup(); request = setup.request; base = setup.base; - destroy = setup.destroy; -}); - -afterEach(() => { - destroy(); }); test('should get ui config', async () => { diff --git a/src/lib/routes/admin-api/context.test.ts b/src/lib/routes/admin-api/context.test.ts index c3b40594a083..0a584ad3323c 100644 --- a/src/lib/routes/admin-api/context.test.ts +++ b/src/lib/routes/admin-api/context.test.ts @@ -20,25 +20,16 @@ async function getSetup() { return { base, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let base; let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); base = setup.base; request = setup.request; - destroy = setup.destroy; -}); - -afterEach(async () => { - await destroy(); }); test('should get all context definitions', () => { diff --git a/src/lib/routes/admin-api/metrics.test.ts b/src/lib/routes/admin-api/metrics.test.ts index ef56d0876dfe..2b704b72494f 100644 --- a/src/lib/routes/admin-api/metrics.test.ts +++ b/src/lib/routes/admin-api/metrics.test.ts @@ -19,25 +19,16 @@ async function getSetup() { stores, perms, config, - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let stores; let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); stores = setup.stores; request = setup.request; - destroy = setup.destroy; -}); - -afterEach(() => { - destroy(); }); test('/api/admin/metrics/seen-toggles is deprecated', () => { diff --git a/src/lib/routes/admin-api/public-signup.test.ts b/src/lib/routes/admin-api/public-signup.test.ts index 342181bbbc05..be0bb8013557 100644 --- a/src/lib/routes/admin-api/public-signup.test.ts +++ b/src/lib/routes/admin-api/public-signup.test.ts @@ -33,16 +33,11 @@ describe('Public Signup API', () => { request: supertest(app), stores, perms, - destroy: () => { - services.clientInstanceService.destroy(); - services.publicSignupTokenService.destroy(); - }, }; } let stores; let request; - let destroy; const user = { username: 'some-username', @@ -55,12 +50,8 @@ describe('Public Signup API', () => { const setup = await getSetup(); stores = setup.stores; request = setup.request; - destroy = setup.destroy; }); - afterEach(() => { - destroy(); - }); const expireAt = (addDays: number = 7): Date => { const now = new Date(); now.setDate(now.getDate() + addDays); diff --git a/src/lib/routes/admin-api/strategy.test.ts b/src/lib/routes/admin-api/strategy.test.ts index f8538ec62a73..d21225f0983d 100644 --- a/src/lib/routes/admin-api/strategy.test.ts +++ b/src/lib/routes/admin-api/strategy.test.ts @@ -5,8 +5,6 @@ import permissions from '../../../test/fixtures/permissions'; import getApp from '../../app'; import { createServices } from '../../services'; -let destroy; - async function getSetup() { const randomBase = `/random${Math.round(Math.random() * 1000)}`; const perms = permissions(); @@ -18,10 +16,6 @@ async function getSetup() { const services = createServices(stores, config); const app = await getApp(config, stores, services); - destroy = () => { - services.clientInstanceService.destroy(); - }; - return { base: randomBase, strategyStore: stores.strategyStore, @@ -30,10 +24,6 @@ async function getSetup() { }; } -afterEach(() => { - destroy(); -}); - test('add version numbers for /strategies', async () => { const { request, base } = await getSetup(); return request diff --git a/src/lib/routes/admin-api/tag.test.ts b/src/lib/routes/admin-api/tag.test.ts index ad183a62ebc1..5a0b2169ca22 100644 --- a/src/lib/routes/admin-api/tag.test.ts +++ b/src/lib/routes/admin-api/tag.test.ts @@ -21,26 +21,18 @@ async function getSetup() { perms, tagStore: stores.tagStore, request: supertest(app), - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let base; let tagStore; let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); base = setup.base; tagStore = setup.tagStore; request = setup.request; - destroy = setup.destroy; -}); -afterEach(() => { - destroy(); }); test('should get empty getTags via admin', () => { diff --git a/src/lib/routes/backstage.test.ts b/src/lib/routes/backstage.test.ts index 23572d566560..fd77cc9624e4 100644 --- a/src/lib/routes/backstage.test.ts +++ b/src/lib/routes/backstage.test.ts @@ -19,5 +19,4 @@ test('should enable prometheus', async () => { .get('/internal-backstage/prometheus') .expect('Content-Type', /text/) .expect(200); - services.clientInstanceService.destroy(); }); diff --git a/src/lib/routes/client-api/metrics.test.ts b/src/lib/routes/client-api/metrics.test.ts index 1fd8cea49101..fb48c324f131 100644 --- a/src/lib/routes/client-api/metrics.test.ts +++ b/src/lib/routes/client-api/metrics.test.ts @@ -19,10 +19,7 @@ async function getSetup(opts?: IUnleashOptions) { request: supertest(app), stores: db.stores, services, - destroy: async () => { - services.clientInstanceService.destroy(); - await db.destroy(); - }, + destroy: db.destroy, }; } @@ -31,7 +28,7 @@ let stores: IUnleashStores; let services: IUnleashServices; let destroy; -beforeEach(async () => { +beforeAll(async () => { const setup = await getSetup(); request = setup.request; stores = setup.stores; @@ -39,10 +36,14 @@ beforeEach(async () => { services = setup.services; }); -afterEach(() => { +afterAll(() => { destroy(); }); +afterEach(async () => { + await stores.featureToggleStore.deleteAll(); +}); + test('should validate client metrics', () => { return request .post('/api/client/metrics') diff --git a/src/lib/routes/client-api/register.test.ts b/src/lib/routes/client-api/register.test.ts index 64704e7513bc..70e0dd21a55f 100644 --- a/src/lib/routes/client-api/register.test.ts +++ b/src/lib/routes/client-api/register.test.ts @@ -14,20 +14,14 @@ async function getSetup() { return { request: supertest(app), stores, - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); request = setup.request; - destroy = setup.destroy; }); afterEach(() => { - destroy(); getLogger.setMuteError(false); }); diff --git a/src/lib/routes/health-check.test.ts b/src/lib/routes/health-check.test.ts index 1149f8e8f6f3..a9b0ec657d12 100644 --- a/src/lib/routes/health-check.test.ts +++ b/src/lib/routes/health-check.test.ts @@ -15,21 +15,15 @@ async function getSetup() { return { request: supertest(app), stores, - destroy: () => { - services.clientInstanceService.destroy(); - }, }; } let request; -let destroy; beforeEach(async () => { const setup = await getSetup(); request = setup.request; - destroy = setup.destroy; }); afterEach(() => { - destroy(); getLogger.setMuteError(false); }); diff --git a/src/lib/routes/public-invite.test.ts b/src/lib/routes/public-invite.test.ts index b55dc6ddd3fe..c76bf6aff406 100644 --- a/src/lib/routes/public-invite.test.ts +++ b/src/lib/routes/public-invite.test.ts @@ -39,16 +39,11 @@ describe('Public Signup API', () => { request: supertest(app), stores, perms, - destroy: () => { - services.clientInstanceService.destroy(); - services.publicSignupTokenService.destroy(); - }, }; } let stores; let request; - let destroy; const user = { username: 'some-username', @@ -61,12 +56,8 @@ describe('Public Signup API', () => { const setup = await getSetup(); stores = setup.stores; request = setup.request; - destroy = setup.destroy; }); - afterEach(() => { - destroy(); - }); const expireAt = (addDays: number = 7): Date => { const now = new Date(); now.setDate(now.getDate() + addDays); diff --git a/src/lib/services/client-metrics/instance-service.test.ts b/src/lib/services/client-metrics/instance-service.test.ts index dcb53880dc39..e706b7a48881 100644 --- a/src/lib/services/client-metrics/instance-service.test.ts +++ b/src/lib/services/client-metrics/instance-service.test.ts @@ -5,45 +5,11 @@ import FakeEventStore from '../../../test/fixtures/fake-event-store'; import { createTestConfig } from '../../../test/config/test-config'; import { FakePrivateProjectChecker } from '../../features/private-project/fakePrivateProjectChecker'; -/** - * A utility to wait for any pending promises in the test subject code. - * For instance, if the test needs to wait for a timeout/interval handler, - * and that handler does something async, advancing the timers is not enough: - * We have to explicitly wait for the second promise. - * For more info, see https://stackoverflow.com/a/51045733/2868829 - * - * Usage in test code after advancing timers, but before making assertions: - * - * test('hello', async () => { - * jest.useFakeTimers('modern'); - * - * // Schedule a timeout with a callback that does something async - * // before calling our spy - * const spy = jest.fn(); - * setTimeout(async () => { - * await Promise.resolve(); - * spy(); - * }, 1000); - * - * expect(spy).not.toHaveBeenCalled(); - * - * jest.advanceTimersByTime(1500); - * await flushPromises(); // this is required to make it work! - * - * expect(spy).toHaveBeenCalledTimes(1); - * - * jest.useRealTimers(); - * }); - */ -function flushPromises() { - return Promise.resolve(setImmediate); -} let config; beforeAll(() => { config = createTestConfig({}); }); test('Multiple registrations of same appname and instanceid within same time period should only cause one registration', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -75,8 +41,8 @@ test('Multiple registrations of same appname and instanceid within same time per await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); - jest.advanceTimersByTime(7000); - await flushPromises(); + + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService expect(appStoreSpy).toHaveBeenCalledTimes(1); expect(bulkSpy).toHaveBeenCalledTimes(1); @@ -93,7 +59,6 @@ test('Multiple registrations of same appname and instanceid within same time per }); test('Multiple unique clients causes multiple registrations', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -136,16 +101,14 @@ test('Multiple unique clients causes multiple registrations', async () => { await clientMetrics.registerClient(client2, '127.0.0.1'); await clientMetrics.registerClient(client2, '127.0.0.1'); - jest.advanceTimersByTime(7000); - await flushPromises(); + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService const registrations = appStoreSpy.mock.calls[0][0]; expect(registrations.length).toBe(2); - jest.useRealTimers(); }); + test('Same client registered outside of dedup interval will be registered twice', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -155,8 +118,6 @@ test('Same client registered outside of dedup interval will be registered twice' bulkUpsert: bulkSpy, }; - const bulkInterval = secondsToMilliseconds(2); - const clientMetrics = new ClientInstanceService( { clientMetricsStoreV2: null, @@ -168,7 +129,6 @@ test('Same client registered outside of dedup interval will be registered twice' }, config, new FakePrivateProjectChecker(), - bulkInterval, ); const client1 = { appName: 'test_app', @@ -181,14 +141,13 @@ test('Same client registered outside of dedup interval will be registered twice' await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); - jest.advanceTimersByTime(3000); + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); await clientMetrics.registerClient(client1, '127.0.0.1'); - jest.advanceTimersByTime(3000); - await flushPromises(); + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService expect(appStoreSpy).toHaveBeenCalledTimes(2); expect(bulkSpy).toHaveBeenCalledTimes(2); @@ -198,11 +157,9 @@ test('Same client registered outside of dedup interval will be registered twice' expect(firstRegistrations.appName).toBe(secondRegistrations.appName); expect(firstRegistrations.instanceId).toBe(secondRegistrations.instanceId); - jest.useRealTimers(); }); test('No registrations during a time period will not call stores', async () => { - jest.useFakeTimers(); const appStoreSpy = jest.fn(); const bulkSpy = jest.fn(); const clientApplicationsStore: any = { @@ -211,7 +168,7 @@ test('No registrations during a time period will not call stores', async () => { const clientInstanceStore: any = { bulkUpsert: bulkSpy, }; - new ClientInstanceService( + const clientMetrics = new ClientInstanceService( { clientMetricsStoreV2: null, strategyStore: null, @@ -223,8 +180,9 @@ test('No registrations during a time period will not call stores', async () => { config, new FakePrivateProjectChecker(), ); - jest.advanceTimersByTime(6000); + + await clientMetrics.bulkAdd(); // in prod called by a SchedulerService + expect(appStoreSpy).toHaveBeenCalledTimes(0); expect(bulkSpy).toHaveBeenCalledTimes(0); - jest.useRealTimers(); }); diff --git a/src/lib/services/client-metrics/instance-service.ts b/src/lib/services/client-metrics/instance-service.ts index 9af1bb41935d..7b31d891a615 100644 --- a/src/lib/services/client-metrics/instance-service.ts +++ b/src/lib/services/client-metrics/instance-service.ts @@ -29,8 +29,6 @@ export default class ClientInstanceService { seenClients: Record = {}; - private timers: NodeJS.Timeout[] = []; - private clientMetricsStoreV2: IClientMetricsStoreV2; private strategyStore: IStrategyStore; @@ -47,10 +45,6 @@ export default class ClientInstanceService { private flagResolver: IFlagResolver; - private bulkInterval: number; - - private announcementInterval: number; - constructor( { clientMetricsStoreV2, @@ -73,8 +67,6 @@ export default class ClientInstanceService { flagResolver, }: Pick, privateProjectChecker: IPrivateProjectChecker, - bulkInterval = secondsToMilliseconds(5), - announcementInterval = minutesToMilliseconds(5), ) { this.clientMetricsStoreV2 = clientMetricsStoreV2; this.strategyStore = strategyStore; @@ -87,18 +79,6 @@ export default class ClientInstanceService { this.logger = getLogger( '/services/client-metrics/client-instance-service.ts', ); - - this.bulkInterval = bulkInterval; - this.announcementInterval = announcementInterval; - this.timers.push( - setInterval(() => this.bulkAdd(), this.bulkInterval).unref(), - ); - this.timers.push( - setInterval( - () => this.announceUnannounced(), - this.announcementInterval, - ).unref(), - ); } public async registerInstance( @@ -248,8 +228,4 @@ export default class ClientInstanceService { async removeInstancesOlderThanTwoDays(): Promise { return this.clientInstanceStore.removeInstancesOlderThanTwoDays(); } - - destroy(): void { - this.timers.forEach(clearInterval); - } } diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 213093f9aac4..e5638cf7a9cc 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -165,6 +165,18 @@ export const scheduleServices = async ( 'removeInstancesOlderThanTwoDays', ); + schedulerService.schedule( + clientInstanceService.bulkAdd.bind(clientInstanceService), + secondsToMilliseconds(5), + 'bulkAddInstances', + ); + + schedulerService.schedule( + clientInstanceService.announceUnannounced.bind(clientInstanceService), + minutesToMilliseconds(5), + 'announceUnannounced', + ); + schedulerService.schedule( projectService.statusJob.bind(projectService), hoursToMilliseconds(24), diff --git a/src/lib/services/public-signup-token-service.ts b/src/lib/services/public-signup-token-service.ts index 3fd855cdb579..57010297977d 100644 --- a/src/lib/services/public-signup-token-service.ts +++ b/src/lib/services/public-signup-token-service.ts @@ -30,8 +30,6 @@ export class PublicSignupTokenService { private logger: Logger; - private timer: NodeJS.Timeout; - private readonly unleashBase: string; constructor( @@ -146,9 +144,4 @@ export class PublicSignupTokenService { private getMinimumDate(date1: Date, date2: Date): Date { return date1 < date2 ? date1 : date2; } - - destroy(): void { - clearInterval(this.timer); - this.timer = null; - } } diff --git a/src/test/e2e/services/client-metrics-service.e2e.test.ts b/src/test/e2e/services/client-metrics-service.e2e.test.ts index 2f258e19a8f4..e9088285dc82 100644 --- a/src/test/e2e/services/client-metrics-service.e2e.test.ts +++ b/src/test/e2e/services/client-metrics-service.e2e.test.ts @@ -12,7 +12,7 @@ const { APPLICATION_CREATED } = require('../../../lib/types/events'); let stores; let db; -let clientInstanceService; +let clientInstanceService: ClientInstanceService; let config: IUnleashConfig; beforeAll(async () => { db = await dbInit('client_metrics_service_serial', getLogger); @@ -25,13 +25,10 @@ beforeAll(async () => { stores, config, new FakePrivateProjectChecker(), - bulkInterval, - announcementInterval, ); }); afterAll(async () => { - await clientInstanceService.destroy(); await db.destroy(); }); test('Apps registered should be announced', async () => { @@ -58,11 +55,11 @@ test('Apps registered should be announced', async () => { }; await clientInstanceService.registerClient(clientRegistration, '127.0.0.1'); await clientInstanceService.registerClient(differentClient, '127.0.0.1'); - await new Promise((res) => setTimeout(res, 1200)); + await clientInstanceService.bulkAdd(); // in prod called by a SchedulerService const first = await stores.clientApplicationsStore.getUnannounced(); expect(first.length).toBe(2); await clientInstanceService.registerClient(clientRegistration, '127.0.0.1'); - await new Promise((res) => setTimeout(res, secondsToMilliseconds(2))); + await clientInstanceService.announceUnannounced(); // in prod called by a SchedulerService const second = await stores.clientApplicationsStore.getUnannounced(); expect(second.length).toBe(0); const events = await stores.eventStore.getEvents();