chore: reimplementation of app stats (#6155)

## About the changes
App stats is mainly used to cap the number of applications reported to
Unleash based on the last 7 days information:
cc2ccb1134/src/lib/middleware/response-time-metrics.ts (L24-L28)

Instead of getting all stats, just calculate appCount statistics

Use scheduler service instead of setInterval

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();
 });

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<void> {
+    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 {

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(

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/);
 });

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<void> {
         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(),
+        await schedulerService.schedule(
+            collectStaticCounters.bind(this),
             hoursToMilliseconds(2),
-            ).unref();
-        });
+            '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<void> {
         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();
         }
     }
 

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<IUnleash, 'stop'> = {

src/test/fixtures/fake-client-instance-store.ts

@@ -28,7 +28,7 @@ export default class FakeClientInstanceStore implements IClientInstanceStore {
     }
 
     setLastSeen(): Promise<void> {
-        return;
+        return Promise.resolve();
     }
 
     async getBySdkName(sdkName: string): Promise<IClientInstance[]> {
@@ -84,7 +84,8 @@ export default class FakeClientInstanceStore implements IClientInstanceStore {
     }
 
     async getDistinctApplicationsCount(): Promise<number> {
-        return this.getDistinctApplications().then((apps) => apps.length);
+        const apps = await this.getDistinctApplications();
+        return apps.length;
     }
 
     async insert(details: INewClientInstance): Promise<void> {