-
Notifications
You must be signed in to change notification settings - Fork 357
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
Conversation
{ | ||
Log.Logger?.ReportInfo(Log.Component.Orchestrator, "Cancelling flow"); | ||
var currentPage = Orchestrator.CurrentPageViewModel.GetType().Name; | ||
TerminateCurrentFlow($"CancelButton_{currentPage}"); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
/azp run |
Azure Pipelines successfully started running 1 pipeline(s). |
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