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.
microsoft · Jul 18, 2023 · a9cefdf · a9cefdf
1 parent 1363c46
commit a9cefdf
Show file tree

Hide file tree

Showing 10 changed files with 138 additions and 29 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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<RiddlerResourc
 		const defaultInternalHistorianUrl =
 			config.get("worker:internalBlobStorageUrl") || defaultHistorianUrl;
 
+		const fetchTenantKeyMetricIntervalMs = config.get("apiCounters:fetchTenantKeyMetricMs");
+
 		return new RiddlerResources(
 			config,
 			tenantsCollectionName,
@@ -148,6 +151,7 @@ export class RiddlerResourcesFactory implements IResourcesFactory<RiddlerResourc
 			defaultHistorianUrl,
 			defaultInternalHistorianUrl,
 			secretManager,
+			fetchTenantKeyMetricIntervalMs,
 			cache,
 		);
 	}
@@ -165,6 +169,7 @@ export class RiddlerRunnerFactory implements IRunnerFactory<RiddlerResources> {
 			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
@@ -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,18 +55,39 @@ 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,
 		private readonly baseOrdererUrl: string,
 		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,19 +359,13 @@ export class TenantManager {
 			includeDisabledTenant,
 			bypassCache,
 		};
-		const fetchTenantKeyMetric = Lumberjack.newLumberMetric(
-			LumberEventName.RiddlerFetchTenantKey,
-		);
-		let uncaughtException;
-		let retrievedFromCache = false;
 
 		try {
 			if (!bypassCache && this.isCacheEnabled) {
 				// Read from cache first
 				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,30 +438,16 @@ 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 {
 				key1: tenantKey1,
 				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<string> {
-		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<boolean> {
@@ -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
@@ -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
@@ -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
@@ -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<string, number>;
+	countersAreActive: boolean;
+}
+
+export class InMemoryApiCounters implements IApiCounters {
+	private readonly apiCounters = new Map<string, number>();
+	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<string, number> {
+		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
@@ -46,3 +46,4 @@ export {
 	DummyRevokedTokenChecker,
 } from "./tokenRevocationManager";
 export { getBooleanFromConfig, getNumberFromConfig } from "./configUtils";
+export { IApiCounters, InMemoryApiCounters } from "./apiCounters";