From 77c49e312b1feed298e16e4fb349e9c522bd19ac Mon Sep 17 00:00:00 2001 From: Steven Gum <14935595+stevengum@users.noreply.github.com> Date: Wed, 6 May 2020 18:15:43 -0700 Subject: [PATCH] Cherry-pick #2145, #2190 and #2196 to master (#2192) * Add Dialog / PageView telemetry (#2145) * Update app insights package version * Add PageView logging * move TelemetryView helper to botTelemetryClient.ts * remove telemetry Extensions class, harden helper method, add tests Co-authored-by: Steven Gum <14935595+stevengum@users.noreply.github.com> * move botbuilder-lg and adaptive-expressions out of preview (#2190) * move botbuilder-lg and adaptive-expressions out of preview * correct set-version script in preview packages * restore preview packages to using Version instead of PreviewPackageVersion * Fixed issues with AdaptiveSkillDialog (#2196) and added unit tests Co-authored-by: Gary Pretty Co-authored-by: Steven Ickman --- .../package.json | 2 +- .../src/applicationInsightsTelemetryClient.ts | 8 +- .../botbuilder-core/src/botTelemetryClient.ts | 42 ++++- .../tests/botTelemetryClient.test.js | 46 ++++++ .../src/skills/adaptiveSkillDialog.ts | 31 ++-- .../tests/adaptiveSkillDialog.test.js | 152 ++++++++++++++++++ .../botbuilder-dialogs/src/componentDialog.ts | 4 +- .../botbuilder-dialogs/src/dialogManager.ts | 6 +- .../botbuilder-dialogs/src/waterfallDialog.ts | 6 +- package.json | 1 + .../util/updateDependenciesInPackageJsons.js | 4 +- 11 files changed, 275 insertions(+), 27 deletions(-) create mode 100644 libraries/botbuilder-core/tests/botTelemetryClient.test.js create mode 100644 libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js diff --git a/libraries/botbuilder-applicationinsights/package.json b/libraries/botbuilder-applicationinsights/package.json index 946bdde983..0c39d70f95 100644 --- a/libraries/botbuilder-applicationinsights/package.json +++ b/libraries/botbuilder-applicationinsights/package.json @@ -21,7 +21,7 @@ "main": "./lib/index.js", "typings": "./lib/index.d.ts", "dependencies": { - "applicationinsights": "1.2.0", + "applicationinsights": "1.7.5", "botbuilder-core": "4.1.6", "cls-hooked": "^4.2.2" }, diff --git a/libraries/botbuilder-applicationinsights/src/applicationInsightsTelemetryClient.ts b/libraries/botbuilder-applicationinsights/src/applicationInsightsTelemetryClient.ts index ab643ebaed..59258af47b 100644 --- a/libraries/botbuilder-applicationinsights/src/applicationInsightsTelemetryClient.ts +++ b/libraries/botbuilder-applicationinsights/src/applicationInsightsTelemetryClient.ts @@ -6,7 +6,7 @@ * Licensed under the MIT License. */ import * as appInsights from 'applicationinsights'; -import { Activity, BotTelemetryClient, TelemetryDependency, TelemetryEvent, TelemetryException, TelemetryTrace } from 'botbuilder-core'; +import { Activity, BotTelemetryClient, BotPageViewTelemetryClient, TelemetryDependency, TelemetryEvent, TelemetryException, TelemetryTrace, TelemetryPageView } from 'botbuilder-core'; import * as cls from 'cls-hooked'; import * as crypto from 'crypto'; const ns: any = cls.createNamespace('my.request'); @@ -61,7 +61,7 @@ export const ApplicationInsightsWebserverMiddleware: any = (req: any, res: any, * myDialog.telemetryClient = appInsightsClient; * ``` */ -export class ApplicationInsightsTelemetryClient implements BotTelemetryClient { +export class ApplicationInsightsTelemetryClient implements BotTelemetryClient, BotPageViewTelemetryClient { private client: appInsights.TelemetryClient; private config: appInsights.Configuration; @@ -117,6 +117,10 @@ export class ApplicationInsightsTelemetryClient implements BotTelemetryClient { this.defaultClient.trackTrace(telemetry as appInsights.Contracts.TraceTelemetry); } + public trackPageView(telemetry: TelemetryPageView): void { + this.defaultClient.trackPageView(telemetry as appInsights.Contracts.PageViewTelemetry); + } + public flush(): void { this.defaultClient.flush(); } diff --git a/libraries/botbuilder-core/src/botTelemetryClient.ts b/libraries/botbuilder-core/src/botTelemetryClient.ts index b2a10ced35..db613ae6b6 100644 --- a/libraries/botbuilder-core/src/botTelemetryClient.ts +++ b/libraries/botbuilder-core/src/botTelemetryClient.ts @@ -27,6 +27,10 @@ export interface BotTelemetryClient { flush(); } +export interface BotPageViewTelemetryClient { + trackPageView(telemetry: TelemetryPageView); +} + export interface TelemetryDependency { dependencyTypeName: string; target: string; @@ -57,11 +61,21 @@ export interface TelemetryTrace { severityLevel?: Severity; } -export class NullTelemetryClient implements BotTelemetryClient { +export interface TelemetryPageView { + name: string; + properties?: {[key: string]: string}; + metrics?: {[key: string]: number }; +} + +export class NullTelemetryClient implements BotTelemetryClient, BotPageViewTelemetryClient { constructor(settings?: any) { // noop } + + trackPageView(telemetry: TelemetryPageView) { + // noop + } trackDependency(telemetry: TelemetryDependency) { // noop @@ -74,6 +88,7 @@ export class NullTelemetryClient implements BotTelemetryClient { trackException(telemetry: TelemetryException) { // noop } + trackTrace(telemetry: TelemetryTrace) { // noop } @@ -81,5 +96,28 @@ export class NullTelemetryClient implements BotTelemetryClient { flush() { // noop } +} + +export function telemetryTrackDialogView(telemetryClient: BotTelemetryClient, dialogName: string, properties?: {[key: string]: any}, metrics?: {[key: string]: number }): void { + if (!clientSupportsTrackDialogView(telemetryClient)) { + throw new TypeError('"telemetryClient" parameter does not have methods trackPageView() or trackTrace()'); + } + if (instanceOfBotPageViewTelemetryClient(telemetryClient)) { + telemetryClient.trackPageView({ name: dialogName, properties: properties, metrics: metrics }); + } + else { + telemetryClient.trackTrace({ message: 'Dialog View: ' + dialogName, severityLevel: Severity.Information } ); + } +} + +function instanceOfBotPageViewTelemetryClient(object: any): object is BotPageViewTelemetryClient { + return 'trackPageView' in object; +} -} \ No newline at end of file +function clientSupportsTrackDialogView(client: any): boolean { + if (!client) { return false; } + if (typeof client.trackPageView !== 'function' && typeof client.trackTrace !== 'function') { + return false; + } + return true; +} diff --git a/libraries/botbuilder-core/tests/botTelemetryClient.test.js b/libraries/botbuilder-core/tests/botTelemetryClient.test.js new file mode 100644 index 0000000000..626e5ae240 --- /dev/null +++ b/libraries/botbuilder-core/tests/botTelemetryClient.test.js @@ -0,0 +1,46 @@ +const { ok, strictEqual } = require('assert'); +const { Severity, telemetryTrackDialogView } = require('../'); + +describe('BotTelemetryClient', function() { + this.timeout(3000); + + describe('"telemetryTrackDialogView" helper', () => { + it('should call client.trackPageView if it exists', () => { + const testClient = { + trackPageView({ name, properties, metrics }) { + ok(name); + ok(properties); + ok(metrics); + } + }; + const testProps = { description: 'value' }; + const testMetrics = { duration: 1 }; + telemetryTrackDialogView(testClient, 'dialogName', testProps, testMetrics); + }); + + it('should call client.trackTrace if trackPageView is not supported', () => { + const testClient = { + trackTrace({ message, severityLevel }) { + ok(message); + strictEqual(severityLevel, Severity.Information); + } + }; + telemetryTrackDialogView(testClient, 'dialogName'); + }); + + it('should throw TypeError if trackTrace and trackPageView do not exist', () => { + try { + telemetryTrackDialogView(undefined, 'dialogName'); + } catch (err) { + strictEqual(err.message, '"telemetryClient" parameter does not have methods trackPageView() or trackTrace()'); + } + + try { + telemetryTrackDialogView({}, 'dialogName'); + } catch (err) { + strictEqual(err.message, '"telemetryClient" parameter does not have methods trackPageView() or trackTrace()'); + } + + }); + }); +}); diff --git a/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts b/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts index 8cc7118811..605e0ae75c 100644 --- a/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts +++ b/libraries/botbuilder-dialogs-adaptive/src/skills/adaptiveSkillDialog.ts @@ -8,9 +8,10 @@ import { SkillDialog, SkillDialogOptions, DialogContext, DialogTurnResult, DialogManager, BeginSkillDialogOptions } from 'botbuilder-dialogs'; import { BoolExpression, StringExpression } from 'adaptive-expressions'; import { TemplateInterface } from '../template'; -import { Activity, ActivityTypes } from 'botbuilder-core'; +import { Activity, ActivityTypes, BotFrameworkClient, SkillConversationIdFactoryBase } from 'botbuilder-core'; -const GLOBAL_SKILL_OPTIONS = Symbol('globalSkillOptions'); +const SKILL_CLIENT = Symbol('skillClient'); +const CONVERSATION_ID_FACTORY = Symbol('conversationIdFactory'); export class AdaptiveSkillDialog extends SkillDialog { @@ -83,13 +84,6 @@ export class AdaptiveSkillDialog extends SkillDialog { return await dc.endDialog(); } - // Update options with global options - const globalOptions = dc.context.turnState.get(GLOBAL_SKILL_OPTIONS); - if (globalOptions) { - this.dialogOptions = Object.assign(globalOptions, this.dialogOptions); - } - if (!this.dialogOptions.conversationState) { dc.dialogManager.conversationState } - // Setup the skill to call const botId = this.botId.getValue(dcState); const skillHostEndpoint = this.skillHostEndpoint.getValue(dcState); @@ -98,6 +92,9 @@ export class AdaptiveSkillDialog extends SkillDialog { if (this.skillAppId) { this.dialogOptions.skill.id = this.dialogOptions.skill.appId = this.skillAppId.getValue(dcState) } if (this.skillEndpoint) { this.dialogOptions.skill.skillEndpoint = this.skillEndpoint.getValue(dcState) } if (this.connectionName) { this.dialogOptions.connectionName = this.connectionName.getValue(dcState) } + if (!this.dialogOptions.conversationState) { this.dialogOptions.conversationState = dc.dialogManager.conversationState } + if (!this.dialogOptions.skillClient) { this.dialogOptions.skillClient = dc.context.turnState.get(SKILL_CLIENT) } + if (!this.dialogOptions.conversationIdFactory) { this.dialogOptions.conversationIdFactory = dc.context.turnState.get(CONVERSATION_ID_FACTORY) } // Get the activity to send to the skill. options = {} as BeginSkillDialogOptions; @@ -126,12 +123,20 @@ export class AdaptiveSkillDialog extends SkillDialog { return await super.continueDialog(dc); } + protected onComputeId(): string { + const appId = this.skillAppId ? this.skillAppId.toString() : ''; + const activity = this.activity ? this.activity.toString() : ''; + return `Skill[${appId}:${activity}]`; + } + /** - * Configures the initial skill dialog options to use. + * Configures the skill client and conversation ID factory to use. * @param dm DialogManager to configure. - * @param options Skill dialog options to use. + * @param skillClient Skill client instance to use. + * @param conversationIdFactory Conversation ID factory to use. */ - static setGlobalSkillOptions(dm: DialogManager, options: SkillDialogOptions): void { - dm.initialTurnState.set(GLOBAL_SKILL_OPTIONS, options); + static setSkillHostOptions(dm: DialogManager, skillClient: BotFrameworkClient, conversationIdFactory: SkillConversationIdFactoryBase): void { + dm.initialTurnState.set(SKILL_CLIENT, skillClient); + dm.initialTurnState.set(CONVERSATION_ID_FACTORY, conversationIdFactory); } } \ No newline at end of file diff --git a/libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js b/libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js new file mode 100644 index 0000000000..b7a86db742 --- /dev/null +++ b/libraries/botbuilder-dialogs-adaptive/tests/adaptiveSkillDialog.test.js @@ -0,0 +1,152 @@ +const { ok, strictEqual } = require('assert'); +const { createHash } = require('crypto'); +const { stub } = require('sinon'); +const { + ActivityTypes, + ConversationState, + MemoryStorage, + TestAdapter, + SkillConversationIdFactoryBase, + StatusCodes, + TurnContext +} = require('botbuilder-core'); +const { BoolExpression, StringExpression } = require('adaptive-expressions'); +const { DialogManager, DialogTurnStatus } = require('botbuilder-dialogs'); +const { AdaptiveSkillDialog } = require('../') + + +class SimpleConversationIdFactory extends SkillConversationIdFactoryBase { + constructor(config = { disableCreateWithOpts: false, disableGetSkillRef: false }) { + super(); + this._conversationRefs = new Map(); + this.disableCreateWithOpts = config.disableCreateWithOpts; + this.disableGetSkillRef = config.disableGetSkillRef; + } + + async createSkillConversationIdWithOptions(opts) { + if (this.disableCreateWithOpts) { + return super.createSkillConversationIdWithOptions(); + } + } + + async createSkillConversationId(options) { + const key = createHash('md5').update(options.activity.conversation.id + options.activity.serviceUrl).digest('hex'); + + const ref = this._conversationRefs.has(key); + if (!ref) { + this._conversationRefs.set(key, { + conversationReference: TurnContext.getConversationReference(options.activity), + oAuthScope: options.fromBotOAuthScope + }); + } + return key; + } + + async getConversationReference() { + + } + + async getSkillConversationReference(skillConversationId) { + return this._conversationRefs.get(skillConversationId); + } + + deleteConversationReference() { + + } +} + +describe('SkillDialog', function() { + this.timeout(3000); + + let activitySent; // Activity + let fromBotIdSent; // string + let toBotIdSent; // string + let toUriSent; // string (URI) + + // Callback to capture the parameters sent to the skill + const captureAction = (fromBotId, toBotId, toUri, serviceUrl, conversationId, activity) => { + // Capture values sent to the skill so we can assert the right parameters were used. + fromBotIdSent = fromBotId; + toBotIdSent = toBotId; + toUriSent = toUri; + activitySent = activity; + } + + // Create BotFrameworkHttpClient and postActivityStub + const [skillClient, postActivityStub] = createSkillClientAndStub(captureAction); + + // Setup dialog manager + const conversationState = new ConversationState(new MemoryStorage()); + const dm = new DialogManager(); + dm.conversationState = conversationState; + AdaptiveSkillDialog.setSkillHostOptions(dm, skillClient, new SimpleConversationIdFactory()); + + // Setup skill dialog + const dialog = new AdaptiveSkillDialog(); + setSkillDialogOptions(dialog); + dm.rootDialog = dialog; + + it('should call skill via beginDialog()', async () => { + // Send initial activity + const adapter = new TestAdapter(async (context) => { + const { turnResult } = await dm.onTurn(context); + + // Assert results and data sent to the SkillClient for first turn + strictEqual(fromBotIdSent, 'SkillCallerId'); + strictEqual(toBotIdSent, 'SkillId'); + strictEqual(toUriSent, 'http://testskill.contoso.com/api/messages'); + strictEqual(activitySent.text, 'test'); + strictEqual(turnResult.status, DialogTurnStatus.waiting); + ok(postActivityStub.calledOnce); + }); + + await adapter.send('test'); + }); +}); + +function setSkillDialogOptions(dialog) { + dialog.disabled = new BoolExpression(false); + dialog.botId = new StringExpression('SkillCallerId'); + dialog.skillHostEndpoint = new StringExpression('http://test.contoso.com/skill/messages'); + dialog.skillAppId = new StringExpression('SkillId'); + dialog.skillEndpoint = new StringExpression('http://testskill.contoso.com/api/messages'); +} + +/** + * @remarks + * captureAction should match the below signature: + * `(fromBotId: string, toBotId: string, toUrl: string, serviceUrl: string, conversationId: string, activity: Activity) => void` + * @param {Function} captureAction + * @param {StatusCodes} returnStatusCode Defaults to StatusCodes.OK + * @returns [BotFrameworkHttpClient, postActivityStub] + */ +function createSkillClientAndStub(captureAction, returnStatusCode = StatusCodes.OK) { + // This require should not fail as this method should only be called after verifying that botbuilder is resolvable. + const { BotFrameworkHttpClient } = require('../../botbuilder'); + + if (captureAction && typeof captureAction !== 'function') { + throw new TypeError(`Failed test arrangement - createSkillClientAndStub() received ${typeof captureAction} instead of undefined or a function.`); + } + + // Create ExpectedReplies object for response body + const dummyActivity = { type: ActivityTypes.Message, attachments: [], entities: [] }; + dummyActivity.text = 'dummy activity'; + const activityList = { activities: [dummyActivity] }; + + // Create wrapper for captureAction + function wrapAction(...args) { + captureAction(...args); + return { status: returnStatusCode, body: activityList }; + } + // Create client and stub + const skillClient = new BotFrameworkHttpClient({}); + const postActivityStub = stub(skillClient, 'postActivity'); + + if (captureAction) { + postActivityStub.callsFake(wrapAction); + } else { + postActivityStub.returns({ status: returnStatusCode, body: activityList }); + } + + return [ skillClient, postActivityStub ]; +} \ No newline at end of file diff --git a/libraries/botbuilder-dialogs/src/componentDialog.ts b/libraries/botbuilder-dialogs/src/componentDialog.ts index 0551618d77..4b4a78a356 100644 --- a/libraries/botbuilder-dialogs/src/componentDialog.ts +++ b/libraries/botbuilder-dialogs/src/componentDialog.ts @@ -5,7 +5,7 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { TurnContext, BotTelemetryClient, NullTelemetryClient } from 'botbuilder-core'; +import { BotTelemetryClient, NullTelemetryClient, telemetryTrackDialogView, TurnContext } from 'botbuilder-core'; import { Dialog, DialogInstance, DialogReason, DialogTurnResult, DialogTurnStatus } from './dialog'; import { DialogContext } from './dialogContext'; import { DialogContainer } from './dialogContainer'; @@ -81,6 +81,8 @@ export class ComponentDialog extends DialogContainer { public async beginDialog(outerDC: DialogContext, options?: O): Promise { await this.checkForVersionChange(outerDC); + telemetryTrackDialogView(this.telemetryClient, this.id); + // Start the inner dialog. const innerDC: DialogContext = this.createChildContext(outerDC) const turnResult: DialogTurnResult = await this.onBeginDialog(innerDC, options); diff --git a/libraries/botbuilder-dialogs/src/dialogManager.ts b/libraries/botbuilder-dialogs/src/dialogManager.ts index 9ad9e013cf..b3f76fb98b 100644 --- a/libraries/botbuilder-dialogs/src/dialogManager.ts +++ b/libraries/botbuilder-dialogs/src/dialogManager.ts @@ -111,9 +111,9 @@ export class DialogManager extends Configurable { if (!this.rootDialogId) { throw new Error(`DialogManager.onTurn: the bot's 'rootDialog' has not been configured.`); } // Copy initial turn state to context - for (const key in this.initialTurnState.keys()) { - context.turnState.set(key, this.initialTurnState.get(key)); - } + this.initialTurnState.forEach((value, key) => { + context.turnState.set(key, value); + }); const botStateSet = new BotStateSet(); diff --git a/libraries/botbuilder-dialogs/src/waterfallDialog.ts b/libraries/botbuilder-dialogs/src/waterfallDialog.ts index 68e439de8e..4df21078a2 100644 --- a/libraries/botbuilder-dialogs/src/waterfallDialog.ts +++ b/libraries/botbuilder-dialogs/src/waterfallDialog.ts @@ -5,8 +5,8 @@ * Copyright (c) Microsoft Corporation. All rights reserved. * Licensed under the MIT License. */ -import { ActivityTypes } from 'botbuilder-core'; -import { TurnContext } from 'botbuilder-core'; +import { ActivityTypes, Severity } from 'botbuilder-core'; +import { TurnContext, telemetryTrackDialogView } from 'botbuilder-core'; import { DialogInstance } from './dialog'; import { Dialog, DialogReason, DialogTurnResult } from './dialog'; import { DialogContext } from './dialogContext'; @@ -151,6 +151,8 @@ export class WaterfallDialog extends Dialog { 'InstanceId': state.values['instanceId'] }}); + telemetryTrackDialogView(this.telemetryClient, this.id); + // Run the first step return await this.runStep(dc, 0, DialogReason.beginCalled); } diff --git a/package.json b/package.json index 06aa085895..92d88cd1bc 100644 --- a/package.json +++ b/package.json @@ -26,6 +26,7 @@ "@types/lodash": "^4.14.134", "@typescript-eslint/eslint-plugin": "^1.10.2", "@typescript-eslint/parser": "^1.10.2", + "applicationinsights": "^1.7.5", "coveralls": "^3.0.4", "eslint": "^5.16.0", "eslint-plugin-only-warn": "^1.0.1", diff --git a/tools/util/updateDependenciesInPackageJsons.js b/tools/util/updateDependenciesInPackageJsons.js index 313ae2f4c7..25e34d597a 100644 --- a/tools/util/updateDependenciesInPackageJsons.js +++ b/tools/util/updateDependenciesInPackageJsons.js @@ -12,9 +12,7 @@ var previewVersion = myArgs[2] || process.env.PreviewPackageVersion; if(previewVersion === 'adaptive-expressions') { previewVersion = undefined; } -var previewPackages = { - 'botbuilder-lg': true, - 'adaptive-expressions': true, +const previewPackages = { 'botbuilder-dialogs-adaptive': true, 'botbuilder-dialogs-adaptive-tests': true, 'botbuilder-dialogs-declarative': true