From 34e5034547d4afc497dfcc12c7d50797631bee21 Mon Sep 17 00:00:00 2001 From: Christopher Kolstad Date: Mon, 21 Feb 2022 12:46:28 +0100 Subject: [PATCH] fix: reduce project overview query count to 2. (#1356) * fix: reduce project overview query count to 2. Previously we've been doing N+1 queries for projects, this now changes to doing one query for projects with feature counts, and then one query for membercounts for all projects and merging that with the first query. --- package.json | 2 +- src/lib/db/index.ts | 2 +- src/lib/db/project-store.ts | 68 +++++++++++++++++-- src/lib/services/project-service.ts | 19 +----- src/lib/types/stores/project-store.ts | 3 +- .../api/admin/project/features.e2e.test.ts | 2 +- src/test/e2e/api/client/feature.e2e.test.ts | 2 +- src/test/fixtures/fake-project-store.ts | 8 ++- yarn.lock | 20 +++++- 9 files changed, 96 insertions(+), 30 deletions(-) diff --git a/package.json b/package.json index 82f95f0e5a..67033a5650 100644 --- a/package.json +++ b/package.json @@ -104,7 +104,7 @@ "nodemailer": "^6.5.0", "owasp-password-strength-test": "^1.3.0", "parse-database-url": "^0.3.0", - "pg": "^8.7.1", + "pg": "^8.7.3", "pg-connection-string": "^2.5.0", "pkginfo": "^0.4.1", "prom-client": "^14.0.0", diff --git a/src/lib/db/index.ts b/src/lib/db/index.ts index 04dab0866f..8d67f87a93 100644 --- a/src/lib/db/index.ts +++ b/src/lib/db/index.ts @@ -51,7 +51,7 @@ export const createStores = ( contextFieldStore: new ContextFieldStore(db, getLogger), settingStore: new SettingStore(db, getLogger), userStore: new UserStore(db, getLogger), - projectStore: new ProjectStore(db, getLogger), + projectStore: new ProjectStore(db, eventBus, getLogger), tagStore: new TagStore(db, eventBus, getLogger), tagTypeStore: new TagTypeStore(db, eventBus, getLogger), addonStore: new AddonStore(db, eventBus, getLogger), diff --git a/src/lib/db/project-store.ts b/src/lib/db/project-store.ts index 44b622a156..ee351ee8a6 100644 --- a/src/lib/db/project-store.ts +++ b/src/lib/db/project-store.ts @@ -2,7 +2,7 @@ import { Knex } from 'knex'; import { Logger, LogProvider } from '../logger'; import NotFoundError from '../error/notfound-error'; -import { IProject } from '../types/model'; +import { IProject, IProjectWithCount } from '../types/model'; import { IProjectHealthUpdate, IProjectInsert, @@ -10,6 +10,9 @@ import { IProjectStore, } from '../types/stores/project-store'; import { DEFAULT_ENV } from '../util/constants'; +import metricsHelper from '../util/metrics-helper'; +import { DB_TIME } from '../metric-events'; +import EventEmitter from 'events'; const COLUMNS = [ 'id', @@ -26,9 +29,16 @@ class ProjectStore implements IProjectStore { private logger: Logger; - constructor(db: Knex, getLogger: LogProvider) { + private timer: Function; + + constructor(db: Knex, eventBus: EventEmitter, getLogger: LogProvider) { this.db = db; this.logger = getLogger('project-store.ts'); + this.timer = (action) => + metricsHelper.wrapTimer(eventBus, DB_TIME, { + store: 'project', + action, + }); } // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types @@ -44,13 +54,61 @@ class ProjectStore implements IProjectStore { async exists(id: string): Promise { const result = await this.db.raw( - `SELECT EXISTS (SELECT 1 FROM ${TABLE} WHERE id = ?) AS present`, + `SELECT EXISTS(SELECT 1 FROM ${TABLE} WHERE id = ?) AS present`, [id], ); const { present } = result.rows[0]; return present; } + async getProjectsWithCounts( + query?: IProjectQuery, + ): Promise { + const projectTimer = this.timer('getProjectsWithCount'); + let projects = this.db(TABLE) + .select( + this.db.raw( + 'projects.id, projects.name, projects.description, projects.health, projects.updated_at, count(features.name) AS number_of_features', + ), + ) + .leftJoin('features', 'features.project', 'projects.id') + .groupBy('projects.id'); + if (query) { + projects = projects.where(query); + } + const projectAndFeatureCount = await projects; + + // @ts-ignore + const projectsWithFeatureCount = projectAndFeatureCount.map( + this.mapProjectWithCountRow, + ); + projectTimer(); + const memberTimer = this.timer('getMemberCount'); + const memberCount = await this.db.raw( + `SELECT count(role_id) as member_count, project FROM role_user GROUP BY project`, + ); + memberTimer(); + const memberMap = new Map( + memberCount.rows.map((c) => [c.project, Number(c.member_count)]), + ); + return projectsWithFeatureCount.map((r) => { + return { ...r, memberCount: memberMap.get(r.id) }; + }); + } + + // eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types + mapProjectWithCountRow(row): IProjectWithCount { + return { + name: row.name, + id: row.id, + description: row.description, + health: row.health, + featureCount: row.number_of_features, + memberCount: row.number_of_users || 0, + updatedAt: row.updated_at, + }; + } + async getAll(query: IProjectQuery = {}): Promise { const rows = await this.db .select(COLUMNS) @@ -71,7 +129,7 @@ class ProjectStore implements IProjectStore { async hasProject(id: string): Promise { const result = await this.db.raw( - `SELECT EXISTS (SELECT 1 FROM ${TABLE} WHERE id = ?) AS present`, + `SELECT EXISTS(SELECT 1 FROM ${TABLE} WHERE id = ?) AS present`, [id], ); const { present } = result.rows[0]; @@ -208,5 +266,5 @@ class ProjectStore implements IProjectStore { }; } } + export default ProjectStore; -module.exports = ProjectStore; diff --git a/src/lib/services/project-service.ts b/src/lib/services/project-service.ts index 78aa7ccffe..01605f1e61 100644 --- a/src/lib/services/project-service.ts +++ b/src/lib/services/project-service.ts @@ -100,24 +100,7 @@ export default class ProjectService { } async getProjects(query?: IProjectQuery): Promise { - const projects = await this.store.getAll(query); - const projectsWithCount = await Promise.all( - projects.map(async (p) => { - let featureCount = 0; - let memberCount = 0; - try { - featureCount = - await this.featureToggleService.getFeatureCountForProject( - p.id, - ); - memberCount = await this.getMembers(p.id); - } catch (e) { - this.logger.warn('Error fetching project counts', e); - } - return { ...p, featureCount, memberCount }; - }), - ); - return projectsWithCount; + return this.store.getProjectsWithCounts(query); } async getProject(id: string): Promise { diff --git a/src/lib/types/stores/project-store.ts b/src/lib/types/stores/project-store.ts index 9d0448e066..2e4c41d8b5 100644 --- a/src/lib/types/stores/project-store.ts +++ b/src/lib/types/stores/project-store.ts @@ -1,4 +1,4 @@ -import { IProject } from '../model'; +import { IProject, IProjectWithCount } from '../model'; import { Store } from './store'; export interface IProjectInsert { @@ -32,6 +32,7 @@ export interface IProjectStore extends Store { deleteEnvironmentForProject(id: string, environment: string): Promise; getEnvironmentsForProject(id: string): Promise; getMembers(projectId: string): Promise; + getProjectsWithCounts(query?: IProjectQuery): Promise; count(): Promise; getAll(query?: IProjectQuery): Promise; } diff --git a/src/test/e2e/api/admin/project/features.e2e.test.ts b/src/test/e2e/api/admin/project/features.e2e.test.ts index d350182bb6..ab7bb3fd70 100644 --- a/src/test/e2e/api/admin/project/features.e2e.test.ts +++ b/src/test/e2e/api/admin/project/features.e2e.test.ts @@ -2039,7 +2039,7 @@ test('Can update impression data with PUT', async () => { }); test('Can create toggle with impression data on different project', async () => { - db.stores.projectStore.create({ + await db.stores.projectStore.create({ id: 'impression-data', name: 'ImpressionData', description: '', diff --git a/src/test/e2e/api/client/feature.e2e.test.ts b/src/test/e2e/api/client/feature.e2e.test.ts index a37178f54f..69dd13290d 100644 --- a/src/test/e2e/api/client/feature.e2e.test.ts +++ b/src/test/e2e/api/client/feature.e2e.test.ts @@ -282,7 +282,7 @@ test('returns a feature toggles impression data for a different project', async description: '', }; - db.stores.projectStore.create(project); + await db.stores.projectStore.create(project); const toggle = { name: 'project-client.impression.data', diff --git a/src/test/fixtures/fake-project-store.ts b/src/test/fixtures/fake-project-store.ts index 5a08f275a2..2494b508e2 100644 --- a/src/test/fixtures/fake-project-store.ts +++ b/src/test/fixtures/fake-project-store.ts @@ -3,7 +3,7 @@ import { IProjectInsert, IProjectStore, } from '../../lib/types/stores/project-store'; -import { IProject } from '../../lib/types/model'; +import { IProject, IProjectWithCount } from '../../lib/types/model'; import NotFoundError from '../../lib/error/notfound-error'; export default class FakeProjectStore implements IProjectStore { @@ -26,6 +26,12 @@ export default class FakeProjectStore implements IProjectStore { this.projectEnvironment.set(id, environments); } + async getProjectsWithCounts(): Promise { + return this.projects.map((p) => { + return { ...p, memberCount: 0, featureCount: 0 }; + }); + } + private createInternal(project: IProjectInsert): IProject { const newProj: IProject = { ...project, diff --git a/yarn.lock b/yarn.lock index c639354d0a..b4fa629cbf 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5756,6 +5756,11 @@ pg-pool@^3.4.1: resolved "https://registry.npmjs.org/pg-pool/-/pg-pool-3.4.1.tgz" integrity sha512-TVHxR/gf3MeJRvchgNHxsYsTCHQ+4wm3VIHSS19z8NC0+gioEhq1okDY1sm/TYbfoP6JLFx01s0ShvZ3puP/iQ== +pg-pool@^3.5.1: + version "3.5.1" + resolved "https://registry.yarnpkg.com/pg-pool/-/pg-pool-3.5.1.tgz#f499ce76f9bf5097488b3b83b19861f28e4ed905" + integrity sha512-6iCR0wVrro6OOHFsyavV+i6KYL4lVNyYAB9RD18w66xSzN+d8b66HiwuP30Gp1SH5O9T82fckkzsRjlrhD0ioQ== + pg-protocol@^1.5.0: version "1.5.0" resolved "https://registry.npmjs.org/pg-protocol/-/pg-protocol-1.5.0.tgz" @@ -5772,7 +5777,7 @@ pg-types@^2.1.0: postgres-date "~1.0.4" postgres-interval "^1.1.0" -pg@^8.0.3, pg@^8.7.1: +pg@^8.0.3: version "8.7.1" resolved "https://registry.npmjs.org/pg/-/pg-8.7.1.tgz" integrity sha512-7bdYcv7V6U3KAtWjpQJJBww0UEsWuh4yQ/EjNf2HeO/NnvKjpvhEIe/A/TleP6wtmSKnUnghs5A9jUoK6iDdkA== @@ -5785,6 +5790,19 @@ pg@^8.0.3, pg@^8.7.1: pg-types "^2.1.0" pgpass "1.x" +pg@^8.7.3: + version "8.7.3" + resolved "https://registry.yarnpkg.com/pg/-/pg-8.7.3.tgz#8a5bdd664ca4fda4db7997ec634c6e5455b27c44" + integrity sha512-HPmH4GH4H3AOprDJOazoIcpI49XFsHCe8xlrjHkWiapdbHK+HLtbm/GQzXYAZwmPju/kzKhjaSfMACG+8cgJcw== + dependencies: + buffer-writer "2.0.0" + packet-reader "1.0.0" + pg-connection-string "^2.5.0" + pg-pool "^3.5.1" + pg-protocol "^1.5.0" + pg-types "^2.1.0" + pgpass "1.x" + pgpass@1.x: version "1.0.4" resolved "https://registry.npmjs.org/pgpass/-/pgpass-1.0.4.tgz"