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

Add setup flow termination event #383

Merged
merged 1 commit into from
May 10, 2023
Merged

Add setup flow termination event #383

merged 1 commit into from
May 10, 2023

Conversation

florelis
Copy link
Member

@florelis florelis commented May 9, 2023

Summary of the pull request

Adding telemetry events for leaving the setup flow, and a related activity ID to correlate it with the flow start event.

References and relevant issues

https://task.ms/44490929
#340

Detailed description of the pull request / Additional comments

Also moved the existing event files and changed their namespaces to try and give them more order.

Validation steps performed

Manually confirmed events are sent with TRTT

PR checklist

  • Closes #xxx
  • Tests added/passed
  • Documentation updated

{
Log.Logger?.ReportInfo(Log.Component.Orchestrator, "Cancelling flow");
var currentPage = Orchestrator.CurrentPageViewModel.GetType().Name;
TerminateCurrentFlow($"CancelButton_{currentPage}");
Copy link
Contributor

Choose a reason for hiding this comment

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

These have me wondering, does it make sense for us to also hook an event handler to the main app window's on close event, where we can check if the user is in the setup flow, and if so log a SetupFlow_Termination telemetry event?

Not for this PR, but did PM ever say anything about if we need to time the duration users take to get from main page to summary page?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't remember hearing about getting the time from main page to summary page, but we can compute it from other events since they come with the time they were emitted. I think there is an event with how many tasks succeeded/failed; we could use that and the start flow event to measure time taken.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would indeed make sense to add a termination event on windows close. But instead of doing it to do it just for the telemetry event, we would do it to add the blocking pop up to confirm closing. I know it has been mentioned multiple times, but I'm not sure if there is a work item for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see makes sense, we can bring it up to Sharla in the next setup flow sync up, to see if it's something they'd want for post build

@florelis
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@florelis florelis merged commit 4eebb7e into microsoft:main May 10, 2023
@florelis florelis deleted the telemetryEvents branch May 22, 2023 18:58
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.

3 participants