From 7c5c8ddb7aed9b7410b33bbda96d4710bb21c1b8 Mon Sep 17 00:00:00 2001 From: Tanuj Mittal Date: Tue, 22 Sep 2020 12:19:06 -0700 Subject: [PATCH] Fix incorrect usage of TelemetryProcessor (#221) * Fix incorrect usage of TelemetryProcessor * Address feedback --- src/Common/Constants.ts | 8 +++ src/Common/MessageHandler.test.ts | 30 ------------ src/Common/MessageHandler.ts | 20 +------- .../Notebook/NotebookTerminalComponent.tsx | 12 +++-- .../DeleteCollectionConfirmationPane.test.ts | 8 ++- .../Panes/DeleteCollectionConfirmationPane.ts | 8 ++- .../DeleteDatabaseConfirmationPane.test.ts | 8 ++- .../Panes/DeleteDatabaseConfirmationPane.ts | 8 ++- src/Explorer/Tabs/MongoShellTab.ts | 2 +- src/Shared/Telemetry/TelemetryProcessor.ts | 46 +++++++++-------- src/Terminal/index.ts | 20 +++++--- src/Utils/WindowUtils.test.ts | 49 +++++++++++++++++++ src/Utils/WindowUtils.ts | 17 +++++++ 13 files changed, 132 insertions(+), 104 deletions(-) create mode 100644 src/Utils/WindowUtils.test.ts create mode 100644 src/Utils/WindowUtils.ts diff --git a/src/Common/Constants.ts b/src/Common/Constants.ts index 8458bc0c0..8762a98e4 100644 --- a/src/Common/Constants.ts +++ b/src/Common/Constants.ts @@ -562,3 +562,11 @@ export class AnalyticalStorageTtl { public static readonly Infinite: number = -1; public static readonly Disabled: number = 0; } + +export class TerminalQueryParams { + public static readonly Terminal = "terminal"; + public static readonly Server = "server"; + public static readonly Token = "token"; + public static readonly SubscriptionId = "subscriptionId"; + public static readonly TerminalEndpoint = "terminalEndpoint"; +} diff --git a/src/Common/MessageHandler.test.ts b/src/Common/MessageHandler.test.ts index 8baef1740..505226243 100644 --- a/src/Common/MessageHandler.test.ts +++ b/src/Common/MessageHandler.test.ts @@ -25,34 +25,4 @@ describe("Message Handler", () => { MessageHandler.runGarbageCollector(); expect(MessageHandler.RequestMap["123"]).toBeUndefined(); }); - - describe("getDataExplorerWindow", () => { - it("should return current window if current window has dataExplorerPlatform property", () => { - const currentWindow: Window = { dataExplorerPlatform: 0 } as any; - - expect(MessageHandler.getDataExplorerWindow(currentWindow)).toEqual(currentWindow); - }); - - it("should return current window's parent if current window's parent has dataExplorerPlatform property", () => { - const parentWindow: Window = { dataExplorerPlatform: 0 } as any; - const currentWindow: Window = { parent: parentWindow } as any; - - expect(MessageHandler.getDataExplorerWindow(currentWindow)).toEqual(parentWindow); - }); - - it("should return undefined if none of the windows in the hierarchy have dataExplorerPlatform property and window's parent is reference to itself", () => { - const parentWindow: Window = {} as any; - (parentWindow as any).parent = parentWindow; // If a window does not have a parent, its parent property is a reference to itself. - const currentWindow: Window = { parent: parentWindow } as any; - - expect(MessageHandler.getDataExplorerWindow(currentWindow)).toBeUndefined(); - }); - - it("should return undefined if none of the windows in the hierarchy have dataExplorerPlatform property and window's parent is not defined", () => { - const parentWindow: Window = {} as any; - const currentWindow: Window = { parent: parentWindow } as any; - - expect(MessageHandler.getDataExplorerWindow(currentWindow)).toBeUndefined(); - }); - }); }); diff --git a/src/Common/MessageHandler.ts b/src/Common/MessageHandler.ts index ce1af9b47..31317800a 100644 --- a/src/Common/MessageHandler.ts +++ b/src/Common/MessageHandler.ts @@ -2,6 +2,7 @@ import { MessageTypes } from "../Contracts/ExplorerContracts"; import Q from "q"; import * as _ from "underscore"; import * as Constants from "./Constants"; +import { getDataExplorerWindow } from "../Utils/WindowUtils"; export interface CachedDataPromise { deferred: Q.Deferred; @@ -61,25 +62,6 @@ export function sendMessage(data: any): void { } } -// Only exported for unit tests -export const getDataExplorerWindow = (currentWindow: Window): Window | undefined => { - // Start with the current window and traverse up the parent hierarchy to find a window - // with `dataExplorerPlatform` property - let dataExplorerWindow: Window | undefined = currentWindow; - // TODO: Need to `any` here since the window imports Explorer which can't be in strict mode yet - // eslint-disable-next-line @typescript-eslint/no-explicit-any - while (dataExplorerWindow && (dataExplorerWindow as any).dataExplorerPlatform == undefined) { - // If a window does not have a parent, its parent property is a reference to itself. - if (dataExplorerWindow.parent == dataExplorerWindow) { - dataExplorerWindow = undefined; - } else { - dataExplorerWindow = dataExplorerWindow.parent; - } - } - - return dataExplorerWindow; -}; - export function canSendMessage(): boolean { return window.parent !== window; } diff --git a/src/Explorer/Controls/Notebook/NotebookTerminalComponent.tsx b/src/Explorer/Controls/Notebook/NotebookTerminalComponent.tsx index 63c6f0768..de3448741 100644 --- a/src/Explorer/Controls/Notebook/NotebookTerminalComponent.tsx +++ b/src/Explorer/Controls/Notebook/NotebookTerminalComponent.tsx @@ -8,6 +8,8 @@ import * as Logger from "../../../Common/Logger"; import * as NotificationConsoleUtils from "../../../Utils/NotificationConsoleUtils"; import { ConsoleDataType } from "../../Menus/NotificationConsole/NotificationConsoleComponent"; import { StringUtils } from "../../../Utils/StringUtils"; +import { userContext } from "../../../UserContext"; +import { TerminalQueryParams } from "../../../Common/Constants"; export interface NotebookTerminalComponentProps { notebookServerInfo: DataModels.NotebookWorkspaceConnectionInfo; @@ -32,11 +34,11 @@ export class NotebookTerminalComponent extends React.Component { let params: Map = new Map(); - params.set("terminal", "true"); + params.set(TerminalQueryParams.Terminal, "true"); const terminalEndpoint: string = this.tryGetTerminalEndpoint(); if (terminalEndpoint) { - params.set("terminalEndpoint", terminalEndpoint); + params.set(TerminalQueryParams.TerminalEndpoint, terminalEndpoint); } return params; @@ -75,11 +77,13 @@ export class NotebookTerminalComponent extends React.Component 0) { - params.set("token", serverInfo.authToken); + params.set(TerminalQueryParams.Token, serverInfo.authToken); } + params.set(TerminalQueryParams.SubscriptionId, userContext.subscriptionId); + let result: string = "terminal.html?"; for (let key of params.keys()) { result += `${key}=${encodeURIComponent(params.get(key))}&`; diff --git a/src/Explorer/Panes/DeleteCollectionConfirmationPane.test.ts b/src/Explorer/Panes/DeleteCollectionConfirmationPane.test.ts index fdf7fa162..5e7f46613 100644 --- a/src/Explorer/Panes/DeleteCollectionConfirmationPane.test.ts +++ b/src/Explorer/Panes/DeleteCollectionConfirmationPane.test.ts @@ -134,11 +134,9 @@ describe("Delete Collection Confirmation Pane", () => { expect(telemetryProcessorSpy.called).toBe(true); let deleteFeedback = new DeleteFeedback(SubscriptionId, AccountName, DataModels.ApiKind.SQL, Feedback); expect( - telemetryProcessorSpy.calledWith( - Action.DeleteCollection, - ActionModifiers.Mark, - JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) - ) + telemetryProcessorSpy.calledWith(Action.DeleteCollection, ActionModifiers.Mark, { + message: JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) + }) ).toBe(true); }); }); diff --git a/src/Explorer/Panes/DeleteCollectionConfirmationPane.ts b/src/Explorer/Panes/DeleteCollectionConfirmationPane.ts index 09c9426e6..ed73d352c 100644 --- a/src/Explorer/Panes/DeleteCollectionConfirmationPane.ts +++ b/src/Explorer/Panes/DeleteCollectionConfirmationPane.ts @@ -88,11 +88,9 @@ export default class DeleteCollectionConfirmationPane extends ContextualPaneBase this.containerDeleteFeedback() ); - TelemetryProcessor.trace( - Action.DeleteCollection, - ActionModifiers.Mark, - JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) - ); + TelemetryProcessor.trace(Action.DeleteCollection, ActionModifiers.Mark, { + message: JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) + }); this.containerDeleteFeedback(""); } diff --git a/src/Explorer/Panes/DeleteDatabaseConfirmationPane.test.ts b/src/Explorer/Panes/DeleteDatabaseConfirmationPane.test.ts index 2167a5b2a..b5299a9ec 100644 --- a/src/Explorer/Panes/DeleteDatabaseConfirmationPane.test.ts +++ b/src/Explorer/Panes/DeleteDatabaseConfirmationPane.test.ts @@ -120,11 +120,9 @@ describe("Delete Database Confirmation Pane", () => { return pane.submit().then(() => { let deleteFeedback = new DeleteFeedback(SubscriptionId, AccountName, DataModels.ApiKind.SQL, Feedback); - expect(TelemetryProcessor.trace).toHaveBeenCalledWith( - Action.DeleteDatabase, - ActionModifiers.Mark, - JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) - ); + expect(TelemetryProcessor.trace).toHaveBeenCalledWith(Action.DeleteDatabase, ActionModifiers.Mark, { + message: JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) + }); }); }); }); diff --git a/src/Explorer/Panes/DeleteDatabaseConfirmationPane.ts b/src/Explorer/Panes/DeleteDatabaseConfirmationPane.ts index b0c32d973..db6e6d4ed 100644 --- a/src/Explorer/Panes/DeleteDatabaseConfirmationPane.ts +++ b/src/Explorer/Panes/DeleteDatabaseConfirmationPane.ts @@ -97,11 +97,9 @@ export default class DeleteDatabaseConfirmationPane extends ContextualPaneBase { this.databaseDeleteFeedback() ); - TelemetryProcessor.trace( - Action.DeleteDatabase, - ActionModifiers.Mark, - JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) - ); + TelemetryProcessor.trace(Action.DeleteDatabase, ActionModifiers.Mark, { + message: JSON.stringify(deleteFeedback, Object.getOwnPropertyNames(deleteFeedback)) + }); this.databaseDeleteFeedback(""); } diff --git a/src/Explorer/Tabs/MongoShellTab.ts b/src/Explorer/Tabs/MongoShellTab.ts index 5369a0e06..f939fd597 100644 --- a/src/Explorer/Tabs/MongoShellTab.ts +++ b/src/Explorer/Tabs/MongoShellTab.ts @@ -142,7 +142,7 @@ export default class MongoShellTab extends TabsBase { return; } - const dataToLog: string = event.data.data.logData; + const dataToLog = { message: event.data.data.logData }; const logType: string = event.data.data.logType; const shellTraceId: string = event.data.data.traceId || "none"; diff --git a/src/Shared/Telemetry/TelemetryProcessor.ts b/src/Shared/Telemetry/TelemetryProcessor.ts index 308ac68f1..be484367f 100644 --- a/src/Shared/Telemetry/TelemetryProcessor.ts +++ b/src/Shared/Telemetry/TelemetryProcessor.ts @@ -4,12 +4,15 @@ import { MessageTypes } from "../../Contracts/ExplorerContracts"; import { appInsights } from "../appInsights"; import { configContext } from "../../ConfigContext"; import { userContext } from "../../UserContext"; +import { getDataExplorerWindow } from "../../Utils/WindowUtils"; /** * Class that persists telemetry data to the portal tables. */ -export function trace(action: Action, actionModifier: string = ActionModifiers.Mark, data?: unknown): void { +type TelemetryData = { [key: string]: unknown }; + +export function trace(action: Action, actionModifier: string = ActionModifiers.Mark, data?: TelemetryData): void { sendMessage({ type: MessageTypes.TelemetryInfo, data: { @@ -22,7 +25,7 @@ export function trace(action: Action, actionModifier: string = ActionModifiers.M appInsights.trackEvent({ name: Action[action] }, getData(actionModifier, data)); } -export function traceStart(action: Action, data?: unknown): number { +export function traceStart(action: Action, data?: TelemetryData): number { const timestamp: number = Date.now(); sendMessage({ type: MessageTypes.TelemetryInfo, @@ -38,7 +41,7 @@ export function traceStart(action: Action, data?: unknown): number { return timestamp; } -export function traceSuccess(action: Action, data?: unknown, timestamp?: number): void { +export function traceSuccess(action: Action, data?: TelemetryData, timestamp?: number): void { sendMessage({ type: MessageTypes.TelemetryInfo, data: { @@ -52,7 +55,7 @@ export function traceSuccess(action: Action, data?: unknown, timestamp?: number) appInsights.stopTrackEvent(Action[action], getData(ActionModifiers.Success, data)); } -export function traceFailure(action: Action, data?: unknown, timestamp?: number): void { +export function traceFailure(action: Action, data?: TelemetryData, timestamp?: number): void { sendMessage({ type: MessageTypes.TelemetryInfo, data: { @@ -66,7 +69,7 @@ export function traceFailure(action: Action, data?: unknown, timestamp?: number) appInsights.stopTrackEvent(Action[action], getData(ActionModifiers.Failed, data)); } -export function traceCancel(action: Action, data?: unknown, timestamp?: number): void { +export function traceCancel(action: Action, data?: TelemetryData, timestamp?: number): void { sendMessage({ type: MessageTypes.TelemetryInfo, data: { @@ -80,7 +83,7 @@ export function traceCancel(action: Action, data?: unknown, timestamp?: number): appInsights.stopTrackEvent(Action[action], getData(ActionModifiers.Cancel, data)); } -export function traceOpen(action: Action, data?: unknown, timestamp?: number): number { +export function traceOpen(action: Action, data?: TelemetryData, timestamp?: number): number { const validTimestamp = timestamp || Date.now(); sendMessage({ type: MessageTypes.TelemetryInfo, @@ -96,7 +99,7 @@ export function traceOpen(action: Action, data?: unknown, timestamp?: number): n return validTimestamp; } -export function traceMark(action: Action, data?: unknown, timestamp?: number): number { +export function traceMark(action: Action, data?: TelemetryData, timestamp?: number): number { const validTimestamp = timestamp || Date.now(); sendMessage({ type: MessageTypes.TelemetryInfo, @@ -112,21 +115,16 @@ export function traceMark(action: Action, data?: unknown, timestamp?: number): n return validTimestamp; } -function getData(actionModifier: string, data: unknown = {}): { [key: string]: string } | undefined { - if (typeof data === "string") { - data = { message: data }; - } - if (typeof data === "object") { - return { - // TODO: Need to `any` here since the window imports Explorer which can't be in strict mode yet - // eslint-disable-next-line @typescript-eslint/no-explicit-any - authType: (window as any).authType, - subscriptionId: userContext.subscriptionId as string, - platform: configContext.platform, - env: process.env.NODE_ENV as string, - actionModifier, - ...data - }; - } - return undefined; +function getData(actionModifier: string, data: TelemetryData = {}): { [key: string]: string } { + const dataExplorerWindow = getDataExplorerWindow(window); + return { + // TODO: Need to `any` here since the window imports Explorer which can't be in strict mode yet + // eslint-disable-next-line @typescript-eslint/no-explicit-any + authType: dataExplorerWindow && (dataExplorerWindow as any).authType, + subscriptionId: userContext.subscriptionId as string, + platform: configContext.platform, + env: process.env.NODE_ENV as string, + actionModifier, + ...data + }; } diff --git a/src/Terminal/index.ts b/src/Terminal/index.ts index 605d290f7..be8a7be2a 100644 --- a/src/Terminal/index.ts +++ b/src/Terminal/index.ts @@ -6,6 +6,8 @@ import { ServerConnection } from "@jupyterlab/services"; import { JupyterLabAppFactory } from "./JupyterLabAppFactory"; import { Action } from "../Shared/Telemetry/TelemetryConstants"; import * as TelemetryProcessor from "../Shared/Telemetry/TelemetryProcessor"; +import { updateUserContext } from "../UserContext"; +import { TerminalQueryParams } from "../Common/Constants"; const getUrlVars = (): { [key: string]: string } => { const vars: { [key: string]: string } = {}; @@ -18,22 +20,22 @@ const getUrlVars = (): { [key: string]: string } => { const createServerSettings = (urlVars: { [key: string]: string }): ServerConnection.ISettings => { let body: BodyInit; - if (urlVars.hasOwnProperty("terminalEndpoint")) { + if (urlVars.hasOwnProperty(TerminalQueryParams.TerminalEndpoint)) { body = JSON.stringify({ - endpoint: urlVars["terminalEndpoint"] + endpoint: urlVars[TerminalQueryParams.TerminalEndpoint] }); } - const server = urlVars["server"]; + const server = urlVars[TerminalQueryParams.Server]; let options: Partial = { baseUrl: server, init: { body }, fetch: window.parent.fetch }; - if (urlVars.hasOwnProperty("token")) { + if (urlVars.hasOwnProperty(TerminalQueryParams.Token)) { options = { baseUrl: server, - token: urlVars["token"], + token: urlVars[TerminalQueryParams.Token], init: { body }, fetch: window.parent.fetch }; @@ -44,6 +46,12 @@ const createServerSettings = (urlVars: { [key: string]: string }): ServerConnect const main = async (): Promise => { const urlVars = getUrlVars(); + + // Initialize userContext. Currently only subscrptionId is required by TelemetryProcessor + updateUserContext({ + subscriptionId: urlVars[TerminalQueryParams.SubscriptionId] + }); + const serverSettings = createServerSettings(urlVars); const startTime = TelemetryProcessor.traceStart(Action.OpenTerminal, { @@ -51,7 +59,7 @@ const main = async (): Promise => { }); try { - if (urlVars.hasOwnProperty("terminal")) { + if (urlVars.hasOwnProperty(TerminalQueryParams.Terminal)) { await JupyterLabAppFactory.createTerminalApp(serverSettings); } else { throw new Error("Only terminal is supported"); diff --git a/src/Utils/WindowUtils.test.ts b/src/Utils/WindowUtils.test.ts new file mode 100644 index 000000000..ea78a82b0 --- /dev/null +++ b/src/Utils/WindowUtils.test.ts @@ -0,0 +1,49 @@ +import { getDataExplorerWindow } from "./WindowUtils"; + +const createWindow = (dataExplorerPlatform: unknown, parent: Window): Window => { + // TODO: Need to `any` here since we're creating a mock window object + // eslint-disable-next-line @typescript-eslint/no-explicit-any + const mockWindow: any = {}; + if (dataExplorerPlatform !== undefined) { + mockWindow.dataExplorerPlatform = dataExplorerPlatform; + } + if (parent) { + mockWindow.parent = parent; + } + return mockWindow; +}; + +describe("WindowUtils", () => { + describe("getDataExplorerWindow", () => { + it("should return current window if current window has dataExplorerPlatform property", () => { + const currentWindow = createWindow(0, undefined); + + expect(getDataExplorerWindow(currentWindow)).toEqual(currentWindow); + }); + + it("should return current window's parent if current window's parent has dataExplorerPlatform property", () => { + const parentWindow = createWindow(0, undefined); + const currentWindow = createWindow(undefined, parentWindow); + + expect(getDataExplorerWindow(currentWindow)).toEqual(parentWindow); + }); + + it("should return undefined if none of the windows in the hierarchy have dataExplorerPlatform property and window's parent is reference to itself", () => { + const parentWindow = createWindow(undefined, undefined); + + // TODO: Need to `any` here since parent is a readonly property + // eslint-disable-next-line @typescript-eslint/no-explicit-any + (parentWindow as any).parent = parentWindow; // If a window does not have a parent, its parent property is a reference to itself. + const currentWindow = createWindow(undefined, parentWindow); + + expect(getDataExplorerWindow(currentWindow)).toBeUndefined(); + }); + + it("should return undefined if none of the windows in the hierarchy have dataExplorerPlatform property and window's parent is not defined", () => { + const parentWindow = createWindow(undefined, undefined); + const currentWindow = createWindow(undefined, parentWindow); + + expect(getDataExplorerWindow(currentWindow)).toBeUndefined(); + }); + }); +}); diff --git a/src/Utils/WindowUtils.ts b/src/Utils/WindowUtils.ts new file mode 100644 index 000000000..e672f288c --- /dev/null +++ b/src/Utils/WindowUtils.ts @@ -0,0 +1,17 @@ +export const getDataExplorerWindow = (currentWindow: Window): Window | undefined => { + // Start with the current window and traverse up the parent hierarchy to find a window + // with `dataExplorerPlatform` property + let dataExplorerWindow: Window | undefined = currentWindow; + // TODO: Need to `any` here since the window imports Explorer which can't be in strict mode yet + // eslint-disable-next-line @typescript-eslint/no-explicit-any + while (dataExplorerWindow && (dataExplorerWindow as any).dataExplorerPlatform === undefined) { + // If a window does not have a parent, its parent property is a reference to itself. + if (dataExplorerWindow.parent === dataExplorerWindow) { + dataExplorerWindow = undefined; + } else { + dataExplorerWindow = dataExplorerWindow.parent; + } + } + + return dataExplorerWindow; +};