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 $ErrorView with simplified views for Error Messages #228

Open
wants to merge 36 commits into
base: master
from

Conversation

@theJasonHelmick
Copy link

commented Sep 26, 2019

No description provided.

theJasonHelmick and others added 30 commits Jul 31, 2019
Co-Authored-By: Steve Lee <slee@microsoft.com>
@iSazonov

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

I see that discussion about $ErrorView is finished and we are ready to accept the RFC. So I suggest to split cmdlets discussion to new RFC.

I think we should resolve main problem with errors before continue the discussion - recognizing terminating and non-terminating errors.
As for error cmdlets it looks like a surplus. All we need is already available to process the $error collection. If we need we could add new views. All that we need for UX - a completer for -View parameter.


3. A new cmdlet `Resolve-ErrorRecord` will produce comprehensive detailed
view of the fully qualified error, including nested inner exceptions.

This comment has been minimized.

Copy link
@dragonwolf83

dragonwolf83 Oct 8, 2019

Instead of defaulting to nested inner exceptions, surface GetBaseException. This is the most inner exception and typically will be what people will want from my experience. I did this with SQL Server APIs and it gave me the best details without having to see all of the nested exceptions.

You can do this today by doing, $_.Exception.GetBaseException().

A [switch] parameter can be used to resolve the full nested inner exceptions for those that still find value in showing all of them.

This comment has been minimized.

Copy link
@KirkMunro

KirkMunro Oct 8, 2019

Contributor

@dragonwolf83: I think that should be additive, not instead of. Provide a view that shows the base exception. Also provide a view that shows nested inner exceptions.

This comment has been minimized.

Copy link
@dragonwolf83

dragonwolf83 Oct 8, 2019

With ConciseView done, you already have a view without base exception. This suggestion is to be additive by including the Base Exception in the next detailed view before going all in with nested exceptions.

Quite often, Base Exception is the same as the Outer Exception. The two exceptions could be compared to see if it should be surfaced or not for a "smarter" view. I don't know how that would impact performance though.

@KirkMunro

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

I see that discussion about $ErrorView is finished and we are ready to accept the RFC. So I suggest to split cmdlets discussion to new RFC.

Do you mean make one RFC just about $ErrorView by itself, and have another about cmdlets for managing $Error? Doing that means the PowerShell Team would have to agree that showing error information with nested exceptions (one of the views they want to have available) is just another view, not something that you need to get from a Resolve-ErrorRecord cmdlet.

@iSazonov

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

Yes. Current formatting system is very power and can showing nested exception too. With $error[0] I want to see all I need. Also I want to get $error[0].<tab> working.

The on-screen experience, when receiving an error message,
is controlled through the views NormalView (the default) and CategoryView. These are user selectable
through the preference variable $ErrorView.
This RFC describes Changing '$ErrorView to an enumeration and adding one additional dynamic view

This comment has been minimized.

Copy link
@KirkMunro

KirkMunro Oct 8, 2019

Contributor

This RFC describes Changing $ErrorView to an enumeration

You're making a mistake by proposing an enumeration be used for $ErrorView, because you're cutting off anyone who wants to build custom error views that way. Also, by using an enumeration, you're forcing yourself to stick with the View suffix to maintain backward compatibility with CategoryView.

You shouldn't do either.

Instead:

  • Leave $ErrorView as a string so that anyone who wants to customize their error message views can do so without having to submit a PR and go through a review and approval process.
  • Drop the View suffix because it's redundant, and by sticking with $ErrorView as a string, you can drop that suffix without breaking backward compatibility.
  • When the $ErrorView value is set, if the incoming value is CategoryView or NormalView, drop the View suffix automatically.
  • When the $ErrorView value is set, if it doesn't match a view, or if it is $null, fall back to Normal (the default).

This approach allows you to remove the redundant suffix, maintain backwards compatibility, and continue offering support to users who want to customize their error view.

@tnieto88

This comment has been minimized.

Copy link

commented Oct 8, 2019

This cmdlet should NOT be called Resolve-ErrorRecord or Resolve-Error due to the conflict with the Az module. Furthermore, it should NOT use the Resolve verb or noun ErrorRecord. The cmdlet should be called Get-Error like @KirkMunro describes. I'll explain my reasoning below.

The reasons I'm hearing that it should use the verb Resolve is to allow it to change the type name to allow for a different default format definition without changing the underlying type to be a formatting object. There is already a perfect example on how do this properly without the workarounds and working against PowerShell conventions and best practices.

Get-Process has two different default format definitions depending on the the parameters used, first is the default and the second is to include usernames. To get the username included in the format a -IncludeUsername switch parameter is used to change the type name from System.Diagnostics.Process to System.Diagnostics.Process#IncludeUserName. The format definitions then define a unique format for that type. The exact same thing can happen here, a change to the type name to append #ConciseView or any of the other built-in or custom format definitions. Then you can have a -View parameter on Get-Error with argument completer to list valid views.

The cmdlet should NOT use the noun ErrorRecord to match the existing cmdlets in use today namely Write-Error. First Record is not useful since users will not know the underlying type of the object, second the error variable is called $Error not $ErrorRecord, third its redundant, fourth Resolve noun is only used by one built-in cmdlet while Get is in 300+. The cmdlet should use Error noun to to match existing error cmdlets Write-Error as well as any new cmdlet in the future like Clear-Error or Remove-Error.

To recap Get-Error adheres to the cmdlet naming convention already defined with Write-Error. The -View parameter changes the type name to append #ViewName to allow for different default format definitions without changing the underlying type.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

I don't think Get is the appropriate verb. Agreed on the rest. 🙂

If anything, I think we should go for Format-Error or Show-Error. My preference is on the latter, but I know others prefer Format.

@tnieto88

This comment has been minimized.

Copy link

commented Oct 8, 2019

@vexx32 would Format-Error get a list of ErrorRecord objects to display on its own or would a Get-Error or $Error be the source? The reason I ask is the other Format cmdlets do nothing without input from another source (cmdlet/variable).

My personal preference would be Format instead of Show since this determining which format definition to use. Traditionally Show has been used for to show a GUI application instead of being related to the F&O system.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

@tnieto88 the current implementation of Resolve-ErrorRecord that @SteveL-MSFT has been showing off has required input iirc, e.g., $Error[0] | Resolve-ErrorRecord

I would imagine this would be the same for Format-Error. :)

@tnieto88

This comment has been minimized.

Copy link

commented Oct 8, 2019

@vexx32 yeah you're right. :)

I think I thought that because of the -Newest and -All parameters. I believe best practice would have the error record object filtered before reaching Format-Error so, that cmdlet can have one single purpose, to define the format definition.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2019

I would agree with that, and to that end perhaps there is room for a Get-Error command to help filter it first, then output the ErrorRecords matching the query.

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Oct 10, 2019

@vexx32 one correction, current implementation doesn't require input, if you just run Resolve-ErrorRecord or the preferred alias rve, then it will automatically use $error[0].

I agree that the collision with Resolve-Error is problematic. At this point, not sure if I like Format-Error or Get-Error (or something else). The noun shouldn't be ErrorRecord as $error can have Exceptions not wrapped as ErrorRecords, but we had to choose it due to Az taking Resolve-Error.

One of the capabilities I'd like to have in the future is a way to register troubleshooters that will run with Resolve-ErrorRecord for specific types of Exceptions for FQErrorId that can do some diagnostics and add additional information as a note property. This makes Resolve a better verb in that case, in my opinion.

@joeyaiello

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

Initial (absolutely non-binding) thoughts on the naming:

  • ErrorRecord feels like overkill on the noun. By default it's grabbing the latest item off of $Error, feels like an error.
  • Not understanding the collision with Azure PowerShell, they use Resolve-AzureRmError/Resolve-AzError
  • Resolve seems to have worked well for Azure PS. I get that it does technically add new information, but I'd argue that it's exposing so much information that many folks likely didn't even know how to access (via Format-* -Force) that it's in the same vein of usefulness as other Resolve cmdlets.
  • I'm actually pretty good with Get-Error. I can't think of anything else I'd expect that cmdlet to do if I saw it in a MenuComplete. It wouldn't expect it to look up an exception type, nor would I expect it to throw anything.
  • Someone threw out redefining Show-*. Given our GUI work in Out-GridView, the momentum of projects like Avalonia, and .NET bringing back WPF/WinForms for Windows, I think we should reserve Show for its original intent. Long term, I think we'll be happy we didn't overload it.
  • Format doesn't make any sense to me because the noun in Format-Foo is the type of formatting being applied: tabular formatting, list formatting, custom formatting, etc. Not that this is useful, but I'd expect a Format-ErrorView to try and wrangle an object it receives into the error view formatter.

So yeah, I'm basically a coin flip between Get-Error and Resolve-Error right now.

@vexx32

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

@joeyaiello I believe Azure PowerShell has a default alias for Resolve-AzError that is called Resolve-Error. @doctordns already ran into this once or twice testing out @SteveL-MSFT's changes. 🙂

@TobiasPSP

This comment has been minimized.

Copy link

commented Oct 11, 2019

Get-Error sounds like a great choice to me, it describes very accurately what is done by the cmdlet. I would argue against Resolve-Error so we don‘t close the door on any initiative that eventually really does resolve an error, i.e. via a public error db/wiki, AI, or whatnot 😀

@KirkMunro

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2019

  • Format doesn't make any sense to me because the noun in Format-Foo is the type of formatting being applied: tabular formatting, list formatting, custom formatting, etc. Not that this is useful, but I'd expect a Format-ErrorView to try and wrangle an object it receives into the error view formatter.

On this point @joeyaiello, combined with your thinking about Get-Error and Format-Error:

  • Historically Get is used to retrieve information. It isn't used to format that information.
  • Format-Custom is technically the more appropriate Format command here, because you would use it to choose a custom view; however, Format-Custom doesn't say much. That's why Format-Error was proposed -- it's more discoverable (common noun, and appropriate verb), easier to remember, and would basically just proxy Format-Custom, while ensuring that the input data was of the appropriate type. It is worth asking: do we want users to get used to Format-Custom like they have gotten used to Format-Table/-List/-Wide? Or is it better to offer specific formatters like Format-Error, so you can use generalized formatters used for any object, or specific formatters for objects that have a specific formatting cmdlet? I think because of discoverability, and because the cmdlet could raise an invalid argument error indicating it could only be used to format error records returned from Get-Error or $Error, that would be a very small hurdle for someone to get over...more like the tinest of speed bumps.

Also, any thoughts on a collection of -Error commands, as proposed earlier in the discussion?

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Oct 11, 2019

After some further considerations, I'm personally leaning towards Get-Error (with alias gerr). As for other suggested sub-features, we can add them to Future Considerations section in the RFC and add as we identify user feedback that there is an actual need for them.

@dragonwolf83

This comment has been minimized.

Copy link

commented Oct 12, 2019

I'm not in favor of the Get-Error or Resolve-Error approach. I think the concept of Views was a good idea and should be expanded upon.

The current approach of another command is considering one user story for interactive use cases, but it doesn't consider non-interactive, logging scenarios.

As it stands, I don't see how I can have a great experience in both without custom handling each use case.

Do I need to always use Resolve-Error in my catch to get that detail? I actually wrote my own Resolve-Error a few years ago and did this. If it gets nested and caught by an outer exception, It puts the entire resolved output as the message, then wrapped a new Resolve-Error I did that so that I would have all the details logged, but it gets unwieldy very fast.

Motivation

As an interactive user, I want concise, readable, colored output to easily scroll through and act upon.

As an interactive user, I want to switch between concise and detailed output after the fact so that I can dig into harder to understand errors.

As a non-interactive task, I want to log the detailed output to a file so that I can review when there is an issue. The error will not be available after the fact to switch between views.

Alternate Proposals

Add the detail from Resolve/Get-Error as an additional view. This can be 2 new views, one with the BaseException and the other with full nested inner exceptions, or just surface the BaseException more often.

Allow contributing custom error views so that I can specify my own level of detail to bring in and make default.

Add a new PreferenceVariable that will set the ErrorView defaults for Interactive and Logging separately. This way, my script can just set the default when redirecting to file or using Out-File. Detailed output should be default. (Extend views to all streams to allow for standardized logging...)

Use Format-Error in interactive scenarios to show the specific error in a different view. This way, I can have ConciseView as default and when I want detail just run:

# Pass in error
$error[0] | Format-Error -View Detailed

# Default to $error[0] so you can just run the cmdlet
Format-Error -View Detailed` 

# Default to $error[0] and Detailed view so you can just run
Format-Error

All views should show FullyQualfiedErrorId so that we can filter an error for that id

$Error | Where FullyQualifiedErrorId -eq "ID1"
@doctordns

This comment has been minimized.

Copy link

commented Oct 12, 2019

As @vexx32 noted, I got slightly confused over the cmdlet name (expecting to test a new cmdlet and instead, executing a similarly named AZ function name.

I think resolve is not a great verb. What are you actually resolving? I think of Resolve-DNSName and Resolve-Path which do resolve things. Displaying a fuller error message is not really, in my view, it not a resolution. Format- or even Get- seem more appropriate verbs.

Whatever is decided, I suspect the AZ team should re-consider this alias.

@doctordns

This comment has been minimized.

Copy link

commented Oct 12, 2019

Get-Error works for me. Would that cmdlet have a -ErrorView parameter to say which view should be used in getting the Errors?

@SteveL-MSFT

This comment has been minimized.

Copy link
Member

commented Oct 12, 2019

For me, the intent of this cmdlet is to get as much info about an error as possible to figure out what the problem is. If there is a -View parameter, I don't think it would map to $ErrorView and would depend on what people think they need. For example, it might make sense to have a Flat view instead of indented view.

@joeyaiello joeyaiello added this to the 7.0-Consider milestone Oct 14, 2019
@joeyaiello

This comment has been minimized.

Copy link
Member

commented Oct 14, 2019

Get-Error is not "a formatting cmdlet". It literally gets an error (or errors) off of the error stack, and then we're applying a new default formatter. In my mind, it's more like Get-Variable.

@PowerShell/powershell-committee agree that we want to go with Get-Error with @theJasonHelmick to update the RFC here.

We also agree on the Azure PS collision, I wasn't aware they were sitting on that alias. We'll bring it up with them next time we sync, I suspect most of the Resolve-Error usage is interactive.


- `Get-Error` will provide the following:

- Display the newest Error ($Error[0]) – default behavior

This comment has been minimized.

Copy link
@felixfbecker

felixfbecker Oct 15, 2019

Nit: In PowerShell, formatting and data are different concerns. Get-Error should not display the newest error, but get or return the newest error. The display should be handled through a formatting view (e.g. a CustomControl in a Format.ps1xml file).

This comment has been minimized.

Copy link
@KirkMunro

KirkMunro Oct 15, 2019

Contributor

That's what is actually happening. You can see the implementation in this PR that was just merged into PowerShell: PowerShell/PowerShell#10727.

This comment has been minimized.

Copy link
@theJasonHelmick

theJasonHelmick Oct 18, 2019

Author

Thanks for the reference @KirkMunro. @felixfbecker did this address your concern?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.