diff --git a/src/lib/features/instance-stats/instance-stats-service.test.ts b/src/lib/features/instance-stats/instance-stats-service.test.ts index 0b9d9d6491..d3f423bec1 100644 --- a/src/lib/features/instance-stats/instance-stats-service.test.ts +++ b/src/lib/features/instance-stats/instance-stats-service.test.ts @@ -25,32 +25,38 @@ beforeEach(() => { createFakeGetProductionChanges(), ); - jest.spyOn(instanceStatsService, 'refreshStatsSnapshot'); + jest.spyOn(instanceStatsService, 'refreshAppCountSnapshot'); + jest.spyOn(instanceStatsService, 'getLabeledAppCounts'); jest.spyOn(instanceStatsService, 'getStats'); // validate initial state without calls to these methods - expect(instanceStatsService.refreshStatsSnapshot).toBeCalledTimes(0); - expect(instanceStatsService.getStats).toBeCalledTimes(0); + expect(instanceStatsService.refreshAppCountSnapshot).toHaveBeenCalledTimes( + 0, + ); + expect(instanceStatsService.getStats).toHaveBeenCalledTimes(0); }); test('get snapshot should not call getStats', async () => { - await instanceStatsService.refreshStatsSnapshot(); - expect(instanceStatsService.getStats).toBeCalledTimes(1); + await instanceStatsService.refreshAppCountSnapshot(); + expect(instanceStatsService.getLabeledAppCounts).toHaveBeenCalledTimes(1); + expect(instanceStatsService.getStats).toHaveBeenCalledTimes(0); // subsequent calls to getStatsSnapshot don't call getStats for (let i = 0; i < 3; i++) { - const stats = instanceStatsService.getStatsSnapshot(); - expect(stats?.clientApps).toStrictEqual([ - { range: 'allTime', count: 0 }, - { range: '30d', count: 0 }, - { range: '7d', count: 0 }, + const { clientApps } = await instanceStatsService.getStats(); + expect(clientApps).toStrictEqual([ + { count: 0, range: '7d' }, + { count: 0, range: '30d' }, + { count: 0, range: 'allTime' }, ]); } // after querying the stats snapshot no call to getStats should be issued - expect(instanceStatsService.getStats).toBeCalledTimes(1); + expect(instanceStatsService.getLabeledAppCounts).toHaveBeenCalledTimes(1); }); test('before the snapshot is refreshed we can still get the appCount', async () => { - expect(instanceStatsService.refreshStatsSnapshot).toBeCalledTimes(0); + expect(instanceStatsService.refreshAppCountSnapshot).toHaveBeenCalledTimes( + 0, + ); expect(instanceStatsService.getAppCountSnapshot('7d')).toBeUndefined(); }); diff --git a/src/lib/features/instance-stats/instance-stats-service.ts b/src/lib/features/instance-stats/instance-stats-service.ts index 60735c5b93..426762bd77 100644 --- a/src/lib/features/instance-stats/instance-stats-service.ts +++ b/src/lib/features/instance-stats/instance-stats-service.ts @@ -99,8 +99,6 @@ export class InstanceStatsService { private clientMetricsStore: IClientMetricsStoreV2; - private snapshot?: InstanceStats; - private appCount?: Partial<{ [key in TimeRange]: number }>; private getActiveUsers: GetActiveUsers; @@ -165,19 +163,22 @@ export class InstanceStatsService { this.clientMetricsStore = clientMetricsStoreV2; } - async refreshStatsSnapshot(): Promise { + async refreshAppCountSnapshot(): Promise< + Partial<{ [key in TimeRange]: number }> + > { try { - this.snapshot = await this.getStats(); - const appCountReplacement = {}; - this.snapshot.clientApps?.forEach((appCount) => { - appCountReplacement[appCount.range] = appCount.count; - }); - this.appCount = appCountReplacement; + this.appCount = await this.getLabeledAppCounts(); + return this.appCount; } catch (error) { this.logger.warn( 'Unable to retrieve statistics. This will be retried', error, ); + return { + '7d': 0, + '30d': 0, + allTime: 0, + }; } } @@ -251,7 +252,7 @@ export class InstanceStatsService { this.strategyStore.count(), this.hasSAML(), this.hasOIDC(), - this.getLabeledAppCounts(), + this.appCount ? this.appCount : this.refreshAppCountSnapshot(), this.eventStore.filteredCount({ type: FEATURES_EXPORTED }), this.eventStore.filteredCount({ type: FEATURES_IMPORTED }), this.getProductionChanges(), @@ -279,7 +280,10 @@ export class InstanceStatsService { strategies, SAMLenabled, OIDCenabled, - clientApps, + clientApps: Object.entries(clientApps).map(([range, count]) => ({ + range: range as TimeRange, + count, + })), featureExports, featureImports, productionChanges, @@ -287,34 +291,19 @@ export class InstanceStatsService { }; } - getStatsSnapshot(): InstanceStats | undefined { - return this.snapshot; - } - async getLabeledAppCounts(): Promise< - { range: TimeRange; count: number }[] + Partial<{ [key in TimeRange]: number }> > { - return [ - { - range: 'allTime', - count: - await this.clientInstanceStore.getDistinctApplicationsCount(), - }, - { - range: '30d', - count: - await this.clientInstanceStore.getDistinctApplicationsCount( - 30, - ), - }, - { - range: '7d', - count: - await this.clientInstanceStore.getDistinctApplicationsCount( - 7, - ), - }, - ]; + const [t7d, t30d, allTime] = await Promise.all([ + this.clientInstanceStore.getDistinctApplicationsCount(7), + this.clientInstanceStore.getDistinctApplicationsCount(30), + this.clientInstanceStore.getDistinctApplicationsCount(), + ]); + return { + '7d': t7d, + '30d': t30d, + allTime, + }; } getAppCountSnapshot(range: TimeRange): number | undefined { diff --git a/src/lib/features/scheduler/schedule-services.ts b/src/lib/features/scheduler/schedule-services.ts index dce98aca85..a37994106f 100644 --- a/src/lib/features/scheduler/schedule-services.ts +++ b/src/lib/features/scheduler/schedule-services.ts @@ -57,9 +57,9 @@ export const scheduleServices = async ( ); schedulerService.schedule( - instanceStatsService.refreshStatsSnapshot.bind(instanceStatsService), + instanceStatsService.refreshAppCountSnapshot.bind(instanceStatsService), minutesToMilliseconds(5), - 'refreshStatsSnapshot', + 'refreshAppCountSnapshot', ); schedulerService.schedule( diff --git a/src/lib/metrics.test.ts b/src/lib/metrics.test.ts index e8ea2b1bdc..65fffc29e1 100644 --- a/src/lib/metrics.test.ts +++ b/src/lib/metrics.test.ts @@ -17,6 +17,8 @@ import { createFakeGetActiveUsers } from './features/instance-stats/getActiveUse import { createFakeGetProductionChanges } from './features/instance-stats/getProductionChanges'; import { IEnvironmentStore, IUnleashStores } from './types'; import FakeEnvironmentStore from './features/project-environments/fake-environment-store'; +import { SchedulerService } from './services'; +import noLogger from '../test/fixtures/no-logger'; const monitor = createMetricsMonitor(); const eventBus = new EventEmitter(); @@ -25,7 +27,8 @@ let eventStore: IEventStore; let environmentStore: IEnvironmentStore; let statsService: InstanceStatsService; let stores: IUnleashStores; -beforeAll(() => { +let schedulerService: SchedulerService; +beforeAll(async () => { const config = createTestConfig({ server: { serverMetrics: true, @@ -49,6 +52,14 @@ beforeAll(() => { createFakeGetProductionChanges(), ); + schedulerService = new SchedulerService( + noLogger, + { + isMaintenanceMode: () => Promise.resolve(false), + }, + eventBus, + ); + const db = { client: { pool: { @@ -61,19 +72,21 @@ beforeAll(() => { }, }, }; - // @ts-ignore - We don't want a full knex implementation for our tests, it's enough that it actually yields the numbers we want. - monitor.startMonitoring( + + await monitor.startMonitoring( config, stores, '4.0.0', eventBus, statsService, - //@ts-ignore + schedulerService, + // @ts-ignore - We don't want a full knex implementation for our tests, it's enough that it actually yields the numbers we want. db, ); }); -afterAll(() => { - monitor.stopMonitoring(); + +afterAll(async () => { + schedulerService.stop(); }); test('should collect metrics for requests', async () => { @@ -160,17 +173,12 @@ test('should collect metrics for db query timings', async () => { }); test('should collect metrics for feature toggle size', async () => { - await new Promise((done) => { - setTimeout(done, 10); - }); const metrics = await prometheusRegister.metrics(); expect(metrics).toMatch(/feature_toggles_total\{version="(.*)"\} 0/); }); test('should collect metrics for total client apps', async () => { - await new Promise((done) => { - setTimeout(done, 10); - }); + await statsService.refreshAppCountSnapshot(); const metrics = await prometheusRegister.metrics(); expect(metrics).toMatch(/client_apps_total\{range="(.*)"\} 0/); }); diff --git a/src/lib/metrics.ts b/src/lib/metrics.ts index d4f49efd10..dcc5c3c85c 100644 --- a/src/lib/metrics.ts +++ b/src/lib/metrics.ts @@ -26,23 +26,18 @@ import { InstanceStatsService } from './features/instance-stats/instance-stats-s import { ValidatedClientMetrics } from './features/metrics/shared/schema'; import { IEnvironment } from './types'; import { createCounter, createGauge, createSummary } from './util/metrics'; +import { SchedulerService } from './services'; export default class MetricsMonitor { - timer?: NodeJS.Timeout; + constructor() {} - poolMetricsTimer?: NodeJS.Timeout; - - constructor() { - this.timer = undefined; - this.poolMetricsTimer = undefined; - } - - startMonitoring( + async startMonitoring( config: IUnleashConfig, stores: IUnleashStores, version: string, eventBus: EventEmitter, instanceStatsService: InstanceStatsService, + schedulerService: SchedulerService, db: Knex, ): Promise { if (!config.server.serverMetrics) { @@ -317,10 +312,8 @@ export default class MetricsMonitor { oidcEnabled.set(stats.OIDCenabled ? 1 : 0); clientAppsTotal.reset(); - stats.clientApps.forEach((clientStat) => - clientAppsTotal - .labels({ range: clientStat.range }) - .set(clientStat.count), + stats.clientApps.forEach(({ range, count }) => + clientAppsTotal.labels({ range }).set(count), ); rateLimits.reset(); @@ -361,13 +354,12 @@ export default class MetricsMonitor { } catch (e) {} } - process.nextTick(() => { - collectStaticCounters(); - this.timer = setInterval( - () => collectStaticCounters(), - hoursToMilliseconds(2), - ).unref(); - }); + await schedulerService.schedule( + collectStaticCounters.bind(this), + hoursToMilliseconds(2), + 'collectStaticCounters', + 0, // no jitter + ); eventBus.on( events.REQUEST_TIME, @@ -548,17 +540,16 @@ export default class MetricsMonitor { } }); - this.configureDbMetrics(db, eventBus); + await this.configureDbMetrics(db, eventBus, schedulerService); return Promise.resolve(); } - stopMonitoring(): void { - clearInterval(this.timer); - clearInterval(this.poolMetricsTimer); - } - - configureDbMetrics(db: Knex, eventBus: EventEmitter): void { + async configureDbMetrics( + db: Knex, + eventBus: EventEmitter, + schedulerService: SchedulerService, + ): Promise { if (db?.client) { const dbPoolMin = createGauge({ name: 'db_pool_min', @@ -594,12 +585,12 @@ export default class MetricsMonitor { dbPoolPendingAcquires.set(data.pendingAcquires); }); - this.registerPoolMetrics(db.client.pool, eventBus); - this.poolMetricsTimer = setInterval( - () => this.registerPoolMetrics(db.client.pool, eventBus), + await schedulerService.schedule( + this.registerPoolMetrics.bind(this, db.client.pool, eventBus), minutesToMilliseconds(1), + 'registerPoolMetrics', + 0, // no jitter ); - this.poolMetricsTimer.unref(); } } diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index f2c93aeb47..4456bc537a 100644 --- a/src/lib/server-impl.ts +++ b/src/lib/server-impl.ts @@ -60,7 +60,6 @@ async function createApp( await stopServer(); } services.schedulerService.stop(); - metricsMonitor.stopMonitoring(); services.addonService.destroy(); await db.destroy(); }; @@ -77,6 +76,7 @@ async function createApp( serverVersion, config.eventBus, services.instanceStatsService, + services.schedulerService, db, ); const unleash: Omit = { diff --git a/src/test/fixtures/fake-client-instance-store.ts b/src/test/fixtures/fake-client-instance-store.ts index 531375d6e2..2d0f6f513d 100644 --- a/src/test/fixtures/fake-client-instance-store.ts +++ b/src/test/fixtures/fake-client-instance-store.ts @@ -28,7 +28,7 @@ export default class FakeClientInstanceStore implements IClientInstanceStore { } setLastSeen(): Promise { - return; + return Promise.resolve(); } async getBySdkName(sdkName: string): Promise { @@ -84,7 +84,8 @@ export default class FakeClientInstanceStore implements IClientInstanceStore { } async getDistinctApplicationsCount(): Promise { - return this.getDistinctApplications().then((apps) => apps.length); + const apps = await this.getDistinctApplications(); + return apps.length; } async insert(details: INewClientInstance): Promise {