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.

Sign up for GitHub

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

Jump to bottom

Add LG telemetry #3469

Merged
merged 3 commits into from
Mar 6, 2020
Merged

Add LG telemetry #3469

merged 3 commits into from
Mar 6, 2020

Conversation

Danieladu
Copy link
Collaborator

@Danieladu Danieladu commented Feb 29, 2020 •
edited

ref： #2783
add LG telemetry client.

@Danieladu
Copy link
Collaborator Author

I found IBotTelemetryClient would be obsolete in this pr #3440.
Should this PR wait for the above PR and use the new class LogTelemetryClient?

@vishwacsena
Copy link
Contributor

@Danieladu yes, this should follow what @garypretty is doing for all up telemetry/ adaptive telemetry. @garypretty can you please add relevant PR #s to this for reference?

@garypretty
Copy link
Contributor

We are just finalising the details of the other PR today. Once we have done that I'll reflect here.

@tomlm tomlm added the WIP label Mar 2, 2020
@garypretty
Copy link
Contributor

@Danieladu @vishwacsena IBotTelemetryClient will now remain. The decision has been taken to extend this interface.

@Danieladu Danieladu marked this pull request as ready for review March 5, 2020 01:38
@Danieladu Danieladu removed the Draft PR label Mar 5, 2020
tomlm
tomlm approved these changes Mar 5, 2020
View reviewed changes
Copy link
Contributor

@tomlm tomlm left a comment

Choose a reason for hiding this comment

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

:shipit:

@vishwacsena
Copy link
Contributor

@garypretty can you take a look and approve if this is in line with your changes to instrument adaptive dialogs?

@vishwacsena vishwacsena added LG P0 Must Fix. Release-blocker R8 Release 8 - March 16th, 2020 labels Mar 6, 2020
@vishwacsena vishwacsena added this to LG, expressions in Adaptive Mar 6, 2020
@vishwacsena vishwacsena removed this from LG, expressions in Adaptive Mar 6, 2020
@vishwacsena
Copy link
Contributor

This is the adaptive PR https://github.com/microsoft/botbuilder-dotnet/pull/3491/files

@boydc2014
Copy link
Contributor

We should just leverage the same telemetry client inside action (IDialog) and log the generation behavior in dialog level, and leave LG alone for now

boydc2014
boydc2014 requested changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@boydc2014 boydc2014 left a comment

Choose a reason for hiding this comment

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

See comments above

@Danieladu Danieladu requested a review from boydc2014 March 6, 2020 06:36
garypretty
garypretty requested changes Mar 6, 2020
View reviewed changes
Copy link
Contributor

@garypretty garypretty left a comment •
edited

Choose a reason for hiding this comment

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

All of the events tracked in this PR appear to be logging the result of a call to .BindToData - I think we should move the tracking into the .BindToData methods - this way we automatically get tracking in the future as LG is used elsewhere in the solution.

Scratch the above, I just realised we don't have access to the TelemetryClient within BindToData - this might be something we can address in the future, but for now the changes look ok to me.

@vishwacsena - I am curious as to what the scenario is that we think folks might want to see these events - I just want to validate that TrackEvent is the right thing to do, or would TrackTrace be more appropriate? It feels like this information is going to be accessed during diagnostic / debug sceanrios, or do you think there is some aspect of this data that we would surface on a dashboard in the future as we do with recognizer results / dialog events? I am ok either way, I just want to validate.

@garypretty garypretty self-requested a review March 6, 2020 09:53
@boydc2014 boydc2014 removed the Draft PR label Mar 6, 2020
@boydc2014 boydc2014 merged commit e4076f1 into master Mar 6, 2020
This was referenced Mar 6, 2020
Closed
Closed
Closed
@boydc2014 boydc2014 deleted the hond/telemetry branch March 6, 2020 12:23
@boydc2014 boydc2014 removed LG P0 Must Fix. Release-blocker R8 Release 8 - March 16th, 2020 labels Mar 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@boydc2014 boydc2014 boydc2014 approved these changes

@garypretty garypretty garypretty approved these changes

@tomlm tomlm tomlm approved these changes

@vishwacsena vishwacsena Awaiting requested review from vishwacsena

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@Danieladu @vishwacsena @garypretty @boydc2014 @tomlm