From 2c826bdbba677928a25881f8fa4fa199eee4c656 Mon Sep 17 00:00:00 2001 From: Tymoteusz Czech <2625371+Tymek@users.noreply.github.com> Date: Mon, 18 Sep 2023 15:05:17 +0200 Subject: [PATCH] feat: Add active users statistics to metrics (#4674) ## About the changes - `getActiveUsers` is using multiple stores, so it is refactored into read-model - Refactored Instance stats service into `features` to co-locate related code Closes https://linear.app/unleash/issue/UNL-230/active-users-prometheus ### Important files `src/lib/features/instance-stats/getActiveUsers.ts` ## Discussion points `getActiveUsers` is coded less _class-based_ then previous similar read-models. In one file instead of 3 (read-model interface, fake read model, sql read model). I find types and functions way more readable, but I'm ready to refactor it to interfaces and classes if consistency is more important. --- src/lib/db/user-store.ts | 25 --- .../instance-stats/getActiveUsers.e2e.test.ts | 154 ++++++++++++++++++ .../features/instance-stats/getActiveUsers.ts | 56 +++++++ .../instance-stats-service.test.ts | 8 +- .../instance-stats}/instance-stats-service.ts | 41 +++-- src/lib/metrics.test.ts | 11 +- src/lib/metrics.ts | 27 ++- .../spec/instance-admin-stats-schema.ts | 7 + src/lib/routes/admin-api/instance-admin.ts | 3 +- src/lib/services/index.ts | 7 +- src/lib/types/services.ts | 2 +- src/lib/types/stores/user-store.ts | 7 - src/test/fixtures/fake-user-store.ts | 21 --- 13 files changed, 289 insertions(+), 80 deletions(-) create mode 100644 src/lib/features/instance-stats/getActiveUsers.e2e.test.ts create mode 100644 src/lib/features/instance-stats/getActiveUsers.ts rename src/lib/{services => features/instance-stats}/instance-stats-service.test.ts (85%) rename src/lib/{services => features/instance-stats}/instance-stats-service.ts (85%) diff --git a/src/lib/db/user-store.ts b/src/lib/db/user-store.ts index b8d33f93b6..6141ecff6a 100644 --- a/src/lib/db/user-store.ts +++ b/src/lib/db/user-store.ts @@ -201,31 +201,6 @@ class UserStore implements IUserStore { .then((res) => Number(res[0].count)); } - async getActiveUsersCount(): Promise<{ - last7: number; - last30: number; - last90: number; - }> { - const result = await this.db.raw( - `SELECT - (SELECT COUNT(*) FROM ${TABLE} WHERE seen_at > NOW() - INTERVAL '1 week') AS last_week, - (SELECT COUNT(*) FROM ${TABLE} WHERE seen_at > NOW() - INTERVAL '1 month') AS last_month, - (SELECT COUNT(*) FROM ${TABLE} WHERE seen_at > NOW() - INTERVAL '3 months') AS last_quarter`, - ); - - const { - last_week: last7, - last_month: last30, - last_quarter: last90, - } = result.rows[0]; - - return { - last7, - last30, - last90, - }; - } - destroy(): void {} async exists(id: number): Promise { diff --git a/src/lib/features/instance-stats/getActiveUsers.e2e.test.ts b/src/lib/features/instance-stats/getActiveUsers.e2e.test.ts new file mode 100644 index 0000000000..28717ff4b6 --- /dev/null +++ b/src/lib/features/instance-stats/getActiveUsers.e2e.test.ts @@ -0,0 +1,154 @@ +import { createGetActiveUsers, type GetActiveUsers } from './getActiveUsers'; +import dbInit, { type ITestDb } from '../../../test/e2e/helpers/database-init'; +import getLogger from '../../../test/fixtures/no-logger'; + +let db: ITestDb; +let getActiveUsers: GetActiveUsers; + +const mockUserDaysAgo = (days: number) => { + const result = new Date(); + result.setDate(result.getDate() - days); + return { + email: `${days}.user@example.com`, + seen_at: result, + }; +}; + +const mockTokenDaysAgo = (userId: number, days: number) => { + const result = new Date(); + result.setDate(result.getDate() - days); + + return { + user_id: userId, + seen_at: result, + secret: 'secret', + expires_at: new Date('2031-12-31'), + }; +}; + +beforeAll(async () => { + db = await dbInit('active_users_serial', getLogger); + getActiveUsers = createGetActiveUsers(db.rawDatabase); +}); + +afterEach(async () => { + await db.rawDatabase('users').delete(); + await db.rawDatabase('personal_access_tokens').delete(); +}); + +afterAll(async () => { + await db.destroy(); +}); + +test('should return 0 users', async () => { + expect(getActiveUsers()).resolves.toEqual({ + last7: 0, + last30: 0, + last60: 0, + last90: 0, + }); +}); + +test('should return 1 user', async () => { + await db.rawDatabase('users').insert(mockUserDaysAgo(1)); + + expect(getActiveUsers()).resolves.toEqual({ + last7: 1, + last30: 1, + last60: 1, + last90: 1, + }); +}); + +test('should handle intervals of activity', async () => { + await db + .rawDatabase('users') + .insert([ + mockUserDaysAgo(5), + mockUserDaysAgo(10), + mockUserDaysAgo(20), + mockUserDaysAgo(40), + mockUserDaysAgo(70), + mockUserDaysAgo(100), + ]); + + expect(getActiveUsers()).resolves.toEqual({ + last7: 1, + last30: 3, + last60: 4, + last90: 5, + }); +}); + +test('should count user as active if they have an active token', async () => { + const users = await db + .rawDatabase('users') + .insert(mockUserDaysAgo(100)) + .returning('id'); + const userId = users[0].id; + await db + .rawDatabase('personal_access_tokens') + .insert(mockTokenDaysAgo(userId, 31)); + + expect(getActiveUsers()).resolves.toEqual({ + last7: 0, + last30: 0, + last60: 1, + last90: 1, + }); +}); + +test('should prioritize user seen_at if newer then token seen_at', async () => { + const users = await db + .rawDatabase('users') + .insert(mockUserDaysAgo(14)) + .returning('id'); + const userId = users[0].id; + await db + .rawDatabase('personal_access_tokens') + .insert([ + mockTokenDaysAgo(userId, 31), + mockTokenDaysAgo(userId, 61), + mockTokenDaysAgo(userId, 91), + ]); + + expect(getActiveUsers()).resolves.toEqual({ + last7: 0, + last30: 1, + last60: 1, + last90: 1, + }); +}); + +test('should handle multiple users and with multiple tokens', async () => { + const users = await db + .rawDatabase('users') + .insert([ + mockUserDaysAgo(5), + mockUserDaysAgo(10), + mockUserDaysAgo(20), + mockUserDaysAgo(40), + mockUserDaysAgo(70), + mockUserDaysAgo(100), + ]) + .returning('id'); + + await db + .rawDatabase('personal_access_tokens') + .insert([ + mockTokenDaysAgo(users[0].id, 31), + mockTokenDaysAgo(users[1].id, 61), + mockTokenDaysAgo(users[1].id, 15), + mockTokenDaysAgo(users[1].id, 55), + mockTokenDaysAgo(users[2].id, 4), + mockTokenDaysAgo(users[3].id, 91), + mockTokenDaysAgo(users[4].id, 91), + ]); + + expect(getActiveUsers()).resolves.toEqual({ + last7: 2, + last30: 3, + last60: 4, + last90: 5, + }); +}); diff --git a/src/lib/features/instance-stats/getActiveUsers.ts b/src/lib/features/instance-stats/getActiveUsers.ts new file mode 100644 index 0000000000..2033def06b --- /dev/null +++ b/src/lib/features/instance-stats/getActiveUsers.ts @@ -0,0 +1,56 @@ +import { type Db } from 'lib/server-impl'; + +export type GetActiveUsers = () => Promise<{ + last7: number; + last30: number; + last60: number; + last90: number; +}>; + +export const createGetActiveUsers = + (db: Db): GetActiveUsers => + async () => { + const combinedQuery = db + .select('id as user_id', 'seen_at') + .from('users') + .unionAll( + db.select('user_id', 'seen_at').from('personal_access_tokens'), + ); + + const result = await db + .with('Combined', combinedQuery) + .select({ + last_week: db.raw( + "COUNT(DISTINCT CASE WHEN seen_at > NOW() - INTERVAL '1 week' THEN user_id END)", + ), + last_month: db.raw( + "COUNT(DISTINCT CASE WHEN seen_at > NOW() - INTERVAL '1 month' THEN user_id END)", + ), + last_two_months: db.raw( + "COUNT(DISTINCT CASE WHEN seen_at > NOW() - INTERVAL '2 months' THEN user_id END)", + ), + last_quarter: db.raw( + "COUNT(DISTINCT CASE WHEN seen_at > NOW() - INTERVAL '3 months' THEN user_id END)", + ), + }) + .from('Combined'); + + return { + last7: parseInt(result?.[0]?.last_week || '0', 10), + last30: parseInt(result?.[0]?.last_month || '0', 10), + last60: parseInt(result?.[0]?.last_two_months || '0', 10), + last90: parseInt(result?.[0]?.last_quarter || '0', 10), + }; + }; + +export const createFakeGetActiveUsers = + ( + activeUsers: Awaited> = { + last7: 0, + last30: 0, + last60: 0, + last90: 0, + }, + ): GetActiveUsers => + () => + Promise.resolve(activeUsers); diff --git a/src/lib/services/instance-stats-service.test.ts b/src/lib/features/instance-stats/instance-stats-service.test.ts similarity index 85% rename from src/lib/services/instance-stats-service.test.ts rename to src/lib/features/instance-stats/instance-stats-service.test.ts index a542d43c04..69532cc0e2 100644 --- a/src/lib/services/instance-stats-service.test.ts +++ b/src/lib/features/instance-stats/instance-stats-service.test.ts @@ -1,7 +1,8 @@ -import { createTestConfig } from '../../test/config/test-config'; +import { createTestConfig } from '../../../test/config/test-config'; import { InstanceStatsService } from './instance-stats-service'; -import createStores from '../../test/fixtures/store'; -import VersionService from './version-service'; +import createStores from '../../../test/fixtures/store'; +import VersionService from '../../services/version-service'; +import { createFakeGetActiveUsers } from './getActiveUsers'; let instanceStatsService: InstanceStatsService; let versionService: VersionService; @@ -14,6 +15,7 @@ beforeEach(() => { stores, config, versionService, + createFakeGetActiveUsers(), ); jest.spyOn(instanceStatsService, 'refreshStatsSnapshot'); diff --git a/src/lib/services/instance-stats-service.ts b/src/lib/features/instance-stats/instance-stats-service.ts similarity index 85% rename from src/lib/services/instance-stats-service.ts rename to src/lib/features/instance-stats/instance-stats-service.ts index 19fa933c8d..901eb1604b 100644 --- a/src/lib/services/instance-stats-service.ts +++ b/src/lib/features/instance-stats/instance-stats-service.ts @@ -1,24 +1,25 @@ import { sha256 } from 'js-sha256'; -import { Logger } from '../logger'; -import { IUnleashConfig } from '../types/option'; +import { Logger } from '../../logger'; +import { IUnleashConfig } from '../../types/option'; import { IClientInstanceStore, IEventStore, IUnleashStores, -} from '../types/stores'; -import { IContextFieldStore } from '../types/stores/context-field-store'; -import { IEnvironmentStore } from '../types/stores/environment-store'; -import { IFeatureToggleStore } from '../types/stores/feature-toggle-store'; -import { IGroupStore } from '../types/stores/group-store'; -import { IProjectStore } from '../types/stores/project-store'; -import { IStrategyStore } from '../types/stores/strategy-store'; -import { IActiveUsers, IUserStore } from '../types/stores/user-store'; -import { ISegmentStore } from '../types/stores/segment-store'; -import { IRoleStore } from '../types/stores/role-store'; -import VersionService from './version-service'; -import { ISettingStore } from '../types/stores/settings-store'; -import { FEATURES_EXPORTED, FEATURES_IMPORTED } from '../types'; -import { CUSTOM_ROOT_ROLE_TYPE } from '../util'; +} from '../../types/stores'; +import { IContextFieldStore } from '../../types/stores/context-field-store'; +import { IEnvironmentStore } from '../../types/stores/environment-store'; +import { IFeatureToggleStore } from '../../types/stores/feature-toggle-store'; +import { IGroupStore } from '../../types/stores/group-store'; +import { IProjectStore } from '../../types/stores/project-store'; +import { IStrategyStore } from '../../types/stores/strategy-store'; +import { IUserStore } from '../../types/stores/user-store'; +import { ISegmentStore } from '../../types/stores/segment-store'; +import { IRoleStore } from '../../types/stores/role-store'; +import VersionService from '../../services/version-service'; +import { ISettingStore } from '../../types/stores/settings-store'; +import { FEATURES_EXPORTED, FEATURES_IMPORTED } from '../../types'; +import { CUSTOM_ROOT_ROLE_TYPE } from '../../util'; +import { type GetActiveUsers } from './getActiveUsers'; export type TimeRange = 'allTime' | '30d' | '7d'; @@ -43,7 +44,7 @@ export interface InstanceStats { SAMLenabled: boolean; OIDCenabled: boolean; clientApps: { range: TimeRange; count: number }[]; - activeUsers: IActiveUsers; + activeUsers: Awaited>; } export interface InstanceStatsSigned extends InstanceStats { @@ -83,6 +84,8 @@ export class InstanceStatsService { private appCount?: Partial<{ [key in TimeRange]: number }>; + private getActiveUsers: GetActiveUsers; + constructor( { featureToggleStore, @@ -114,6 +117,7 @@ export class InstanceStatsService { >, { getLogger }: Pick, versionService: VersionService, + getActiveUsers: GetActiveUsers, ) { this.strategyStore = strategyStore; this.userStore = userStore; @@ -129,6 +133,7 @@ export class InstanceStatsService { this.eventStore = eventStore; this.clientInstanceStore = clientInstanceStore; this.logger = getLogger('services/stats-service.js'); + this.getActiveUsers = getActiveUsers; } async refreshStatsSnapshot(): Promise { @@ -195,7 +200,7 @@ export class InstanceStatsService { ] = await Promise.all([ this.getToggleCount(), this.userStore.count(), - this.userStore.getActiveUsersCount(), + this.getActiveUsers(), this.projectStore.count(), this.contextFieldStore.count(), this.groupStore.count(), diff --git a/src/lib/metrics.test.ts b/src/lib/metrics.test.ts index 51d4d26f6f..1d1daeede2 100644 --- a/src/lib/metrics.test.ts +++ b/src/lib/metrics.test.ts @@ -10,8 +10,9 @@ import { } from './types/events'; import { createMetricsMonitor } from './metrics'; import createStores from '../test/fixtures/store'; -import { InstanceStatsService } from './services/instance-stats-service'; +import { InstanceStatsService } from './features/instance-stats/instance-stats-service'; import VersionService from './services/version-service'; +import { createFakeGetActiveUsers } from './features/instance-stats/getActiveUsers'; const monitor = createMetricsMonitor(); const eventBus = new EventEmitter(); @@ -28,7 +29,13 @@ beforeAll(() => { stores = createStores(); eventStore = stores.eventStore; const versionService = new VersionService(stores, config); - statsService = new InstanceStatsService(stores, config, versionService); + statsService = new InstanceStatsService( + stores, + config, + versionService, + createFakeGetActiveUsers(), + ); + const db = { client: { pool: { diff --git a/src/lib/metrics.ts b/src/lib/metrics.ts index ee8ee790e9..18c8824ffd 100644 --- a/src/lib/metrics.ts +++ b/src/lib/metrics.ts @@ -22,7 +22,7 @@ import { IUnleashConfig } from './types/option'; import { IUnleashStores } from './types/stores'; import { hoursToMilliseconds, minutesToMilliseconds } from 'date-fns'; import Timer = NodeJS.Timer; -import { InstanceStatsService } from './services/instance-stats-service'; +import { InstanceStatsService } from './features/instance-stats/instance-stats-service'; import { ValidatedClientMetrics } from './services/client-metrics/schema'; export default class MetricsMonitor { @@ -86,6 +86,22 @@ export default class MetricsMonitor { name: 'users_total', help: 'Number of users', }); + const usersActive7days = new client.Gauge({ + name: 'users_active_7', + help: 'Number of users active in the last 7 days', + }); + const usersActive30days = new client.Gauge({ + name: 'users_active_30', + help: 'Number of users active in the last 30 days', + }); + const usersActive60days = new client.Gauge({ + name: 'users_active_60', + help: 'Number of users active in the last 60 days', + }); + const usersActive90days = new client.Gauge({ + name: 'users_active_90', + help: 'Number of users active in the last 90 days', + }); const projectsTotal = new client.Gauge({ name: 'projects_total', help: 'Number of projects', @@ -167,6 +183,15 @@ export default class MetricsMonitor { usersTotal.reset(); usersTotal.set(stats.users); + usersActive7days.reset(); + usersActive7days.set(stats.activeUsers.last7); + usersActive30days.reset(); + usersActive30days.set(stats.activeUsers.last30); + usersActive60days.reset(); + usersActive60days.set(stats.activeUsers.last60); + usersActive90days.reset(); + usersActive90days.set(stats.activeUsers.last90); + projectsTotal.reset(); projectsTotal.set(stats.projects); diff --git a/src/lib/openapi/spec/instance-admin-stats-schema.ts b/src/lib/openapi/spec/instance-admin-stats-schema.ts index 1296d0f053..5928acb322 100644 --- a/src/lib/openapi/spec/instance-admin-stats-schema.ts +++ b/src/lib/openapi/spec/instance-admin-stats-schema.ts @@ -58,6 +58,13 @@ export const instanceAdminStatsSchema = { example: 10, minimum: 0, }, + last60: { + type: 'number', + description: + 'The number of active users in the last 60 days', + example: 12, + minimum: 0, + }, last90: { type: 'number', description: diff --git a/src/lib/routes/admin-api/instance-admin.ts b/src/lib/routes/admin-api/instance-admin.ts index e9f1117662..02610a3e69 100644 --- a/src/lib/routes/admin-api/instance-admin.ts +++ b/src/lib/routes/admin-api/instance-admin.ts @@ -10,7 +10,7 @@ import { InstanceStats, InstanceStatsService, InstanceStatsSigned, -} from '../../services/instance-stats-service'; +} from '../../features/instance-stats/instance-stats-service'; import { OpenApiService } from '../../services/openapi-service'; import { createCsvResponseSchema, @@ -110,6 +110,7 @@ class InstanceAdminController extends Controller { versionOSS: '5.1.7', activeUsers: { last90: 15, + last60: 12, last30: 10, last7: 5, }, diff --git a/src/lib/services/index.ts b/src/lib/services/index.ts index 3205009c8c..bbb483741b 100644 --- a/src/lib/services/index.ts +++ b/src/lib/services/index.ts @@ -36,7 +36,7 @@ import EdgeService from './edge-service'; import PatService from './pat-service'; import { PublicSignupTokenService } from './public-signup-token-service'; import { LastSeenService } from './client-metrics/last-seen-service'; -import { InstanceStatsService } from './instance-stats-service'; +import { InstanceStatsService } from '../features/instance-stats/instance-stats-service'; import { FavoritesService } from './favorites-service'; import MaintenanceService from './maintenance-service'; import { @@ -64,6 +64,10 @@ import { createFakePrivateProjectChecker, createPrivateProjectChecker, } from '../features/private-project/createPrivateProjectChecker'; +import { + createFakeGetActiveUsers, + createGetActiveUsers, +} from '../features/instance-stats/getActiveUsers'; // TODO: will be moved to scheduler feature directory export const scheduleServices = async ( @@ -262,6 +266,7 @@ export const createServices = ( stores, config, versionService, + db ? createGetActiveUsers(db) : createFakeGetActiveUsers(), ); const schedulerService = new SchedulerService(config.getLogger); diff --git a/src/lib/types/services.ts b/src/lib/types/services.ts index 047c1d9938..b9e1284e08 100644 --- a/src/lib/types/services.ts +++ b/src/lib/types/services.ts @@ -33,7 +33,7 @@ import EdgeService from '../services/edge-service'; import PatService from '../services/pat-service'; import { PublicSignupTokenService } from '../services/public-signup-token-service'; import { LastSeenService } from '../services/client-metrics/last-seen-service'; -import { InstanceStatsService } from '../services/instance-stats-service'; +import { InstanceStatsService } from '../features/instance-stats/instance-stats-service'; import { FavoritesService } from '../services/favorites-service'; import MaintenanceService from '../services/maintenance-service'; import { AccountService } from '../services/account-service'; diff --git a/src/lib/types/stores/user-store.ts b/src/lib/types/stores/user-store.ts index 23de31bb49..32b3beaa83 100644 --- a/src/lib/types/stores/user-store.ts +++ b/src/lib/types/stores/user-store.ts @@ -19,12 +19,6 @@ export interface IUserUpdateFields { email?: string; } -export interface IActiveUsers { - last7: number; - last30: number; - last90: number; -} - export interface IUserStore extends Store { update(id: number, fields: IUserUpdateFields): Promise; insert(user: ICreateUser): Promise; @@ -38,5 +32,4 @@ export interface IUserStore extends Store { incLoginAttempts(user: IUser): Promise; successfullyLogin(user: IUser): Promise; count(): Promise; - getActiveUsersCount(): Promise; } diff --git a/src/test/fixtures/fake-user-store.ts b/src/test/fixtures/fake-user-store.ts index 89da421cb1..f7d247472a 100644 --- a/src/test/fixtures/fake-user-store.ts +++ b/src/test/fixtures/fake-user-store.ts @@ -1,6 +1,5 @@ import User, { IUser } from '../../lib/types/user'; import { - IActiveUsers, ICreateUser, IUserLookup, IUserStore, @@ -47,26 +46,6 @@ class UserStoreMock implements IUserStore { return this.data.find((u) => u.id === key); } - async getActiveUsersCount(): Promise { - return Promise.resolve({ - last7: this.data.filter( - (u) => - u.seenAt && - u.seenAt > new Date(Date.now() - 7 * 24 * 60 * 60 * 1000), - ).length, - last30: this.data.filter( - (u) => - u.seenAt && - u.seenAt > new Date(Date.now() - 30 * 24 * 60 * 60 * 1000), - ).length, - last90: this.data.filter( - (u) => - u.seenAt && - u.seenAt > new Date(Date.now() - 90 * 24 * 60 * 60 * 1000), - ).length, - }); - } - async insert(user: User): Promise { // eslint-disable-next-line no-param-reassign user.id = this.idSeq;