From 42a5e13c333d34be82734bcc3cd91ff7e8dd7d25 Mon Sep 17 00:00:00 2001 From: Thomas Norling Date: Thu, 2 Nov 2023 14:02:36 -0700 Subject: [PATCH] Change ServerTelemetryEntity from Class to Type (#6651) Continuation of the work to convert cache entities from classes to types and moving the related static methods to CacheHelpers namespace --- ...-df07c979-2cd0-4c10-bacf-bd89de87433c.json | 7 ++++ ...-36947fe1-a24c-4fe7-976d-3be088e98af2.json | 7 ++++ ...-4b69c21b-f2e9-4feb-9dab-fd5ba6fcfb53.json | 7 ++++ .../src/cache/BrowserCacheManager.ts | 13 +++----- .../test/cache/BrowserCacheManager.spec.ts | 24 ++++++-------- .../cache/entities/ServerTelemetryEntity.ts | 32 ++----------------- .../src/cache/utils/CacheHelpers.ts | 21 ++++++++++++ .../server/ServerTelemetryManager.ts | 18 +++++++---- .../entities/ServerTelemetryEntity.spec.ts | 24 +++++++------- .../telemetry/ServerTelemetryManager.spec.ts | 6 +++- lib/msal-node/src/cache/NodeStorage.ts | 2 +- 11 files changed, 89 insertions(+), 72 deletions(-) create mode 100644 change/@azure-msal-browser-df07c979-2cd0-4c10-bacf-bd89de87433c.json create mode 100644 change/@azure-msal-common-36947fe1-a24c-4fe7-976d-3be088e98af2.json create mode 100644 change/@azure-msal-node-4b69c21b-f2e9-4feb-9dab-fd5ba6fcfb53.json diff --git a/change/@azure-msal-browser-df07c979-2cd0-4c10-bacf-bd89de87433c.json b/change/@azure-msal-browser-df07c979-2cd0-4c10-bacf-bd89de87433c.json new file mode 100644 index 0000000000..a95794f4d1 --- /dev/null +++ b/change/@azure-msal-browser-df07c979-2cd0-4c10-bacf-bd89de87433c.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Convert ServerTelemetryEntity to Type instead of Class #6651", + "packageName": "@azure/msal-browser", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@azure-msal-common-36947fe1-a24c-4fe7-976d-3be088e98af2.json b/change/@azure-msal-common-36947fe1-a24c-4fe7-976d-3be088e98af2.json new file mode 100644 index 0000000000..9333556280 --- /dev/null +++ b/change/@azure-msal-common-36947fe1-a24c-4fe7-976d-3be088e98af2.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Convert ServerTelemetryEntity to Type instead of Class #6651", + "packageName": "@azure/msal-common", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/change/@azure-msal-node-4b69c21b-f2e9-4feb-9dab-fd5ba6fcfb53.json b/change/@azure-msal-node-4b69c21b-f2e9-4feb-9dab-fd5ba6fcfb53.json new file mode 100644 index 0000000000..dc8333bedd --- /dev/null +++ b/change/@azure-msal-node-4b69c21b-f2e9-4feb-9dab-fd5ba6fcfb53.json @@ -0,0 +1,7 @@ +{ + "type": "minor", + "comment": "Convert ServerTelemetryEntity to Type instead of Class #6651", + "packageName": "@azure/msal-node", + "email": "thomas.norling@microsoft.com", + "dependentChangeType": "patch" +} diff --git a/lib/msal-browser/src/cache/BrowserCacheManager.ts b/lib/msal-browser/src/cache/BrowserCacheManager.ts index 1722c3d787..a1d5db4cb3 100644 --- a/lib/msal-browser/src/cache/BrowserCacheManager.ts +++ b/lib/msal-browser/src/cache/BrowserCacheManager.ts @@ -880,12 +880,12 @@ export class BrowserCacheManager extends CacheManager { ); return null; } - const parsedMetadata = this.validateAndParseJson(value); + const parsedEntity = this.validateAndParseJson(value); if ( - !parsedMetadata || - !ServerTelemetryEntity.isServerTelemetryEntity( + !parsedEntity || + !CacheHelpers.isServerTelemetryEntity( serverTelemetryKey, - parsedMetadata + parsedEntity ) ) { this.logger.trace( @@ -895,10 +895,7 @@ export class BrowserCacheManager extends CacheManager { } this.logger.trace("BrowserCacheManager.getServerTelemetry: cache hit"); - return CacheManager.toObject( - new ServerTelemetryEntity(), - parsedMetadata - ); + return parsedEntity as ServerTelemetryEntity; } /** diff --git a/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts b/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts index c57aa2d428..7d3d1e7365 100644 --- a/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts +++ b/lib/msal-browser/test/cache/BrowserCacheManager.spec.ts @@ -1384,7 +1384,11 @@ describe("BrowserCacheManager tests", () => { it("getServerTelemetry returns ServerTelemetryEntity", () => { const testKey = "server-telemetry-clientId"; - const testVal = new ServerTelemetryEntity(); + const testVal = { + failedRequests: ["61|test-correlationId"], + errors: ["test_error"], + cacheHits: 2, + }; browserLocalStorage.setServerTelemetry(testKey, testVal); browserSessionStorage.setServerTelemetry(testKey, testVal); @@ -1392,15 +1396,9 @@ describe("BrowserCacheManager tests", () => { expect( browserSessionStorage.getServerTelemetry(testKey) ).toEqual(testVal); - expect( - browserSessionStorage.getServerTelemetry(testKey) - ).toBeInstanceOf(ServerTelemetryEntity); expect( browserLocalStorage.getServerTelemetry(testKey) ).toEqual(testVal); - expect( - browserLocalStorage.getServerTelemetry(testKey) - ).toBeInstanceOf(ServerTelemetryEntity); }); }); @@ -2294,7 +2292,11 @@ describe("BrowserCacheManager tests", () => { it("getServerTelemetry returns ServerTelemetryEntity", () => { const testKey = "server-telemetry-clientId"; - const testVal = new ServerTelemetryEntity(); + const testVal = { + failedRequests: ["61|test-correlationId"], + errors: ["test_error"], + cacheHits: 2, + }; browserLocalStorage.setServerTelemetry(testKey, testVal); browserSessionStorage.setServerTelemetry(testKey, testVal); @@ -2302,15 +2304,9 @@ describe("BrowserCacheManager tests", () => { expect( browserSessionStorage.getServerTelemetry(testKey) ).toEqual(testVal); - expect( - browserSessionStorage.getServerTelemetry(testKey) - ).toBeInstanceOf(ServerTelemetryEntity); expect( browserLocalStorage.getServerTelemetry(testKey) ).toEqual(testVal); - expect( - browserLocalStorage.getServerTelemetry(testKey) - ).toBeInstanceOf(ServerTelemetryEntity); }); }); diff --git a/lib/msal-common/src/cache/entities/ServerTelemetryEntity.ts b/lib/msal-common/src/cache/entities/ServerTelemetryEntity.ts index 19c95cfb2e..1748cf13ee 100644 --- a/lib/msal-common/src/cache/entities/ServerTelemetryEntity.ts +++ b/lib/msal-common/src/cache/entities/ServerTelemetryEntity.ts @@ -3,36 +3,8 @@ * Licensed under the MIT License. */ -import { SERVER_TELEM_CONSTANTS } from "../../utils/Constants"; - -export class ServerTelemetryEntity { +export type ServerTelemetryEntity = { failedRequests: Array; errors: string[]; cacheHits: number; - - constructor() { - this.failedRequests = []; - this.errors = []; - this.cacheHits = 0; - } - - /** - * validates if a given cache entry is "Telemetry", parses - * @param key - * @param entity - */ - static isServerTelemetryEntity(key: string, entity?: object): boolean { - const validateKey: boolean = - key.indexOf(SERVER_TELEM_CONSTANTS.CACHE_KEY) === 0; - let validateEntity: boolean = true; - - if (entity) { - validateEntity = - entity.hasOwnProperty("failedRequests") && - entity.hasOwnProperty("errors") && - entity.hasOwnProperty("cacheHits"); - } - - return validateKey && validateEntity; - } -} +}; diff --git a/lib/msal-common/src/cache/utils/CacheHelpers.ts b/lib/msal-common/src/cache/utils/CacheHelpers.ts index 4ef89ce8f3..cd9af986a0 100644 --- a/lib/msal-common/src/cache/utils/CacheHelpers.ts +++ b/lib/msal-common/src/cache/utils/CacheHelpers.ts @@ -12,6 +12,7 @@ import { import { AuthenticationScheme, CredentialType, + SERVER_TELEM_CONSTANTS, Separators, } from "../../utils/Constants"; import { TimeUtils } from "../../utils/TimeUtils"; @@ -304,3 +305,23 @@ function generateScheme(credentialEntity: CredentialEntity): string { ? credentialEntity.tokenType.toLowerCase() : ""; } + +/** + * validates if a given cache entry is "Telemetry", parses + * @param key + * @param entity + */ +export function isServerTelemetryEntity(key: string, entity?: object): boolean { + const validateKey: boolean = + key.indexOf(SERVER_TELEM_CONSTANTS.CACHE_KEY) === 0; + let validateEntity: boolean = true; + + if (entity) { + validateEntity = + entity.hasOwnProperty("failedRequests") && + entity.hasOwnProperty("errors") && + entity.hasOwnProperty("cacheHits"); + } + + return validateKey && validateEntity; +} diff --git a/lib/msal-common/src/telemetry/server/ServerTelemetryManager.ts b/lib/msal-common/src/telemetry/server/ServerTelemetryManager.ts index f449fe6459..7778ddb2ab 100644 --- a/lib/msal-common/src/telemetry/server/ServerTelemetryManager.ts +++ b/lib/msal-common/src/telemetry/server/ServerTelemetryManager.ts @@ -160,7 +160,11 @@ export class ServerTelemetryManager { * Get the server telemetry entity from cache or initialize a new one */ getLastRequests(): ServerTelemetryEntity { - const initialValue: ServerTelemetryEntity = new ServerTelemetryEntity(); + const initialValue: ServerTelemetryEntity = { + failedRequests: [], + errors: [], + cacheHits: 0, + }; const lastRequests = this.cacheManager.getServerTelemetry( this.telemetryCacheKey ) as ServerTelemetryEntity; @@ -181,11 +185,13 @@ export class ServerTelemetryManager { this.cacheManager.removeItem(this.telemetryCacheKey); } else { // Partial data was flushed to server, construct a new telemetry cache item with errors that were not flushed - const serverTelemEntity = new ServerTelemetryEntity(); - serverTelemEntity.failedRequests = - lastRequests.failedRequests.slice(numErrorsFlushed * 2); // failedRequests contains 2 items for each error - serverTelemEntity.errors = - lastRequests.errors.slice(numErrorsFlushed); + const serverTelemEntity: ServerTelemetryEntity = { + failedRequests: lastRequests.failedRequests.slice( + numErrorsFlushed * 2 + ), // failedRequests contains 2 items for each error + errors: lastRequests.errors.slice(numErrorsFlushed), + cacheHits: 0, + }; this.cacheManager.setServerTelemetry( this.telemetryCacheKey, diff --git a/lib/msal-common/test/cache/entities/ServerTelemetryEntity.spec.ts b/lib/msal-common/test/cache/entities/ServerTelemetryEntity.spec.ts index 8b12593e9d..84eeed5b72 100644 --- a/lib/msal-common/test/cache/entities/ServerTelemetryEntity.spec.ts +++ b/lib/msal-common/test/cache/entities/ServerTelemetryEntity.spec.ts @@ -1,9 +1,9 @@ -import { ServerTelemetryEntity } from "../../../src/cache/entities/ServerTelemetryEntity"; import { SERVER_TELEM_CONSTANTS, Separators, } from "../../../src/utils/Constants"; import { TEST_CONFIG } from "../../test_kit/StringConstants"; +import * as CacheHelpers from "../../../src/cache/utils/CacheHelpers"; describe("ServerTelemetryEntity.ts Unit Tests", () => { const ServerTelemetryKey = @@ -19,7 +19,7 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { }; expect( - ServerTelemetryEntity.isServerTelemetryEntity( + CacheHelpers.isServerTelemetryEntity( ServerTelemetryKey, serverTelemetryObject ) @@ -34,7 +34,7 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { }; expect( - ServerTelemetryEntity.isServerTelemetryEntity( + CacheHelpers.isServerTelemetryEntity( ServerTelemetryKey, missingCacheHits ) @@ -46,7 +46,7 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { cacheHits: 0, }; expect( - ServerTelemetryEntity.isServerTelemetryEntity( + CacheHelpers.isServerTelemetryEntity( ServerTelemetryKey, missingErrors ) @@ -58,7 +58,7 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { cacheHits: 0, }; expect( - ServerTelemetryEntity.isServerTelemetryEntity( + CacheHelpers.isServerTelemetryEntity( ServerTelemetryKey, missingFailedRequests ) @@ -68,7 +68,7 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { testParam: "test", }; expect( - ServerTelemetryEntity.isServerTelemetryEntity( + CacheHelpers.isServerTelemetryEntity( ServerTelemetryKey, noCommonValues ) @@ -76,9 +76,9 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { }); it("Verify an object is a ServerTelemetry Entity only when cache key is passed", () => { - expect( - ServerTelemetryEntity.isServerTelemetryEntity(ServerTelemetryKey) - ).toBe(true); + expect(CacheHelpers.isServerTelemetryEntity(ServerTelemetryKey)).toBe( + true + ); }); it("Verify an object is not a ServerTelemetry Entity only when cache key is passed", () => { @@ -86,8 +86,8 @@ describe("ServerTelemetryEntity.ts Unit Tests", () => { Separators.CACHE_KEY_SEPARATOR + SERVER_TELEM_CONSTANTS.CACHE_KEY + TEST_CONFIG.MSAL_CLIENT_ID; - expect( - ServerTelemetryEntity.isServerTelemetryEntity(ServerTelemetryKey) - ).toBe(false); + expect(CacheHelpers.isServerTelemetryEntity(ServerTelemetryKey)).toBe( + false + ); }); }); diff --git a/lib/msal-common/test/telemetry/ServerTelemetryManager.spec.ts b/lib/msal-common/test/telemetry/ServerTelemetryManager.spec.ts index 74a270725f..76468a4162 100644 --- a/lib/msal-common/test/telemetry/ServerTelemetryManager.spec.ts +++ b/lib/msal-common/test/telemetry/ServerTelemetryManager.spec.ts @@ -361,7 +361,11 @@ describe("ServerTelemetryManager.ts", () => { }); it("incrementCacheHits", () => { - const initialCacheValue = new ServerTelemetryEntity(); + const initialCacheValue: ServerTelemetryEntity = { + failedRequests: [], + errors: [], + cacheHits: 0, + }; initialCacheValue.cacheHits = 1; testCacheManager.setServerTelemetry(cacheKey, initialCacheValue); const telemetryManager = new ServerTelemetryManager( diff --git a/lib/msal-node/src/cache/NodeStorage.ts b/lib/msal-node/src/cache/NodeStorage.ts index 5878ab4762..2804c75a50 100644 --- a/lib/msal-node/src/cache/NodeStorage.ts +++ b/lib/msal-node/src/cache/NodeStorage.ts @@ -339,7 +339,7 @@ export class NodeStorage extends CacheManager { ) as ServerTelemetryEntity; if ( serverTelemetryEntity && - ServerTelemetryEntity.isServerTelemetryEntity( + CacheHelpers.isServerTelemetryEntity( serverTelemetrykey, serverTelemetryEntity )