From 7c774b22e882339aaabb38895d81028ce38bfb6f Mon Sep 17 00:00:00 2001 From: Thomas Heartman Date: Tue, 20 Aug 2024 11:31:45 +0200 Subject: [PATCH] fix: don't count flags multiple times (bonus: don't count non-project events) (#7931) This PR fixes an issue where the number of flags belonging to a project was wrong in the new getProjectsForAdminUi. The cause was that we now join with the events table to get the most "lastUpdatedAt" data. This meant that we got multiple rows for each flag, so we counted the same flag multiple times. The fix was to use a "distinct". Additionally, I used this as an excuse to write some more tests that I'd been thinking about. And in doing so also uncovered another bug that would only ever surface in verrry rare conditions: if a flag had been created in project A, but moved to project B AND the feature-project-change event hadn't fired correctly, project B's last updated could show data from that feature in project A. I've also taken the liberty of doing a little bit of cleanup. --- .../project/project-owners-read-model.test.ts | 2 +- .../project/project-read-model-type.ts | 2 +- .../project/project-read-model.test.ts | 159 ++++++++++++++++++ .../features/project/project-read-model.ts | 14 +- src/lib/types/events.ts | 2 +- 5 files changed, 172 insertions(+), 7 deletions(-) create mode 100644 src/lib/features/project/project-read-model.test.ts diff --git a/src/lib/features/project/project-owners-read-model.test.ts b/src/lib/features/project/project-owners-read-model.test.ts index 139d2d8ec6..eb6055e571 100644 --- a/src/lib/features/project/project-owners-read-model.test.ts +++ b/src/lib/features/project/project-owners-read-model.test.ts @@ -20,7 +20,7 @@ const mockProjectData = (name: string): ProjectForUi => ({ createdAt: new Date(), favorite: false, lastReportedFlagUsage: null, - lastFlagUpdate: null, + lastUpdatedAt: null, }); describe('unit tests', () => { diff --git a/src/lib/features/project/project-read-model-type.ts b/src/lib/features/project/project-read-model-type.ts index 09301f0326..ff5e31284c 100644 --- a/src/lib/features/project/project-read-model-type.ts +++ b/src/lib/features/project/project-read-model-type.ts @@ -13,7 +13,7 @@ export type ProjectForUi = { archivedAt?: Date; featureCount: number; lastReportedFlagUsage: Date | null; - lastFlagUpdate: Date | null; + lastUpdatedAt: Date | null; }; // @todo remove with flag useProjectReadModel diff --git a/src/lib/features/project/project-read-model.test.ts b/src/lib/features/project/project-read-model.test.ts new file mode 100644 index 0000000000..c58e2af507 --- /dev/null +++ b/src/lib/features/project/project-read-model.test.ts @@ -0,0 +1,159 @@ +import dbInit, { type ITestDb } from '../../../test/e2e/helpers/database-init'; +import getLogger from '../../../test/fixtures/no-logger'; +import type { IFeatureToggleStore } from '../feature-toggle/types/feature-toggle-store-type'; +import type { + IEventStore, + IFlagResolver, + IProjectReadModel, + IProjectStore, +} from '../../types'; +import { ProjectReadModel } from './project-read-model'; +import type EventEmitter from 'events'; + +let db: ITestDb; +let flagStore: IFeatureToggleStore; +let projectStore: IProjectStore; +let eventStore: IEventStore; +let projectReadModel: IProjectReadModel; + +const alwaysOnFlagResolver = { + isEnabled() { + return true; + }, +} as unknown as IFlagResolver; + +beforeAll(async () => { + db = await dbInit('feature_lifecycle_read_model', getLogger); + projectReadModel = new ProjectReadModel( + db.rawDatabase, + { emit: () => {} } as unknown as EventEmitter, + alwaysOnFlagResolver, + ); + projectStore = db.stores.projectStore; + eventStore = db.stores.eventStore; + flagStore = db.stores.featureToggleStore; +}); + +afterAll(async () => { + if (db) { + await db.destroy(); + } +}); + +beforeEach(async () => { + await projectStore.deleteAll(); + await flagStore.deleteAll(); +}); + +test("it doesn't count flags multiple times when they have multiple events associated with them", async () => { + const projectId = 'project'; + const flagName = 'flag'; + await projectStore.create({ id: projectId, name: projectId }); + + await flagStore.create(projectId, { name: flagName, createdByUserId: 1 }); + + await eventStore.store({ + type: 'feature-created', + createdBy: 'admin', + ip: '', + createdByUserId: 1, + featureName: flagName, + project: projectId, + }); + await eventStore.store({ + type: 'feature-updated', + createdBy: 'admin', + ip: '', + createdByUserId: 1, + featureName: flagName, + project: projectId, + }); + + const withFlags = await projectReadModel.getProjectsForAdminUi(); + expect(withFlags).toMatchObject([{ id: projectId, featureCount: 1 }]); + + const insightsQuery = await projectReadModel.getProjectsForInsights(); + expect(insightsQuery).toMatchObject([{ id: projectId, featureCount: 1 }]); +}); + +test('it uses the last flag change for an flag in the project as lastUpdated', async () => { + const projectId = 'project'; + const flagName = 'flag'; + await projectStore.create({ id: projectId, name: projectId }); + await eventStore.store({ + type: 'project-created', + createdBy: 'admin', + createdByUserId: 1, + project: projectId, + ip: '', + }); + + const noEvents = await projectReadModel.getProjectsForAdminUi(); + + expect(noEvents[0].lastUpdatedAt).toBeNull(); + + await flagStore.create(projectId, { name: flagName, createdByUserId: 1 }); + await eventStore.store({ + type: 'feature-created', + createdBy: 'admin', + ip: '', + createdByUserId: 1, + featureName: flagName, + project: projectId, + }); + + const withEvents = await projectReadModel.getProjectsForAdminUi(); + expect(withEvents[0].lastUpdatedAt).not.toBeNull(); +}); + +test('it does not consider flag events in a different project for lastUpdatedAt, and does not count flags belonging to a different project', async () => { + const projectId1 = 'project1'; + await projectStore.create({ id: projectId1, name: 'Project1' }); + + const projectId2 = 'project2'; + await projectStore.create({ id: projectId2, name: 'Project2' }); + + const flagName = 'flag'; + await flagStore.create(projectId1, { name: flagName, createdByUserId: 1 }); + await eventStore.store({ + type: 'feature-created', + createdBy: 'admin', + ip: '', + createdByUserId: 1, + featureName: flagName, + project: projectId2, + }); + + const withEvents = await projectReadModel.getProjectsForAdminUi(); + + expect(withEvents).toMatchObject([ + // no events for the flag in this project (i.e. if a flag + // has been moved from one project to the next (before the + // moving event has been counted)). In practice, you'll + // always get a "feature-project-change" event to count + { id: projectId1, lastUpdatedAt: null }, + // this flag no longer exists in this project because it + // has been moved, so it should not be counted + { id: projectId2, lastUpdatedAt: null }, + ]); +}); + +test('it uses the last flag metrics received for lastReportedFlagUsage', async () => { + const projectId = 'project'; + const flagName = 'flag'; + await projectStore.create({ id: projectId, name: projectId }); + + const noUsage = await projectReadModel.getProjectsForAdminUi(); + expect(noUsage[0].lastReportedFlagUsage).toBeNull(); + + await flagStore.create(projectId, { name: flagName, createdByUserId: 1 }); + + await flagStore.setLastSeen([ + { featureName: flagName, environment: 'development' }, + ]); + + const flag = await flagStore.get(flagName); + + const result = await projectReadModel.getProjectsForAdminUi(); + expect(result[0].lastReportedFlagUsage).toEqual(flag.lastSeenAt); +}); diff --git a/src/lib/features/project/project-read-model.ts b/src/lib/features/project/project-read-model.ts index ceca2b6748..e3b03d7761 100644 --- a/src/lib/features/project/project-read-model.ts +++ b/src/lib/features/project/project-read-model.ts @@ -28,7 +28,7 @@ const mapProjectForUi = (row): ProjectForUi => { archivedAt: row.archived_at, mode: row.project_mode || 'open', lastReportedFlagUsage: row.last_usage, - lastFlagUpdate: row.last_updated, + lastUpdatedAt: row.last_updated, }; }; @@ -74,7 +74,13 @@ export class ProjectReadModel implements IProjectReadModel { 'project_settings.project', 'projects.id', ) - .leftJoin('events', 'events.feature_name', 'features.name') + .leftJoin('events', (join) => { + join.on('events.feature_name', '=', 'features.name').andOn( + 'events.project', + '=', + 'projects.id', + ); + }) .orderBy('projects.name', 'asc'); if (this.flagResolver.isEnabled('archiveProjects')) { @@ -92,7 +98,7 @@ export class ProjectReadModel implements IProjectReadModel { let selectColumns = [ this.db.raw( 'projects.id, projects.name, projects.description, projects.health, projects.created_at, ' + - 'count(features.name) FILTER (WHERE features.archived_at is null) AS number_of_features, ' + + 'count(DISTINCT features.name) FILTER (WHERE features.archived_at is null) AS number_of_features, ' + 'MAX(features.last_seen_at) AS last_usage,' + 'MAX(events.created_at) AS last_updated', ), @@ -169,7 +175,7 @@ export class ProjectReadModel implements IProjectReadModel { 'projects.id, projects.health, ' + 'count(features.name) FILTER (WHERE features.archived_at is null) AS number_of_features, ' + 'count(features.name) FILTER (WHERE features.archived_at is null and features.stale IS TRUE) AS stale_feature_count, ' + - 'count(features.name) FILTER (WHERE features.archived_at is null and features.potentially_stale IS TRUE) AS potentially_stale_feature_count,', + 'count(features.name) FILTER (WHERE features.archived_at is null and features.potentially_stale IS TRUE) AS potentially_stale_feature_count', ), 'project_stats.avg_time_to_prod_current_window', ] as (string | Raw)[]; diff --git a/src/lib/types/events.ts b/src/lib/types/events.ts index 711fa2ac1a..09f83d462f 100644 --- a/src/lib/types/events.ts +++ b/src/lib/types/events.ts @@ -354,7 +354,7 @@ export const IEventTypes = [ ] as const; export type IEventType = (typeof IEventTypes)[number]; -// this rerpresents the write model for events +// this represents the write model for events export interface IBaseEvent { type: IEventType; createdBy: string;