Skip to content

Revisit how status is displayed to the user #247

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

Closed
6 tasks
HowardWolosky opened this issue Jun 26, 2020 · 0 comments · Fixed by #253
Closed
6 tasks

Revisit how status is displayed to the user #247

HowardWolosky opened this issue Jun 26, 2020 · 0 comments · Fixed by #253
Labels
discussion We are looking for additional community feedback on this topic before proceeding further. enhancement An issue or pull request introducing new functionality to the project.

Comments

@HowardWolosky
Copy link
Member

HowardWolosky commented Jun 26, 2020

Feature Idea Summary

The initial idea of having status/progress displayed to the user for each REST request was an attempt to alleviate concern from the user that they might not know what's going on or why their request is taking so long.

As @X-Guardian determined though with #206, it turns out that using status ends up increasing the duration of the commands by a significant margin (see details below).

It seems like it might be time to revisit how this all works.

Proposal (pending some Telemetry review on actual accumulated run times of the various commands):

  • For Invoke-GHRestMethod, I think we should probably just drop status in general. I think that most GitHub REST requests take under 1 second right now, so we're not really gaining a lot of user value by the status feature.
  • For Invoke-GHRestMethodMultipleResult, I think in the end it's not achieving the intended value. The goal here was to inform the user that we're still trying to get results. The issue is that we don't know how many results there are, so we can't give a real percentage-complete status to the user.
    • If there is a way to determine how many requests will be necessary so that we can display the percent complete, we should
    • If not, we should consider maybe just doing things silently and possibly outputting an Informational message every 10 seconds or so to inform the user that we're still collecting results.

Actions:

  • Verify from telemetry that most commands are running < 1 second.
  • Determine if there's any way to know how many requests a multiple-request method is going to take
  • Simplify Invoke-GHRestMethod and Invoke-SendTelemetryEvent to no longer have bifurcating execution paths depending on NoStatus
  • Update Invoke-GHRestMethodMultipleResult to either:
    • Display a true percentage completed to the user
    • Or - Write an Informational message to the user every x seconds of execution (currently thinking maybe every 10 seconds?)

I think we don't do anything about NoStatus for now in the functions/configuration. We can leave that in for now and just have it be ignored. After this is in for a while, if we feel that it's working well this new way, then we can deprecate the switch/configuration (possibly leaving it in the function signatures to avoid the breaking change, but never referencing it anywhere else).

Feature Idea Additional Details

# Get the current user
## With status
(1..20 |% { Measure-Command { Get-GitHubUser -Current } }).TotalSeconds | Measure-Object -Average
# 1.187 seconds

## Without status
(1..20 |% { Measure-Command { Get-GitHubUser -Current -NoStatus } }).TotalSeconds | Measure-Object -Average
# 0.1899 seconds (6.2 times faster)

#----------------------

# Get _this_ issue
## With status
(1..20 |% { Measure-Command { Get-GitHubIssue -OwnerName microsoft -RepositoryName PowerShellForGitHub -Issue 247 } }).TotalSeconds | Measure-Object -Average
# 1.191 seconds

## Without status
(1..20 |% { Measure-Command { Get-GitHubIssue -OwnerName microsoft -RepositoryName PowerShellForGitHub -Issue 247 -NoStatus }).TotalSeconds | Measure-Object -Average
# 0.238 seconds (5 times faster)

#----------------------

# Get all of the teams in the microsoft org (~2100 teams at the moment)
## With status
(1..20 |% { Measure-Command { $null = Get-GitTeam -OrganizationName microsoft} }).TotalSeconds | Measure-Object -Average
# 97.5 seconds

## Without status
(1..20 |% { Measure-Command { $null = Get-GitTeam -OrganizationName microsoft -NoStatus} }).TotalSeconds | Measure-Object -Average
# 22.327 seconds (4.3 times faster)

Requested Assignment

I'm just suggesting this idea, but don't want to implement it.

Operating System

OsName               : Microsoft Windows 10 Pro
OsOperatingSystemSKU : 48
OsArchitecture       : 64-bit
WindowsVersion       : 1909
WindowsBuildLabEx    : 18362.1.amd64fre.19h1_release.190318-1202
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

PowerShell Version

Name                           Value
----                           -----
PSVersion                      5.1.18362.752
PSEdition                      Desktop
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0...}
BuildVersion                   10.0.18362.752
CLRVersion                     4.0.30319.42000
WSManStackVersion              3.0
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1

Module Version

Running: 0.14.0
Installed:
@HowardWolosky HowardWolosky added enhancement An issue or pull request introducing new functionality to the project. discussion We are looking for additional community feedback on this topic before proceeding further. labels Jun 26, 2020
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 29, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 29, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 29, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 29, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 29, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 30, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit to HowardWolosky/PowerShellForGitHub that referenced this issue Jun 30, 2020
Status for commands was originally added to this module based on
my experience with other REST API's where individual commands could
easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take
under one second.  The additional work that PowerShell has to do in
order to display progress to the user can easily make the overall
command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most
part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no
longer have bifurcating execution paths based on the value of
`$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands
  in order to avoid a breaking change.  It may be removed in a future
  update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include
  the next page's number and the total number of pages for the
  REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold`
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the
  user tracking the number of remaining requests for the overall
  execution of the requested command based on this threshold value.  It
  will only display the progress bar if the number of requets needed
  meets or exceeds this threshold value.  This defaults to 10, and can
  be disabled with a value of 0.  Practical usage has shown that this
  adds less than a second of additional time to the overall execution
  of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes microsoft#247
HowardWolosky added a commit that referenced this issue Jun 30, 2020
* Reimagining status for the module

Status for commands was originally added to this module based on my experience with other REST API's where individual commands could easily take 10-20 seconds.

Practical usage has shown that most GitHub requests in reality take under one second.  The additional work that PowerShell has to do in order to display progress to the user can easily make the overall command take 4-6 times longer than its actual execution time.

Therefore, status is being ripped out of this module (for the most part).  `Invoke-GHRestMethod` and `Invoke-SendTelemetryEvent` no longer have bifurcating execution paths based on the value of `$NoStatus`.  Everything runs synchronously now on the command prompt.

* `DefaultNoStatus` has been deprecated.  Its value will be ignored.
* The `NoStatus` switch has not been removed from the module commands in order to avoid a breaking change.  It may be removed in a future update.
* `Invoke-GHRestMethod -ExtendedResult` has been updated to include the next page's number and the total number of pages for the REST request.
* A new configuration value has been added: `MultiRequestProgressThreshold
  `Invoke-GHRestMethodMultipleResult` will display a ProgressBar to the user tracking the number of remaining requests for the overall execution of the requested command based on this threshold value.  It will only display the progress bar if the number of requets needed meets or exceeds this threshold value.  This defaults to 10, and can be disabled with a value of 0.  
  Practical usage has shown that this adds less than a second of additional time to the overall execution of a multi-request command (quite different than the previous status).
* `Wait-JobWithAnimation` has been removed since it's no longer used.

Fixes #247
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion We are looking for additional community feedback on this topic before proceeding further. enhancement An issue or pull request introducing new functionality to the project.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant