Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Adaptive Runtime to add telemetry client to the TurnState #5509

Merged
merged 2 commits into from Apr 27, 2021

Conversation

garypretty
Copy link
Contributor

Fixes #5504

Description

PR to ensure adaptive dialog events are being logged correctly.

Specific Changes

  • Ensures the IBotTelemetryClient is added to the TurnState, which then ensures adaptive dialog events are correctly captured.
  • Amends the value of dialogId captured on the AdaptiveDialogAction event to use the parent dialog Id.
  • Adds an Instance Id property to the AdaptiveDialogStart / AdaptiveDialogComplete / AdaptiveDialogAction events to ensure they can be correlated within App Insights queries (this brings parity with waterfall dialog telemetry events).

…o amend the AdaptiveDialogAction telemetry event to use correct dialog Id and log dialog instance Id
@garypretty garypretty requested review from a team as code owners April 21, 2021 14:11
@@ -56,6 +56,8 @@ public async Task AdaptiveDialogBotTurnState()

var turnContext = new TurnContext(adapterMock.Object, activity);

var telemetryClient = new NullBotTelemetryClient();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we see a test where if instanceId is null ActionScope:323, nothing breaks? This is my main concern with this PR

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we test that we'd be safe.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@garypretty
Copy link
Contributor Author

Removed instanceId addition within this PR as requested

@johnataylor
Copy link
Member

How about having the telemetry middleware add the telemetry client to the turn state? there are some advantages to taking that approach

@johnataylor johnataylor self-requested a review April 26, 2021 20:12
Copy link
Member

@johnataylor johnataylor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets evaluate adding the telemetry client to the turn state from the telemetry middleware

(I have a DRAFT pr that plays with this idea. It has the advantage of keeping the telemetry stuff basically in one place, however, fails to model the dependency the AdaptiveDialog has on IBotTelemetryClient - so I'm not sure its any better)

@johnataylor
Copy link
Member

@carlosscastro @garypretty this is what the alternative might look like #5535

@johnataylor
Copy link
Member

johnataylor commented Apr 26, 2021

What you are saying here is that an AdaptiveDialog depends on a a IBotTelemetryClient. Is that what you intend?

So assuming it is, then by extension the Dependency Injection mechanism is pulled in. In which case take a look at the AddBotRuntimeTelemetry function in the ServicesCollectionExtensions.

What would make things more consistent would be to remove the lines:

            if (string.IsNullOrEmpty(telemetrySettings?.Options?.ConnectionString) && string.IsNullOrEmpty(telemetrySettings?.Options?.InstrumentationKey))
            {
                services.AddSingleton<IBotTelemetryClient, NullBotTelemetryClient>();
            }

After all you are applying the default in your constructor.

And then replace the AddSingleton with a TryAddSingleton (line 139) and then edit the lambda to apply the defaulting to NullBotTelemetryClient if the GetService returns null (using these inline lambdas is not great - it just shows we haven't modeled the dependencies.) and then get rid of this lambda in R14

So I'm thinking I actually quite like this change - because its modeling the dependency explicitly - however I do think we should fix up AddBotRuntimeTelemetry as part of this PR to make things consistent.

@johnataylor
Copy link
Member

I think fixing the AddBotRuntimeTelemetry is too bigger change at this point

@johnataylor johnataylor merged commit 6e1ee07 into main Apr 27, 2021
@johnataylor johnataylor deleted the garyp/adaptive-telemetry branch April 27, 2021 20:15
EricDahlvang pushed a commit that referenced this pull request Apr 27, 2021
* Update Adaptive Runtime to add telemetry client to the TurnState.  Also amend the AdaptiveDialogAction  telemetry event to use correct dialog Id and log dialog instance Id

* Remove instanceId implementation and tracking for adaptive dialogs
mrivera-ms pushed a commit that referenced this pull request Apr 27, 2021
…) (#5548)

* Update Adaptive Runtime to add telemetry client to the TurnState.  Also amend the AdaptiveDialogAction  telemetry event to use correct dialog Id and log dialog instance Id

* Remove instanceId implementation and tracking for adaptive dialogs

Co-authored-by: Gary Pretty <gary@garypretty.co.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Runtime is not logging dialog telemetry events
3 participants