Fix incorrect usage of TelemetryProcessor (#221)

* Fix incorrect usage of TelemetryProcessor

* Address feedback

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";
+}

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();
-    });
-  });
 });

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<T> {
   deferred: Q.Deferred<T>;
@@ -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;
 }

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<NotebookTerminalC
 
   public getTerminalParams(): Map<string, string> {
     let params: Map<string, string> = new Map<string, string>();
-    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<NotebookTerminalC
       return "";
     }
 
-    params.set("server", serverInfo.notebookServerEndpoint);
+    params.set(TerminalQueryParams.Server, serverInfo.notebookServerEndpoint);
     if (serverInfo.authToken && serverInfo.authToken.length > 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))}&`;

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);
       });
     });

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("");
         }

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))
+        });
       });
     });
   });

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("");
         }

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";
 

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,15 +115,12 @@ 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") {
+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: (window as any).authType,
+    authType: dataExplorerWindow && (dataExplorerWindow as any).authType,
     subscriptionId: userContext.subscriptionId as string,
     platform: configContext.platform,
     env: process.env.NODE_ENV as string,
@@ -128,5 +128,3 @@ function getData(actionModifier: string, data: unknown = {}): { [key: string]: s
     ...data
   };
 }
-  return undefined;
-}

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<ServerConnection.ISettings> = {
     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<void> => {
   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<void> => {
   });
 
   try {
-    if (urlVars.hasOwnProperty("terminal")) {
+    if (urlVars.hasOwnProperty(TerminalQueryParams.Terminal)) {
       await JupyterLabAppFactory.createTerminalApp(serverSettings);
     } else {
       throw new Error("Only terminal is supported");

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();
+    });
+  });
+});

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;
+};