Skip to content

Commit

Permalink
chore: reimplementation of app stats
Browse files Browse the repository at this point in the history
  • Loading branch information
@gastonfournier
gastonfournier committed Feb 7, 2024
1 parent 11bfcd6 commit 49622f1
Show file tree
Hide file tree
Showing 6 changed files with 72 additions and 95 deletions.
32 changes: 19 additions & 13 deletions src/lib/features/instance-stats/instance-stats-service.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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({
allTime: 0,
'30d': 0,
'7d': 0,
});
}
// 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();
});
54 changes: 15 additions & 39 deletions src/lib/features/instance-stats/instance-stats-service.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ export interface InstanceStats {
strategies: number;
SAMLenabled: boolean;
OIDCenabled: boolean;
clientApps: { range: TimeRange; count: number }[];
clientApps: Partial<{ [key in TimeRange]: number }>;
activeUsers: Awaited<ReturnType<GetActiveUsers>>;
productionChanges: Awaited<ReturnType<GetProductionChanges>>;
previousDayMetricsBucketsCount: {
Expand Down Expand Up @@ -99,8 +99,6 @@ export class InstanceStatsService {

private clientMetricsStore: IClientMetricsStoreV2;

private snapshot?: InstanceStats;

private appCount?: Partial<{ [key in TimeRange]: number }>;

private getActiveUsers: GetActiveUsers;
Expand Down Expand Up @@ -165,14 +163,9 @@ export class InstanceStatsService {
this.clientMetricsStore = clientMetricsStoreV2;
}

async refreshStatsSnapshot(): Promise<void> {
async refreshAppCountSnapshot(): Promise<void> {
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();
} catch (error) {
this.logger.warn(
'Unable to retrieve statistics. This will be retried',
Expand Down Expand Up @@ -229,7 +222,6 @@ export class InstanceStatsService {
strategies,
SAMLenabled,
OIDCenabled,
clientApps,
featureExports,
featureImports,
productionChanges,
Expand All @@ -251,7 +243,6 @@ export class InstanceStatsService {
this.strategyStore.count(),
this.hasSAML(),
this.hasOIDC(),
this.getLabeledAppCounts(),
this.eventStore.filteredCount({ type: FEATURES_EXPORTED }),
this.eventStore.filteredCount({ type: FEATURES_IMPORTED }),
this.getProductionChanges(),
Expand Down Expand Up @@ -279,42 +270,27 @@ export class InstanceStatsService {
strategies,
SAMLenabled,
OIDCenabled,
clientApps,
clientApps: this.appCount ?? {},
featureExports,
featureImports,
productionChanges,
previousDayMetricsBucketsCount,
};
}

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 {
Expand Down
4 changes: 2 additions & 2 deletions src/lib/features/scheduler/schedule-services.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,9 +57,9 @@ export const scheduleServices = async (
);

schedulerService.schedule(
instanceStatsService.refreshStatsSnapshot.bind(instanceStatsService),
instanceStatsService.refreshAppCountSnapshot.bind(instanceStatsService),
minutesToMilliseconds(5),
'refreshStatsSnapshot',
'refreshAppCountSnapshot',
);

schedulerService.schedule(
Expand Down
28 changes: 15 additions & 13 deletions src/lib/metrics.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand All @@ -25,7 +27,7 @@ let eventStore: IEventStore;
let environmentStore: IEnvironmentStore;
let statsService: InstanceStatsService;
let stores: IUnleashStores;
beforeAll(() => {
beforeAll(async () => {
const config = createTestConfig({
server: {
serverMetrics: true,
Expand All @@ -49,6 +51,14 @@ beforeAll(() => {
createFakeGetProductionChanges(),
);

const schedulerService = new SchedulerService(
noLogger,
{
isMaintenanceMode: () => Promise.resolve(false),
},
eventBus,
);

const db = {
client: {
pool: {
Expand All @@ -61,20 +71,18 @@ 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();
});

test('should collect metrics for requests', async () => {
eventBus.emit(REQUEST_TIME, {
Expand Down Expand Up @@ -160,17 +168,11 @@ 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);
});
const metrics = await prometheusRegister.metrics();
expect(metrics).toMatch(/client_apps_total\{range="(.*)"\} 0/);
});
Expand Down
47 changes: 20 additions & 27 deletions src/lib/metrics.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -361,13 +356,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,
Expand Down Expand Up @@ -548,17 +542,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',
Expand Down Expand Up @@ -594,12 +587,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();
}
}

Expand Down
2 changes: 1 addition & 1 deletion src/lib/server-impl.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@ async function createApp(
await stopServer();
}
services.schedulerService.stop();
metricsMonitor.stopMonitoring();
services.addonService.destroy();
await db.destroy();
};
Expand All @@ -77,6 +76,7 @@ async function createApp(
serverVersion,
config.eventBus,
services.instanceStatsService,
services.schedulerService,
db,
);
const unleash: Omit<IUnleash, 'stop'> = {
Expand Down

0 comments on commit 49622f1

Please sign in to comment.