From 1e991afc14488c4b3d3b1276b3f7a564c6cb5289 Mon Sep 17 00:00:00 2001 From: zhangxin511 Date: Thu, 3 Aug 2023 14:27:17 -0400 Subject: [PATCH] Hold a reference to collection instead of create one for each request (#16688) ## Description For tenant manager in riddler and get delta in alfred, we resolve collections for every request. The metric for db request is collected periodically and using object created on each request would not collect data properly. Co-authored-by: Xin Zhang --- .../src/alfred/runnerFactory.ts | 2 +- .../src/alfred/services/deltaService.ts | 8 ++-- .../routerlicious-base/src/riddler/api.ts | 10 ++--- .../routerlicious-base/src/riddler/app.ts | 9 ++-- .../routerlicious-base/src/riddler/runner.ts | 9 ++-- .../src/riddler/runnerFactory.ts | 6 ++- .../src/riddler/tenantManager.ts | 45 ++++++------------- .../src/test/alfred/api.spec.ts | 6 ++- .../src/test/riddler/api.spec.ts | 12 +++-- 9 files changed, 46 insertions(+), 61 deletions(-) diff --git a/server/routerlicious/packages/routerlicious-base/src/alfred/runnerFactory.ts b/server/routerlicious/packages/routerlicious-base/src/alfred/runnerFactory.ts index f6ea29fbea54..68c06cbd4c5b 100644 --- a/server/routerlicious/packages/routerlicious-base/src/alfred/runnerFactory.ts +++ b/server/routerlicious/packages/routerlicious-base/src/alfred/runnerFactory.ts @@ -540,7 +540,7 @@ export class AlfredResourcesFactory implements core.IResourcesFactory, protected readonly tenantManager: ITenantManager, ) {} @@ -68,9 +68,7 @@ export class DeltaService implements IDeltaService { query: any, sort: any, ): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(collectionName); - const dbDeltas = await collection.find(query, sort); + const dbDeltas = await this.deltasCollection.find(query, sort); return dbDeltas.map((delta) => delta.operation); } diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts index 7a1150b2be35..a20bec98f2a8 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts @@ -5,21 +5,20 @@ import { getRandomName } from "@fluidframework/server-services-client"; import { - MongoManager, ISecretManager, ITenantStorage, ITenantOrderer, ITenantCustomData, ICache, + ICollection, } from "@fluidframework/server-services-core"; import { handleResponse } from "@fluidframework/server-services"; import { Router } from "express"; import { getParam } from "@fluidframework/server-services-utils"; -import { TenantManager } from "./tenantManager"; +import { ITenantDocument, TenantManager } from "./tenantManager"; export function create( - collectionName: string, - mongoManager: MongoManager, + tenantsCollection: ICollection, baseOrderUrl: string, defaultHistorianUrl: string, defaultInternalHistorianUrl: string, @@ -30,8 +29,7 @@ export function create( ): Router { const router: Router = Router(); const manager = new TenantManager( - mongoManager, - collectionName, + tenantsCollection, baseOrderUrl, defaultHistorianUrl, defaultInternalHistorianUrl, diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts index 93670d409265..b18823c62df0 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts @@ -3,7 +3,7 @@ * Licensed under the MIT License. */ -import { MongoManager, ISecretManager, ICache } from "@fluidframework/server-services-core"; +import { ISecretManager, ICache, ICollection } from "@fluidframework/server-services-core"; import { BaseTelemetryProperties } from "@fluidframework/server-services-telemetry"; import * as bodyParser from "body-parser"; import express from "express"; @@ -14,10 +14,10 @@ import { } from "@fluidframework/server-services-utils"; import { catch404, getTenantIdFromRequest, handleError } from "../utils"; import * as api from "./api"; +import { ITenantDocument } from "./tenantManager"; export function create( - collectionName: string, - mongoManager: MongoManager, + tenantsCollection: ICollection, loggerFormat: string, baseOrdererUrl: string, defaultHistorianUrl: string, @@ -52,8 +52,7 @@ export function create( app.use( "/api", api.create( - collectionName, - mongoManager, + tenantsCollection, baseOrdererUrl, defaultHistorianUrl, defaultInternalHistorianUrl, diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts index 25a5dc9414c3..0ad24caed847 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts @@ -5,16 +5,17 @@ import { Deferred } from "@fluidframework/common-utils"; import { - MongoManager, IRunner, ISecretManager, IWebServerFactory, IWebServer, ICache, + ICollection, } from "@fluidframework/server-services-core"; import { Lumberjack } from "@fluidframework/server-services-telemetry"; import * as winston from "winston"; import * as app from "./app"; +import { ITenantDocument } from "./tenantManager"; export class RiddlerRunner implements IRunner { private server: IWebServer; @@ -22,9 +23,8 @@ export class RiddlerRunner implements IRunner { constructor( private readonly serverFactory: IWebServerFactory, - private readonly collectionName: string, + private readonly tenantsCollection: ICollection, private readonly port: string | number, - private readonly mongoManager: MongoManager, private readonly loggerFormat: string, private readonly baseOrdererUrl: string, private readonly defaultHistorianUrl: string, @@ -41,8 +41,7 @@ export class RiddlerRunner implements IRunner { // Create the HTTP server and attach alfred to it const riddler = app.create( - this.collectionName, - this.mongoManager, + this.tenantsCollection, this.loggerFormat, this.baseOrdererUrl, this.defaultHistorianUrl, diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts index 7b927b588515..39f344c5ca86 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts @@ -15,6 +15,7 @@ import { IRunner, IRunnerFactory, IWebServerFactory, + ICollection, } from "@fluidframework/server-services-core"; import * as utils from "@fluidframework/server-services-utils"; import { Provider } from "nconf"; @@ -29,6 +30,7 @@ export class RiddlerResources implements IResources { constructor( public readonly config: Provider, + public readonly tenantsCollection: ICollection, public readonly tenantsCollectionName: string, public readonly mongoManager: MongoManager, public readonly port: any, @@ -147,6 +149,7 @@ export class RiddlerResourcesFactory implements IResourcesFactory { public async create(resources: RiddlerResources): Promise { return new RiddlerRunner( resources.webServerFactory, - resources.tenantsCollectionName, + resources.tenantsCollection, resources.port, - resources.mongoManager, resources.loggerFormat, resources.baseOrdererUrl, resources.defaultHistorianUrl, diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts index 1cf9a8fcffb6..d0e7e4ef4827 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts @@ -13,9 +13,9 @@ import { ITenantOrderer, ITenantStorage, KeyName, - MongoManager, ISecretManager, ICache, + ICollection, } from "@fluidframework/server-services-core"; import { isNetworkError, NetworkError } from "@fluidframework/server-services-client"; import { BaseTelemetryProperties, Lumberjack } from "@fluidframework/server-services-telemetry"; @@ -84,8 +84,7 @@ export class TenantManager { Object.values(StorageRequestMetric), ); constructor( - private readonly mongoManager: MongoManager, - private readonly collectionName: string, + private readonly tenantsCollection: ICollection, private readonly baseOrdererUrl: string, private readonly defaultHistorianUrl: string, private readonly defaultInternalHistorianUrl: string, @@ -288,8 +287,6 @@ export class TenantManager { orderer: ITenantOrderer, customData: ITenantCustomData, ): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); const latestKeyVersion = this.secretManager.getLatestKeyVersion(); const tenantKey1 = this.generateTenantKey(); @@ -318,7 +315,7 @@ export class TenantManager { } const id = await this.runWithDatabaseRequestCounter(async () => - collection.insertOne({ + this.tenantsCollection.insertOne({ _id: tenantId, key: encryptedTenantKey1, secondaryKey: encryptedTenantKey2, @@ -337,11 +334,8 @@ export class TenantManager { * Updates the tenant configured storage provider */ public async updateStorage(tenantId: string, storage: ITenantStorage): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); - await this.runWithDatabaseRequestCounter(async () => - collection.update({ _id: tenantId }, { storage }, null), + this.tenantsCollection.update({ _id: tenantId }, { storage }, null), ); return (await this.getTenantDocument(tenantId)).storage; @@ -351,10 +345,7 @@ export class TenantManager { * Updates the tenant configured orderer */ public async updateOrderer(tenantId: string, orderer: ITenantOrderer): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); - - await collection.update({ _id: tenantId }, { orderer }, null); + await this.tenantsCollection.update({ _id: tenantId }, { orderer }, null); return (await this.getTenantDocument(tenantId)).orderer; } @@ -366,14 +357,12 @@ export class TenantManager { tenantId: string, customData: ITenantCustomData, ): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); const accessInfo = customData.externalStorageData?.accessInfo; if (accessInfo) { customData.externalStorageData.accessInfo = this.encryptAccessInfo(accessInfo); } await this.runWithDatabaseRequestCounter(async () => - collection.update({ _id: tenantId }, { customData }, null), + this.tenantsCollection.update({ _id: tenantId }, { customData }, null), ); const tenantDocument = await this.getTenantDocument(tenantId, true); if (tenantDocument.disabled) { @@ -534,10 +523,8 @@ export class TenantManager { keyName === KeyName.key2 ? { secondaryKey: encryptedNewTenantKey } : { key: encryptedNewTenantKey }; - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); await this.runWithDatabaseRequestCounter(async () => - collection.update({ _id: tenantId }, updateKey, null), + this.tenantsCollection.update({ _id: tenantId }, updateKey, null), ); return tenantKeys; @@ -662,11 +649,8 @@ export class TenantManager { tenantId: string, includeDisabledTenant = false, ): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); - const found = await this.runWithDatabaseRequestCounter(async () => - collection.findOne({ _id: tenantId }), + this.tenantsCollection.findOne({ _id: tenantId }), ); if (!found || (found.disabled && !includeDisabledTenant)) { return null; @@ -681,10 +665,9 @@ export class TenantManager { * Retrieves all the raw database tenant documents */ private async getAllTenantDocuments(includeDisabledTenant = false): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); - - const allFound = await this.runWithDatabaseRequestCounter(async () => collection.findAll()); + const allFound = await this.runWithDatabaseRequestCounter(async () => + this.tenantsCollection.findAll(), + ); allFound.forEach((found) => { this.attachDefaultsToTenantDocument(found); @@ -700,8 +683,6 @@ export class TenantManager { * If no scheduledDeletionTime is provided the tenant is only soft-deleted. */ public async deleteTenant(tenantId: string, scheduledDeletionTime?: Date): Promise { - const db = await this.mongoManager.getDatabase(); - const collection = db.collection(this.collectionName); const softDelete = !scheduledDeletionTime || scheduledDeletionTime.getTime() > Date.now(); if (softDelete) { const query = { @@ -710,7 +691,7 @@ export class TenantManager { }; await this.runWithDatabaseRequestCounter(async () => - collection.update( + this.tenantsCollection.update( query, { disabled: true, @@ -721,7 +702,7 @@ export class TenantManager { ); } else { await this.runWithDatabaseRequestCounter(async () => - collection.deleteOne({ _id: tenantId }), + this.tenantsCollection.deleteOne({ _id: tenantId }), ); } // invalidate cache diff --git a/server/routerlicious/packages/routerlicious-base/src/test/alfred/api.spec.ts b/server/routerlicious/packages/routerlicious-base/src/test/alfred/api.spec.ts index a0a37a9b6f30..9f12ca1cf281 100644 --- a/server/routerlicious/packages/routerlicious-base/src/test/alfred/api.spec.ts +++ b/server/routerlicious/packages/routerlicious-base/src/test/alfred/api.spec.ts @@ -115,7 +115,11 @@ describe("Routerlicious", () => { scopes, )}`; const defaultProducer = new TestProducer(new TestKafka()); - const defaultDeltaService = new DeltaService(defaultMongoManager, defaultTenantManager); + const deltasCollection = await defaultDbManager.getDeltaCollection( + undefined, + undefined, + ); + const defaultDeltaService = new DeltaService(deltasCollection, defaultTenantManager); const defaultDocumentRepository = new TestNotImplementedDocumentRepository(); const defaultDocumentDeleteService = new DocumentDeleteService(); let app: express.Application; diff --git a/server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts b/server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts index 3e7cbf3100da..a387be87814f 100644 --- a/server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts +++ b/server/routerlicious/packages/routerlicious-base/src/test/riddler/api.spec.ts @@ -15,10 +15,12 @@ import { } from "@fluidframework/server-services-core"; import * as riddlerApp from "../../riddler/app"; import Sinon from "sinon"; +import { ITenantDocument } from "../../riddler"; const documentsCollectionName = "testDocuments"; const deltasCollectionName = "testDeltas"; const rawDeltasCollectionName = "testRawDeltas"; +const testTenantsCollectionName = "test-tenantAdmins"; class TestSecretManager implements ISecretManager { constructor(private readonly encryptionKey: string) {} @@ -38,7 +40,7 @@ class TestSecretManager implements ISecretManager { describe("Routerlicious", () => { describe("Riddler", () => { - describe("API", () => { + describe("API", async () => { const document = { _id: "doc-id", content: "Hello, World!", @@ -47,8 +49,12 @@ describe("Routerlicious", () => { [documentsCollectionName]: [document], [deltasCollectionName]: [], [rawDeltasCollectionName]: [], + [testTenantsCollectionName]: [], }); const defaultMongoManager = new MongoManager(defaultDbFactory); + const defaultDb = await defaultMongoManager.getDatabase(); + const defaultTenantsCollection = + defaultDb.collection(testTenantsCollectionName); const testTenantId = "test-tenant-id"; let app: express.Application; @@ -73,7 +79,6 @@ describe("Routerlicious", () => { }; beforeEach(() => { - const testCollectionName = "test-tenantAdmins"; const testLoggerFormat = "test-logger-format"; const testBaseOrdererUrl = "test-base-orderer-url"; const testExtHistorianUrl = "test-ext-historian-url"; @@ -86,8 +91,7 @@ describe("Routerlicious", () => { const testRiddlerStorageRequestMetricIntervalMs = 60000; app = riddlerApp.create( - testCollectionName, - defaultMongoManager, + defaultTenantsCollection, testLoggerFormat, testBaseOrdererUrl, testExtHistorianUrl,