Fixes for QnA / Recognizer Adaptive telemetry #3715

garypretty · 2020-04-09T14:57:00Z

Adds UseTelemetry extension method for DiaogManager which adds IBotTelemetryClient to turn state for use by adaptive recognizers.
Adaptive recognizers will now use the IBotTelemetryClient on the turn state unless the TelemetryClient property on the recognizer has been set.
Adds LogPersonalInformation property to QnAMaker dialogs and LUIS / QnA adaptive recognizers. Adaptive LUIS / QnA will look in settings.telemetry.logPersonalInformation by default.
Ensures QnAMaker client is always passed a TelemetryClient if set, along with the LogPersonalInformation flag (Fixes QnAMakerDialog should create QnAMaker with more parameters #3472)
LUIS Adaptive recognizer: Ensure telemetry properties passed in can override default properties

@cwhitten @vishwacsena Once these changes have been approved, I can raise an issue for composer to detail the changes needed in the runtime for telemetry.

…ould be null (#3536)

…3539) (#3542) * fix lack of a generic typed method for posting activities to skills * remove duplicated code Co-authored-by: Swagat Mishra <swagatm@microsoft.com> Co-authored-by: swagat mishra <swagatmishra2007@gmail.com> Co-authored-by: Swagat Mishra <swagatm@microsoft.com>

#3543) * Added check to only cancel dialogs in RunAsync when EoC comes from a parent bot (root or skill). * Applied URI prefix to caller ID as per OBI spec https://github.com/microsoft/botframework-obi/blob/master/protocols/botframework-activity/botframework-activity.md#bot-calling-skill (cherry picked from commit 3730963)

…3571) [OAuth]do not set signInLink to empty when OAuthAppCredentials is set

vishwacsena · 2020-04-10T05:00:52Z

libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/LUIS/LuisAdaptiveRecognizer.cs

+        /// The flag to indicate in personal information should be logged in telemetry.
+        /// </value>
+        [JsonProperty("logPersonalInformation")]
+        public BoolExpression LogPersonalInformation { get; set; } = "=settings.telemetry.logPersonalInformation";


For the new property, we need an update to the component schema file as well as re-gen the sdk.schema.

tomlm · 2020-04-15T16:18:53Z

libraries/Microsoft.Bot.Builder.Dialogs/Recognizer.cs


-        protected virtual Dictionary<string, string> FillRecognizerResultTelemetryProperties(RecognizerResult recognizerResult, Dictionary<string, string> telemetryProperties, ITurnContext turnContext = null)
+        protected virtual Dictionary<string, string> FillRecognizerResultTelemetryProperties(RecognizerResult recognizerResult, Dictionary<string, string> telemetryProperties, DialogContext dialogContext = null)
        {


Was this shipped in 4.8? I would think this is a breaking change to change this signature.
I doubt you need the DC, you just sneed the ITurnContext.

It was. I had misunderstood the way the expression value worked, which means that the current implementation doesn't get the right application ID, so I think I do need the dialog context unless there is another way to get the value ->

botbuilder-dotnet/libraries/Microsoft.Bot.Builder.Dialogs.Adaptive/LUIS/LuisAdaptiveRecognizer.cs

Lines 140 to 156 in 0c85976

protected override Dictionary<string, string> FillRecognizerResultTelemetryProperties(RecognizerResult recognizerResult, Dictionary<string, string> telemetryProperties, DialogContext dc)

{

var (logPersonalInfo, error) = this.LogPersonalInformation.TryGetValue(dc.State);

var (applicationId, error2) = this.ApplicationId.TryGetValue(dc.State);

var topTwoIntents = (recognizerResult.Intents.Count > 0) ? recognizerResult.Intents.OrderByDescending(x => x.Value.Score).Take(2).ToArray() : null;

// Add the intent score and conversation id properties

var properties = new Dictionary<string, string>()

{

{ LuisTelemetryConstants.ApplicationIdProperty, applicationId },

{ LuisTelemetryConstants.IntentProperty, topTwoIntents?[0].Key ?? string.Empty },

{ LuisTelemetryConstants.IntentScoreProperty, topTwoIntents?[0].Value.Score?.ToString("N2") ?? "0.00" },

{ LuisTelemetryConstants.Intent2Property, (topTwoIntents?.Count() > 1) ? topTwoIntents?[1].Key ?? string.Empty : string.Empty },

{ LuisTelemetryConstants.IntentScore2Property, (topTwoIntents?.Count() > 1) ? topTwoIntents?[1].Value.Score?.ToString("N2") ?? "0.00" : "0.00" },

{ LuisTelemetryConstants.FromIdProperty, dc.Context.Activity.From.Id },

};

If I do need DialogContext, given that adaptive isn't GA yet and we have not documented any of the telemetry bits yet, I think this is an acceptable change for us to make here. Thoughts?

tomlm

🕐

libraries/Microsoft.Bot.Builder.Dialogs/Recognizer.cs

…class.

gabog

Reviewed with the SDK team

garypretty · 2020-04-16T13:10:43Z

Based on the review from the SDK team, we have decided that given the method with the altered signature is only currently used by the Adaptive recognizers, and is essentially only used internally, we will make the change.

Tom Laird-McConnell and others added 9 commits March 10, 2020 12:46

Fix AdaptiveTesting to have correct preview labels

e808a72

Proactive auth: trust incoming token url conditionally (#3533)

81ad47b

Add unit test for CancelAllDialogs and fix eventName and eventValue c…

4aee5e9

…ould be null (#3536)

Merge pull request #3564 from microsoft/tedlee/fix-oauth-credentials (#…

1e99d5e

…3571) [OAuth]do not set signInLink to empty when OAuthAppCredentials is set

Address QnAMaker / recognizer telemetry across dialogs / adaptive

d6e022f

Merge latest master into branch

2d1d12a

Fixes following merge from master

0c85976

garypretty requested a review from tomlm April 9, 2020 15:44

vishwacsena reviewed Apr 10, 2020

View reviewed changes

cwhitten added the adaptive label Apr 15, 2020

tomlm reviewed Apr 15, 2020

View reviewed changes

tomlm previously requested changes Apr 15, 2020

View reviewed changes

tomlm reviewed Apr 15, 2020

View reviewed changes

libraries/Microsoft.Bot.Builder.Dialogs/Recognizer.cs Show resolved Hide resolved

garypretty added 2 commits April 15, 2020 18:41

Added comments to the TrackRecognizerResult method on the Recognizer …

1a3fd47

…class.

More comments.

fb89716

gabog approved these changes Apr 15, 2020

View reviewed changes

Merge latest master into branch

e9c0e86

garypretty requested review from johnataylor and mrivera-ms as code owners April 16, 2020 09:14

mrivera-ms approved these changes Apr 16, 2020

View reviewed changes

johnataylor approved these changes Apr 16, 2020

View reviewed changes

garypretty merged commit dabc4f4 into master Apr 16, 2020

garypretty deleted the gary/adaptive-qna-telemetry branch April 16, 2020 16:44

matiasroldan6 mentioned this pull request Sep 14, 2020

[PORT] logPersonalInformation flag missing in QnAMakerDialog microsoft/botbuilder-js#2779

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes for QnA / Recognizer Adaptive telemetry #3715

Fixes for QnA / Recognizer Adaptive telemetry #3715

Uh oh!

garypretty commented Apr 9, 2020 •

edited

Loading

Uh oh!

vishwacsena Apr 10, 2020 •

edited

Loading

Uh oh!

tomlm Apr 15, 2020 •

edited

Loading

Uh oh!

garypretty Apr 15, 2020

Uh oh!

tomlm left a comment

Uh oh!

Uh oh!

gabog left a comment

Uh oh!

garypretty commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

	protected override Dictionary<string, string> FillRecognizerResultTelemetryProperties(RecognizerResult recognizerResult, Dictionary<string, string> telemetryProperties, DialogContext dc)
	{
	var (logPersonalInfo, error) = this.LogPersonalInformation.TryGetValue(dc.State);
	var (applicationId, error2) = this.ApplicationId.TryGetValue(dc.State);

	var topTwoIntents = (recognizerResult.Intents.Count > 0) ? recognizerResult.Intents.OrderByDescending(x => x.Value.Score).Take(2).ToArray() : null;

	// Add the intent score and conversation id properties
	var properties = new Dictionary<string, string>()
	{
	{ LuisTelemetryConstants.ApplicationIdProperty, applicationId },
	{ LuisTelemetryConstants.IntentProperty, topTwoIntents?[0].Key ?? string.Empty },
	{ LuisTelemetryConstants.IntentScoreProperty, topTwoIntents?[0].Value.Score?.ToString("N2") ?? "0.00" },
	{ LuisTelemetryConstants.Intent2Property, (topTwoIntents?.Count() > 1) ? topTwoIntents?[1].Key ?? string.Empty : string.Empty },
	{ LuisTelemetryConstants.IntentScore2Property, (topTwoIntents?.Count() > 1) ? topTwoIntents?[1].Value.Score?.ToString("N2") ?? "0.00" : "0.00" },
	{ LuisTelemetryConstants.FromIdProperty, dc.Context.Activity.From.Id },
	};

Fixes for QnA / Recognizer Adaptive telemetry #3715

Fixes for QnA / Recognizer Adaptive telemetry #3715

Uh oh!

Conversation

garypretty commented Apr 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishwacsena Apr 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomlm Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

garypretty Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

tomlm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gabog left a comment

Choose a reason for hiding this comment

Uh oh!

garypretty commented Apr 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

garypretty commented Apr 9, 2020 •

edited

Loading

vishwacsena Apr 10, 2020 •

edited

Loading

tomlm Apr 15, 2020 •

edited

Loading