Adaptive Telemetry Instrumentation #3491
Conversation
| /// </summary> | ||
| /// <value>The <see cref="IBotTelemetryClient"/> being used to log events.</value> | ||
| [JsonIgnore] | ||
| public IBotTelemetryClient TelemetryClient { get; set; } = new NullBotTelemetryClient(); |
There was a problem hiding this comment.
public IBotTelemetryClient TelemetryClient { get; set; } = new NullBotTelemetryClient() [](start = 8, length = 87)
Do we have to add properties for this? Why not dialogContext.TurnState.Get()?
There was a problem hiding this comment.
The TelemetryClient isn't currently added to TurnState. I saw your suggestion regarding having the middleware add it, but that telemetry middleware is currently optional and we want to ensure this telemetry lights up for folks who already have it enabled when they start to adopt adaptive.
There was a problem hiding this comment.
This also keeps the telemetry implementation consistent across the SDK. Any developers who are currently using telemetry may be using the telemetry client to log trace / custom events etc. It's better, in my opinion, to keep that consistent so they can continue to use the same approach.
| @@ -82,6 +82,7 @@ public Dialog RootDialog | |||
| if (value != null) | |||
| { | |||
| this.rootDialogId = value.Id; | |||
| this.Dialogs.TelemetryClient = value.TelemetryClient; | |||
There was a problem hiding this comment.
this.Dialogs.TelemetryClient = value.TelemetryClient; [](start = 19, length = 54)
?? Is this right?
There was a problem hiding this comment.
Yes, the underlying DialogSet needs to have the TelemetryClient set, after which point it will automatically assign the client to any dialogs that are added.
| return await wrapper.RecognizeAsync(tempContext, cancellationToken).ConfigureAwait(false); | ||
| var result = await wrapper.RecognizeAsync(tempContext, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| this.TelemetryClient.TrackEvent("LuisResult", this.FillRecognizerResultTelemetryProperties(result, telemetryProperties, dialogContext.Context), telemetryMetrics); |
There was a problem hiding this comment.
this.TelemetryClient [](start = 9, length = 23)
Why not dialogContext.TurnState.Get() ?
There was a problem hiding this comment.
As per above, The TelemetryClient isn't currently added to TurnState. I saw your suggestion regarding having the middleware add it, but that telemetry middleware is currently optional and we want to ensure this telemetry lights up for folks who already have it enabled when they start to adopt adaptive.
| @@ -212,6 +212,8 @@ public override async Task<RecognizerResult> RecognizeAsync(DialogContext dialog | |||
| recognizerResult.Intents.Add("None", new IntentScore() { Score = 1.0f }); | |||
| } | |||
|
|
|||
| this.TelemetryClient.TrackEvent("QnAMakerRecognizerResult", this.FillRecognizerResultTelemetryProperties(recognizerResult, telemetryProperties), telemetryMetrics); | |||
There was a problem hiding this comment.
this.TelemetryClient [](start = 11, length = 21)
Why not dialogContext.TurnState.Get()?
There was a problem hiding this comment.
As per response above.
| return ProcessResults(results); | ||
| var result = ProcessResults(results); | ||
|
|
||
| this.TelemetryClient.TrackEvent("CrossTrainedRecognizerSetResult", this.FillRecognizerResultTelemetryProperties(result, telemetryProperties), telemetryMetrics); |
There was a problem hiding this comment.
As per response above.
… trigger events. Fixed comment breaking build.
Addressed #2406
Adds the P0 telemetry for adaptive - covering dialog start / cancel / complete, triggers, actions and adaptive recognizers.