From a91edccb46bfa8eaad3cdb98c93e535f7832dbd1 Mon Sep 17 00:00:00 2001 From: Tony Anziano Date: Mon, 27 Jan 2020 14:16:13 -0800 Subject: [PATCH] Added new telemetry events and properties. --- CHANGELOG.md | 1 + .../src/state/sagas/azureAuthSaga.spec.ts | 9 ++- .../client/src/state/sagas/azureAuthSaga.ts | 6 +- .../app/client/src/state/sagas/botSagas.ts | 20 ++++-- .../sagas/frameworkSettingsSagas.spec.ts | 59 ++++++++++++---- .../src/state/sagas/frameworkSettingsSagas.ts | 20 +++++- .../client/src/utils/getSettingsDelta.spec.ts | 70 +++++++++++++++++++ .../app/client/src/utils/getSettingsDelta.ts | 56 +++++++++++++++ packages/app/client/src/utils/index.ts | 1 + .../app/main/src/commands/ngrokCommands.ts | 2 + .../handlers/sendTokenResponse.spec.ts | 6 ++ .../emulator/handlers/sendTokenResponse.ts | 3 + .../app/main/src/state/sagas/settingsSagas.ts | 2 +- .../src/telemetry/telemetryService.spec.ts | 16 +++-- .../main/src/telemetry/telemetryService.ts | 4 ++ 15 files changed, 243 insertions(+), 32 deletions(-) create mode 100644 packages/app/client/src/utils/getSettingsDelta.spec.ts create mode 100644 packages/app/client/src/utils/getSettingsDelta.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 35c41727a..f3938c5cb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## Added - [client/main] Added Ngrok Status Viewer in PR [2032](https://github.com/microsoft/BotFramework-Emulator/pull/2032) - [client/main] Changed conversation infrastructure to use Web Sockets to communicate with Web Chat in PR [2034](https://github.com/microsoft/BotFramework-Emulator/pull/2034) +- [client/main] Added new telemetry events and properties in PR [2063](https://github.com/microsoft/BotFramework-Emulator/pull/2063) ## Fixed - [client] Hid services pane by default in PR [2059](https://github.com/microsoft/BotFramework-Emulator/pull/2059) diff --git a/packages/app/client/src/state/sagas/azureAuthSaga.spec.ts b/packages/app/client/src/state/sagas/azureAuthSaga.spec.ts index 61842a6ee..806326e9c 100644 --- a/packages/app/client/src/state/sagas/azureAuthSaga.spec.ts +++ b/packages/app/client/src/state/sagas/azureAuthSaga.spec.ts @@ -198,7 +198,9 @@ describe('The azureAuthSaga', () => { ct++; } expect(ct).toBe(5); - expect(remoteCallSpy).toHaveBeenCalledWith(SharedConstants.Commands.Telemetry.TrackEvent, 'signIn_failure'); + expect(remoteCallSpy).toHaveBeenCalledWith(SharedConstants.Commands.Telemetry.TrackEvent, 'azure_signIn', { + success: false, + }); }); it('should contain 6 steps when the Azure login dialog prompt is confirmed and auth succeeds', async () => { @@ -267,7 +269,10 @@ describe('The azureAuthSaga', () => { } expect(ct).toBe(6); expect(store.getState().azureAuth.access_token).toBe('a valid access_token'); - expect(remoteCallSpy).toHaveBeenCalledWith(SharedConstants.Commands.Telemetry.TrackEvent, 'signIn_success'); + expect(remoteCallSpy).toHaveBeenCalledWith(SharedConstants.Commands.Telemetry.TrackEvent, 'azure_signIn', { + persistLogin: true, + success: true, + }); }); }); }); diff --git a/packages/app/client/src/state/sagas/azureAuthSaga.ts b/packages/app/client/src/state/sagas/azureAuthSaga.ts index ea4e77bed..741af9dc2 100644 --- a/packages/app/client/src/state/sagas/azureAuthSaga.ts +++ b/packages/app/client/src/state/sagas/azureAuthSaga.ts @@ -74,10 +74,12 @@ export class AzureAuthSaga { PersistAzureLoginChanged, persistLogin ); - AzureAuthSaga.commandService.remoteCall(TrackEvent, 'signIn_success').catch(_e => void 0); + AzureAuthSaga.commandService + .remoteCall(TrackEvent, 'azure_signIn', { persistLogin: !!persistLogin, success: true }) + .catch(_e => void 0); } else { yield DialogService.showDialog(action.payload.loginFailedDialog); - AzureAuthSaga.commandService.remoteCall(TrackEvent, 'signIn_failure').catch(_e => void 0); + AzureAuthSaga.commandService.remoteCall(TrackEvent, 'azure_signIn', { success: false }).catch(_e => void 0); } yield put(azureArmTokenDataChanged(azureAuth.access_token)); return azureAuth; diff --git a/packages/app/client/src/state/sagas/botSagas.ts b/packages/app/client/src/state/sagas/botSagas.ts index 241f75756..77e2472ce 100644 --- a/packages/app/client/src/state/sagas/botSagas.ts +++ b/packages/app/client/src/state/sagas/botSagas.ts @@ -168,14 +168,20 @@ export class BotSagas { // telemetry if (!action.payload.isFromBotFile) { - BotSagas.commandService.remoteCall(SharedConstants.Commands.Telemetry.TrackEvent, 'bot_open', { - numOfServices: 0, - source: 'url', - }); - } - if (!isLocalHostUrl(action.payload.endpoint)) { - BotSagas.commandService.remoteCall(SharedConstants.Commands.Telemetry.TrackEvent, 'livechat_openRemote').catch(); + BotSagas.commandService + .remoteCall(SharedConstants.Commands.Telemetry.TrackEvent, 'bot_open', { + numOfServices: 0, + source: 'url', + }) + .catch(_ => void 0); } + BotSagas.commandService + .remoteCall(SharedConstants.Commands.Telemetry.TrackEvent, 'livechat_open', { + isDebug: action.payload.mode === 'debug', + isGov: action.payload.channelService === 'azureusgovernment', + isRemote: !isLocalHostUrl(action.payload.endpoint), + }) + .catch(_ => void 0); } } diff --git a/packages/app/client/src/state/sagas/frameworkSettingsSagas.spec.ts b/packages/app/client/src/state/sagas/frameworkSettingsSagas.spec.ts index ff9da5961..5636cdafb 100644 --- a/packages/app/client/src/state/sagas/frameworkSettingsSagas.spec.ts +++ b/packages/app/client/src/state/sagas/frameworkSettingsSagas.spec.ts @@ -30,6 +30,7 @@ // OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION // WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. // + import { beginAdd, editor, @@ -40,14 +41,20 @@ import { setDirtyFlag, setFrameworkSettings, FrameworkActionType, + FrameworkSettings, SharedConstants, } from '@bfemulator/app-shared'; import { applyMiddleware, combineReducers, createStore } from 'redux'; import sagaMiddlewareFactory from 'redux-saga'; -import { put, select, takeEvery } from 'redux-saga/effects'; +import { call, put, select, takeEvery } from 'redux-saga/effects'; import { CommandServiceImpl, CommandServiceInstance } from '@bfemulator/sdk-shared'; -import { activeDocumentSelector, frameworkSettingsSagas, FrameworkSettingsSagas } from './frameworkSettingsSagas'; +import { + activeDocumentSelector, + frameworkSettingsSagas, + FrameworkSettingsSagas, + getFrameworkSettings, +} from './frameworkSettingsSagas'; jest.mock('electron', () => ({ ipcMain: new Proxy( @@ -101,31 +108,59 @@ describe('The frameworkSettingsSagas', () => { }); it('should register the expected generators', () => { - const it = frameworkSettingsSagas(); - expect(it.next().value).toEqual( + const gen = frameworkSettingsSagas(); + expect(gen.next().value).toEqual( takeEvery(FrameworkActionType.SAVE_FRAMEWORK_SETTINGS, FrameworkSettingsSagas.saveFrameworkSettings) ); }); it('should save the framework settings', async () => { - const it = FrameworkSettingsSagas.saveFrameworkSettings(saveFrameworkSettingsAction({})); + const currentSettings: Partial = { + autoUpdate: false, + useCustomId: false, + usePrereleases: false, + userGUID: '', + ngrokPath: 'some/path/to/ngrok', + }; + const updatedSettings: Partial = { + autoUpdate: true, + useCustomId: true, + usePrereleases: false, + userGUID: 'some-user-id', + ngrokPath: 'some/different/path/to/ngrok', + }; + const gen = FrameworkSettingsSagas.saveFrameworkSettings(saveFrameworkSettingsAction(updatedSettings)); // selector to get the active document from the state - const selector = it.next().value; + const selector = gen.next().value; expect(selector).toEqual(select(activeDocumentSelector)); const value = selector.SELECT.selector(mockStore.getState()); // put the dirty state to false - expect(it.next(value).value).toEqual(put(setDirtyFlag(value.documentId, false))); - expect(it.next().value).toEqual(put(setFrameworkSettings({}))); - expect(it.next().done).toBe(true); + expect(gen.next(value).value).toEqual(put(setDirtyFlag(value.documentId, false))); + expect(gen.next().value).toEqual(put(setFrameworkSettings(updatedSettings))); + expect(gen.next().value).toEqual(select(getFrameworkSettings)); + expect(gen.next(currentSettings).value).toEqual( + call( + [commandService, commandService.remoteCall], + SharedConstants.Commands.Telemetry.TrackEvent, + 'app_changeSettings', + { + autoUpdate: true, + useCustomId: true, + userGUID: 'some-user-id', + ngrokPath: 'some/different/path/to/ngrok', + } + ) + ); + expect(gen.next().done).toBe(true); }); it('should send a notification when saving the settings fails', () => { - const it = FrameworkSettingsSagas.saveFrameworkSettings(saveFrameworkSettingsAction({})); - it.next(); + const gen = FrameworkSettingsSagas.saveFrameworkSettings(saveFrameworkSettingsAction({})); + gen.next(); const errMsg = `Error while saving emulator settings: oh noes!`; const notification = newNotification(errMsg); notification.timestamp = jasmine.any(Number) as any; notification.id = jasmine.any(String) as any; - expect(it.throw('oh noes!').value).toEqual(put(beginAdd(notification))); + expect(gen.throw('oh noes!').value).toEqual(put(beginAdd(notification))); }); }); diff --git a/packages/app/client/src/state/sagas/frameworkSettingsSagas.ts b/packages/app/client/src/state/sagas/frameworkSettingsSagas.ts index 016fe0552..c21de6b1b 100644 --- a/packages/app/client/src/state/sagas/frameworkSettingsSagas.ts +++ b/packages/app/client/src/state/sagas/frameworkSettingsSagas.ts @@ -40,10 +40,13 @@ import { FrameworkAction, FrameworkActionType, FrameworkSettings, + SharedConstants } from '@bfemulator/app-shared'; -import { ForkEffect, put, select, takeEvery } from 'redux-saga/effects'; +import { ForkEffect, call, put, select, takeEvery } from 'redux-saga/effects'; +import { CommandServiceImpl, CommandServiceInstance } from '@bfemulator/sdk-shared'; import { RootState } from '../store'; +import { getSettingsDelta } from '../../utils'; export const activeDocumentSelector = (state: RootState) => { const { editors, activeEditor } = state.editor; @@ -51,7 +54,12 @@ export const activeDocumentSelector = (state: RootState) => { return editors[activeEditor].documents[activeDocumentId]; }; +export const getFrameworkSettings = (state: RootState): FrameworkSettings => state.framework; + export class FrameworkSettingsSagas { + @CommandServiceInstance() + private static commandService: CommandServiceImpl; + // when saving settings from the settings editor we need to mark the document as clean // and then set the settings public static *saveFrameworkSettings(action: FrameworkAction): IterableIterator { @@ -59,6 +67,16 @@ export class FrameworkSettingsSagas { const activeDoc: Document = yield select(activeDocumentSelector); yield put(setDirtyFlag(activeDoc.documentId, false)); // mark as clean yield put(setFrameworkSettings(action.payload)); + const currentSettings = yield select(getFrameworkSettings); + const settingsDelta = getSettingsDelta(currentSettings, action.payload); + if (settingsDelta) { + yield call( + [FrameworkSettingsSagas.commandService, FrameworkSettingsSagas.commandService.remoteCall], + SharedConstants.Commands.Telemetry.TrackEvent, + 'app_changeSettings', + settingsDelta + ); + } } catch (e) { const errMsg = `Error while saving emulator settings: ${e}`; const notification = newNotification(errMsg); diff --git a/packages/app/client/src/utils/getSettingsDelta.spec.ts b/packages/app/client/src/utils/getSettingsDelta.spec.ts new file mode 100644 index 000000000..1abbec891 --- /dev/null +++ b/packages/app/client/src/utils/getSettingsDelta.spec.ts @@ -0,0 +1,70 @@ +// +// Bot Framework Emulator Github: +// https://github.com/Microsoft/BotFramwork-Emulator +// +// Copyright (c) Microsoft Corporation +// All rights reserved. +// +// MIT License: +// Permission is hereby granted, free of charge, to any person obtaining +// a copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to +// permit persons to whom the Software is furnished to do so, subject to +// the following conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED ""AS IS"", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE +// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION +// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION +// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +// + +import { FrameworkSettings } from '@bfemulator/app-shared'; + +import { getSettingsDelta } from './getSettingsDelta'; + +describe('getSettingsDelta', () => { + it('should return an object containing the delta between 2 settings objects', () => { + const currentSettings: Partial = { + autoUpdate: true, + locale: 'en-us', + use10Tokens: true, + usePrereleases: true, + userGUID: 'some-id', + }; + const updatedSettings: Partial = { + autoUpdate: true, + runNgrokAtStartup: true, + use10Tokens: false, + usePrereleases: false, + userGUID: 'some-other-id', + }; + + expect(getSettingsDelta(currentSettings, updatedSettings)).toEqual({ + locale: undefined, + runNgrokAtStartup: true, + use10Tokens: false, + usePrereleases: false, + userGUID: 'some-other-id', + }); + }); + + it('should return undefined for settings objects that do not contain a delta', () => { + const currentSettings: Partial = { + autoUpdate: true, + use10Tokens: true, + usePrereleases: true, + userGUID: 'some-id', + }; + const updatedSettings = currentSettings; + + expect(getSettingsDelta(currentSettings, updatedSettings)).toBe(undefined); + }); +}); diff --git a/packages/app/client/src/utils/getSettingsDelta.ts b/packages/app/client/src/utils/getSettingsDelta.ts new file mode 100644 index 000000000..3459d687e --- /dev/null +++ b/packages/app/client/src/utils/getSettingsDelta.ts @@ -0,0 +1,56 @@ +// +// Copyright (c) Microsoft. All rights reserved. +// Licensed under the MIT license. +// +// Microsoft Bot Framework: http://botframework.com +// +// Bot Framework Emulator Github: +// https://github.com/Microsoft/BotFramwork-Emulator +// +// Copyright (c) Microsoft Corporation +// All rights reserved. +// +// MIT License: +// Permission is hereby granted, free of charge, to any person obtaining +// a copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to +// permit persons to whom the Software is furnished to do so, subject to +// the following conditions: +// +// The above copyright notice and this permission notice shall be +// included in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED ""AS IS"", WITHOUT WARRANTY OF ANY KIND, +// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND +// NONINFRINGEMENT. IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE +// LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION +// OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION +// WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. +// + +import { FrameworkSettings } from '@bfemulator/app-shared'; + +export function getSettingsDelta( + prevSettings: FrameworkSettings, + updatedSettings: FrameworkSettings +): Partial { + const delta: Partial = {}; + // get delta for keys present in updated settings + for (const key in updatedSettings) { + const prevVal = prevSettings[key]; + const updatedVal = updatedSettings[key]; + if (prevVal !== updatedVal) { + delta[key] = updatedVal; + } + } + // get delta for any keys that were deleted from updated settings + for (const key in prevSettings) { + if (!Object.prototype.hasOwnProperty.call(updatedSettings, key)) { + delta[key] = undefined; + } + } + return Object.keys(delta).length ? delta : undefined; +} diff --git a/packages/app/client/src/utils/index.ts b/packages/app/client/src/utils/index.ts index 59782bfb9..77eb555cd 100644 --- a/packages/app/client/src/utils/index.ts +++ b/packages/app/client/src/utils/index.ts @@ -34,5 +34,6 @@ export * from './debounce'; export * from './expandFlatTree'; export * from './getGlobal'; +export * from './getSettingsDelta'; export * from './generateBotSecret'; export * from './chatUtils'; diff --git a/packages/app/main/src/commands/ngrokCommands.ts b/packages/app/main/src/commands/ngrokCommands.ts index 46475d249..15f9cdbb1 100644 --- a/packages/app/main/src/commands/ngrokCommands.ts +++ b/packages/app/main/src/commands/ngrokCommands.ts @@ -36,6 +36,7 @@ import { Command } from '@bfemulator/sdk-shared'; import { store } from '../state/store'; import { Emulator } from '../emulator'; +import { TelemetryService } from '../telemetry'; const Commands = SharedConstants.Commands.Ngrok; @@ -48,6 +49,7 @@ export class NgrokCommands { try { await emulator.ngrok.recycle(); emulator.ngrok.broadcastNgrokReconnected(); + TelemetryService.trackEvent('ngrok_reconnect'); } catch (e) { throw new Error(`There was an error while trying to reconnect ngrok: ${e}`); } diff --git a/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.spec.ts b/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.spec.ts index e68ca0613..ba29f5fef 100644 --- a/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.spec.ts +++ b/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.spec.ts @@ -35,6 +35,12 @@ import * as HttpStatus from 'http-status-codes'; import { sendTokenResponse } from './sendTokenResponse'; +jest.mock('../../../../telemetry', () => ({ + TelemetryService: { + trackEvent: jest.fn(), + }, +})); + describe('sendTokenResponse handler', () => { it('should send a 200 if the result of sending the token was a 200', async () => { const req: any = { diff --git a/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.ts b/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.ts index 0a09d33be..7ae10c2f9 100644 --- a/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.ts +++ b/packages/app/main/src/server/routes/emulator/handlers/sendTokenResponse.ts @@ -34,6 +34,8 @@ import * as HttpStatus from 'http-status-codes'; import { Next, Response } from 'restify'; +import { TelemetryService } from '../../../../telemetry'; + import { ConversationAwareRequest } from './getConversation'; export async function sendTokenResponse(req: ConversationAwareRequest, res: Response, next: Next): Promise { @@ -46,6 +48,7 @@ export async function sendTokenResponse(req: ConversationAwareRequest, res: Resp if (statusCode === HttpStatus.OK) { res.send(HttpStatus.OK, body); + TelemetryService.trackEvent('oauth_sendToken'); } else { res.send(statusCode); } diff --git a/packages/app/main/src/state/sagas/settingsSagas.ts b/packages/app/main/src/state/sagas/settingsSagas.ts index e916b647e..63668f919 100644 --- a/packages/app/main/src/state/sagas/settingsSagas.ts +++ b/packages/app/main/src/state/sagas/settingsSagas.ts @@ -68,7 +68,7 @@ export class SettingsSagas { public static *setFramework(action: FrameworkAction): IterableIterator { const emulator = Emulator.getInstance(); yield emulator.ngrok.updateNgrokFromSettings(action.payload); - //emulator.framework.server.botEmulator.facilities.locale = action.payload.locale; + emulator.server.state.locale = action.payload.locale; yield* SettingsSagas.pushClientAwareSettings(); } diff --git a/packages/app/main/src/telemetry/telemetryService.spec.ts b/packages/app/main/src/telemetry/telemetryService.spec.ts index 2783e4e5a..70cc477ee 100644 --- a/packages/app/main/src/telemetry/telemetryService.spec.ts +++ b/packages/app/main/src/telemetry/telemetryService.spec.ts @@ -112,7 +112,7 @@ describe('TelemetryService', () => { }); it('should not track events if disabled or if no name is provided', () => { - const mockAITrackEvent = jest.fn((_name, _properties) => null); + const mockAITrackEvent = jest.fn(() => null); (TelemetryService as any)._client = { trackEvent: mockAITrackEvent }; mockSettings = { framework: { collectUsageData: false } }; @@ -124,12 +124,11 @@ describe('TelemetryService', () => { expect(mockAITrackEvent).not.toHaveBeenCalled(); }); - // NOTE: Disabled for v4.3 - /* it('should track events', () => { + global['__JEST_ENV__'] = false; const mockStartup = jest.fn(() => null); (TelemetryService as any).startup = mockStartup; - const mockAutoCollect = jest.fn(_config => mockAppInsights); + const mockAutoCollect = jest.fn(() => mockAppInsights); mockAppInsights = { setAutoCollectConsole: mockAutoCollect, setAutoCollectDependencies: mockAutoCollect, @@ -137,15 +136,18 @@ describe('TelemetryService', () => { setAutoCollectPerformance: mockAutoCollect, setAutoCollectRequests: mockAutoCollect, }; - const mockAITrackEvent = jest.fn((_name, _properties) => null); + const mockAITrackEvent = jest.fn(() => null); (TelemetryService as any)._client = { trackEvent: mockAITrackEvent }; TelemetryService.trackEvent('someEvent', { some: 'property' }); expect(mockStartup).toHaveBeenCalled; expect(mockAITrackEvent).toHaveBeenCalledWith({ name: 'someEvent', - properties: { some: 'property' }, + properties: { + some: 'property', + toolName: 'bf-emulator', + }, }); + global['__JEST_ENV__'] = true; }); - */ }); diff --git a/packages/app/main/src/telemetry/telemetryService.ts b/packages/app/main/src/telemetry/telemetryService.ts index bb2e02266..eae534716 100644 --- a/packages/app/main/src/telemetry/telemetryService.ts +++ b/packages/app/main/src/telemetry/telemetryService.ts @@ -50,6 +50,10 @@ export class TelemetryService { this.startup(); } try { + properties = { + ...properties, + toolName: 'bf-emulator', + }; this._client.trackEvent({ name, properties }); } catch (e) { // swallow the exception; we don't want to crash the app