From a9cefdf185a1326761b5ef577e8512792289dab9 Mon Sep 17 00:00:00 2001 From: kekachmar <107499405+kekachmar@users.noreply.github.com> Date: Tue, 18 Jul 2023 16:12:53 -0400 Subject: [PATCH] add api counter for riddler fetchTenantKey metric (#16423) Currently, fetchTenantKeyMetric generates many logs in our service. We decided to reduce the number of logs in riddler by using apiCounter to count these metrics and log the values after a given period of time. --- .../templates/fluid-configmap.yaml | 3 + .../routerlicious-base/src/riddler/api.ts | 2 + .../routerlicious-base/src/riddler/app.ts | 2 + .../routerlicious-base/src/riddler/runner.ts | 2 + .../src/riddler/runnerFactory.ts | 5 ++ .../src/riddler/tenantManager.ts | 69 ++++++++++-------- .../src/test/riddler/api.spec.ts | 8 +++ .../packages/routerlicious/config/config.json | 3 + .../services-utils/src/apiCounters.ts | 72 +++++++++++++++++++ .../packages/services-utils/src/index.ts | 1 + 10 files changed, 138 insertions(+), 29 deletions(-) create mode 100644 server/routerlicious/packages/services-utils/src/apiCounters.ts diff --git a/server/routerlicious/kubernetes/routerlicious/templates/fluid-configmap.yaml b/server/routerlicious/kubernetes/routerlicious/templates/fluid-configmap.yaml index 9caa7c845c40..96550fdc2192 100644 --- a/server/routerlicious/kubernetes/routerlicious/templates/fluid-configmap.yaml +++ b/server/routerlicious/kubernetes/routerlicious/templates/fluid-configmap.yaml @@ -73,6 +73,9 @@ data: "checkpointsTTLInSeconds": {{ .Values.checkpoints.checkpointsTTLInSeconds }}, "kafkaCheckpointOnReprocessingOp": {{ .Values.checkpoints.kafkaCheckpointOnReprocessingOp }} }, + "apiCounters": { + "fetchTenantKeyMetricMs": 60000, + }, "alfred": { "kafkaClientId": "{{ template "alfred.fullname" . }}", "maxMessageSize": "16KB", diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts index 94d40fb58db1..3bcce944b930 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/api.ts @@ -24,6 +24,7 @@ export function create( defaultHistorianUrl: string, defaultInternalHistorianUrl: string, secretManager: ISecretManager, + fetchTenantKeyMetricInterval: number, cache?: ICache, ): Router { const router: Router = Router(); @@ -34,6 +35,7 @@ export function create( defaultHistorianUrl, defaultInternalHistorianUrl, secretManager, + fetchTenantKeyMetricInterval, cache, ); diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts index b782738d4944..7671daa09df0 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/app.ts @@ -23,6 +23,7 @@ export function create( defaultHistorianUrl: string, defaultInternalHistorianUrl: string, secretManager: ISecretManager, + fetchTenantKeyMetricInterval: number, cache?: ICache, ) { // Express app configuration @@ -56,6 +57,7 @@ export function create( defaultHistorianUrl, defaultInternalHistorianUrl, secretManager, + fetchTenantKeyMetricInterval, cache, ), ); diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts index eb30d3b481c7..76512d544067 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/runner.ts @@ -30,6 +30,7 @@ export class RiddlerRunner implements IRunner { private readonly defaultHistorianUrl: string, private readonly defaultInternalHistorianUrl: string, private readonly secretManager: ISecretManager, + private readonly fetchTenantKeyMetricInterval: number, private readonly cache?: ICache, ) {} @@ -46,6 +47,7 @@ export class RiddlerRunner implements IRunner { this.defaultHistorianUrl, this.defaultInternalHistorianUrl, this.secretManager, + this.fetchTenantKeyMetricInterval, this.cache, ); riddler.set("port", this.port); diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts index 8548ee67d2ba..8ea5b8e0cf26 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/runnerFactory.ts @@ -37,6 +37,7 @@ export class RiddlerResources implements IResources { public readonly defaultHistorianUrl: string, public readonly defaultInternalHistorianUrl: string, public readonly secretManager: ISecretManager, + public readonly fetchTenantKeyMetricIntervalMs: number, public readonly cache: RedisCache, ) { const httpServerConfig: services.IHttpServerConfig = config.get("system:httpServer"); @@ -138,6 +139,8 @@ export class RiddlerResourcesFactory implements IResourcesFactory { resources.defaultHistorianUrl, resources.defaultInternalHistorianUrl, resources.secretManager, + resources.fetchTenantKeyMetricIntervalMs, resources.cache, ); } diff --git a/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts b/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts index ad90dbef9f4c..26116edb08ad 100644 --- a/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts +++ b/server/routerlicious/packages/routerlicious-base/src/riddler/tenantManager.ts @@ -18,11 +18,8 @@ import { ICache, } from "@fluidframework/server-services-core"; import { isNetworkError, NetworkError } from "@fluidframework/server-services-client"; -import { - BaseTelemetryProperties, - LumberEventName, - Lumberjack, -} from "@fluidframework/server-services-telemetry"; +import { BaseTelemetryProperties, Lumberjack } from "@fluidframework/server-services-telemetry"; +import { IApiCounters, InMemoryApiCounters } from "@fluidframework/server-services-utils"; import * as jwt from "jsonwebtoken"; import * as _ from "lodash"; import * as winston from "winston"; @@ -58,8 +55,19 @@ export interface ITenantDocument { scheduledDeletionTime?: string; } +enum FetchTenantKeyMetric { + RetrieveFromCacheSucess = "retrieveFromCacheSuccess", + NotFoundInCache = "notFoundInCache", + RetrieveFromCacheError = "retrieveFromCacheError", + SetKeyInCacheSuccess = "settingKeyInCacheSucceeded", + SetKeyInCacheFailure = "settingKeyInCacheFailed", +} + export class TenantManager { private readonly isCacheEnabled; + private readonly apiCounter: IApiCounters = new InMemoryApiCounters( + Object.values(FetchTenantKeyMetric), + ); constructor( private readonly mongoManager: MongoManager, private readonly collectionName: string, @@ -67,9 +75,19 @@ export class TenantManager { private readonly defaultHistorianUrl: string, private readonly defaultInternalHistorianUrl: string, private readonly secretManager: ISecretManager, + private readonly fetchTenantKeyMetricInterval: number, private readonly cache?: ICache, ) { this.isCacheEnabled = this.cache ? true : false; + if (fetchTenantKeyMetricInterval) { + setInterval(() => { + if (!this.apiCounter.countersAreActive) { + return; + } + Lumberjack.info("Fetch tenant key api counters", this.apiCounter.getCounters()); + this.apiCounter.resetAllCounters(); + }, this.fetchTenantKeyMetricInterval); + } } /** @@ -341,11 +359,6 @@ export class TenantManager { includeDisabledTenant, bypassCache, }; - const fetchTenantKeyMetric = Lumberjack.newLumberMetric( - LumberEventName.RiddlerFetchTenantKey, - ); - let uncaughtException; - let retrievedFromCache = false; try { if (!bypassCache && this.isCacheEnabled) { @@ -353,7 +366,6 @@ export class TenantManager { try { const cachedKey = await this.getKeyFromCache(tenantId); if (cachedKey) { - retrievedFromCache = true; const tenantKeys = this.decryptCachedKeys(cachedKey); // This is an edge case where the used encryption key is not valid. // If both decrypted tenant keys are null, it means it hits this case, @@ -426,11 +438,7 @@ export class TenantManager { if (encryptionKeyVersion) { cacheKeys.encryptionKeyVersion = encryptionKeyVersion; } - const setKeyInCacheSucceeded = await this.setKeyInCache(tenantId, cacheKeys); - fetchTenantKeyMetric.setProperty( - "settingKeyInCacheSucceeded", - setKeyInCacheSucceeded, - ); + await this.setKeyInCache(tenantId, cacheKeys); } return { @@ -438,18 +446,8 @@ export class TenantManager { key2: tenantKey2, }; } catch (error) { - uncaughtException = error; + Lumberjack.error(`Error getting tenant keys.`, error); throw error; - } finally { - fetchTenantKeyMetric.setProperty("retrievedFromCache", retrievedFromCache); - if (!uncaughtException) { - fetchTenantKeyMetric.success(`Successfully retrieved tenant keys.`); - } else { - fetchTenantKeyMetric.error( - `Error trying to retrieve tenant keys.`, - uncaughtException, - ); - } } } @@ -708,7 +706,19 @@ export class TenantManager { } private async getKeyFromCache(tenantId: string): Promise { - return this.cache?.get(`tenantKeys:${tenantId}`); + try { + const cachedKey = await this.cache?.get(`tenantKeys:${tenantId}`); + if (cachedKey == null) { + this.apiCounter.incrementCounter(FetchTenantKeyMetric.NotFoundInCache); + } else { + this.apiCounter.incrementCounter(FetchTenantKeyMetric.RetrieveFromCacheSucess); + } + return cachedKey; + } catch (error) { + Lumberjack.error(`Error trying to retreive tenant keys from the cache.`, error); + this.apiCounter.incrementCounter(FetchTenantKeyMetric.RetrieveFromCacheError); + throw error; + } } private async deleteKeyFromCache(tenantId: string): Promise { @@ -719,10 +729,11 @@ export class TenantManager { const lumberProperties = { [BaseTelemetryProperties.tenantId]: tenantId }; try { await this.cache?.set(`tenantKeys:${tenantId}`, JSON.stringify(value)); - Lumberjack.info(`Added tenant keys to cache.`, lumberProperties); + this.apiCounter.incrementCounter(FetchTenantKeyMetric.SetKeyInCacheSuccess); return true; } catch (error) { Lumberjack.error(`Setting tenant key in the cache failed`, lumberProperties, error); + this.apiCounter.incrementCounter(FetchTenantKeyMetric.SetKeyInCacheFailure); return false; } } 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 85332b2f33eb..800717fa70de 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 @@ -14,6 +14,7 @@ import { ISecretManager, } from "@fluidframework/server-services-core"; import * as riddlerApp from "../../riddler/app"; +import Sinon from "sinon"; const documentsCollectionName = "testDocuments"; const deltasCollectionName = "testDeltas"; @@ -80,6 +81,8 @@ describe("Routerlicious", () => { const testSecretManager = new TestSecretManager( crypto.randomBytes(32).toString("base64"), ); + Sinon.useFakeTimers(); + const testFetchTenantKeyMetricIntervalMs = 60000; app = riddlerApp.create( testCollectionName, @@ -89,10 +92,15 @@ describe("Routerlicious", () => { testExtHistorianUrl, testIntHistorianUrl, testSecretManager, + testFetchTenantKeyMetricIntervalMs, ); supertest = request(app); }); + afterEach(() => { + Sinon.restore(); + }); + it("POST /tenants/:id/validate", async () => { await assertCorrelationId(`/api/tenants/${testTenantId}/validate`, "post"); }); diff --git a/server/routerlicious/packages/routerlicious/config/config.json b/server/routerlicious/packages/routerlicious/config/config.json index e7276e1d5335..2a5777a73c4a 100644 --- a/server/routerlicious/packages/routerlicious/config/config.json +++ b/server/routerlicious/packages/routerlicious/config/config.json @@ -58,6 +58,9 @@ "kafkaCheckpointOnReprocessingOp": false, "ignoreCheckpointFlushException": false }, + "apiCounters": { + "fetchTenantKeyMetricMs": 60000 + }, "alfred": { "kafkaClientId": "alfred", "maxMessageSize": "16KB", diff --git a/server/routerlicious/packages/services-utils/src/apiCounters.ts b/server/routerlicious/packages/services-utils/src/apiCounters.ts new file mode 100644 index 000000000000..5dbe59a95d68 --- /dev/null +++ b/server/routerlicious/packages/services-utils/src/apiCounters.ts @@ -0,0 +1,72 @@ +/*! + * Copyright (c) Microsoft Corporation and contributors. All rights reserved. + * Licensed under the MIT License. + */ + +export interface IApiCounters { + initializeCounter(apiName: string): void; + initializeCounters(apiNames: string[]): void; + incrementCounter(apiName: string, incrementBy?: number): void; + decrementCounter(apiName: string, decrementBy?: number): void; + resetAllCounters(): void; + getCounter(apiName: string): number | undefined; + getCounters(): Record; + countersAreActive: boolean; +} + +export class InMemoryApiCounters implements IApiCounters { + private readonly apiCounters = new Map(); + constructor(apiNames?: string[]) { + if (apiNames && apiNames.length > 0) { + this.initializeCounters(apiNames); + } + } + + public initializeCounter(apiName: string): void { + this.apiCounters.set(apiName, 0); + } + + public initializeCounters(apiNames: string[]): void { + apiNames.forEach((apiName) => this.apiCounters.set(apiName, 0)); + } + + public incrementCounter(apiName: string, incrementBy = 1): void { + if (incrementBy < 1) { + return; + } + const currentValue = this.apiCounters.get(apiName) ?? 0; + this.apiCounters.set(apiName, currentValue + incrementBy); + } + + public decrementCounter(apiName: string, decrementBy = 1): void { + if (decrementBy < 1) { + return; + } + const currentValue = this.apiCounters.get(apiName) ?? 0; + const tentativeUpdate = currentValue - decrementBy; + // If the decrement would result in a negative number, reset it to 0. + const updatedValue = tentativeUpdate > 0 ? tentativeUpdate : 0; + this.apiCounters.set(apiName, updatedValue); + } + + public resetAllCounters(): void { + this.apiCounters.forEach((_: number, key) => this.apiCounters.set(key, 0)); + } + + public getCounter(apiName: string): number | undefined { + return this.apiCounters.get(apiName); + } + + public getCounters(): Record { + return Object.fromEntries(this.apiCounters); + } + + get countersAreActive(): boolean { + for (const v of this.apiCounters.values()) { + if (v > 0) { + return true; + } + } + return false; + } +} diff --git a/server/routerlicious/packages/services-utils/src/index.ts b/server/routerlicious/packages/services-utils/src/index.ts index 86fc38c2698e..f00f9426f6fe 100644 --- a/server/routerlicious/packages/services-utils/src/index.ts +++ b/server/routerlicious/packages/services-utils/src/index.ts @@ -46,3 +46,4 @@ export { DummyRevokedTokenChecker, } from "./tokenRevocationManager"; export { getBooleanFromConfig, getNumberFromConfig } from "./configUtils"; +export { IApiCounters, InMemoryApiCounters } from "./apiCounters";