[Core] New error output #16257

houk-ms · 2020-12-14T07:11:09Z

Description

This PR applies the new error output to support the feature requests #15693.

1. The UI changes of new error output includes

the new output UI designs
removing error types
multiple recommended commands (at most 3 commands + 1 reference link)
descriptions of the recommended commands
colorizations
command group level hyperlinks

An overview of the new error output.

2. The none-UI changes

Set core.disable_error_recommendation=true with az config to disable recommendations
The recommended commands are re-ordered by parameter matching with user's input
The recommended commands are eithe all from Aladdin service or all from examples, in avoid of duplicate commands
Contextual values are disabled to replace the placeholders as it will make command description not valid.

Testing Guide

Type any command that will triger the client side userfault. E.g, the command below, you shold see the output above.

az vm create -n test-vm

Disable the error recommendation and then test the same command, you could see the recommendations are disabled.

az config set core.error_recommendation=off
az vm create -n test-vm

houk-ms · 2020-12-14T07:11:40Z

Wait for service's readiness.

yonzhan · 2020-12-14T07:41:04Z

Which parts are pending on Service?

houk-ms · 2020-12-15T07:39:06Z

Which parts are pending on Service?

Aladdin service's new response :)

houk-ms · 2020-12-15T07:55:07Z

Ready for review.

Aladdin endpoint is for testing now and needs to be replaced.

houk-ms · 2020-12-15T08:32:18Z

@chenlomis for awareness and possible comments.

@christopher-o-toole

houk-ms · 2020-12-15T08:36:17Z

/azp run

azure-pipelines · 2020-12-15T08:36:30Z

Azure Pipelines successfully started running 2 pipeline(s).

chenlomis · 2020-12-15T18:40:26Z

@chenlomis for awareness and possible comments.

@christopher-o-toole

Very cool! Thank you for putting this together!

Quick questions:

What's the diff for the following 2 commands aside from the fact that one has optional tag and the other doesn't?
Is there an easy way to dedupe them by any chance?

houk-ms · 2020-12-16T02:53:38Z

Quick questions:

What's the diff for the following 2 commands aside from the fact that one has optional tag and the other doesn't?
Is there an easy way to dedupe them by any chance?

Thanks @chenlomis for the comments. These are the recommended commands from Aladdin service, @christopher-o-toole may want to say somthing here?

For me, I think they are not duplicate commands for users. Because for every command, it always has a minimum set of arguments that could make it work. And all other argument combinations need to include these necessary arguments. We just do more advanced customizations with more arguments.

Take vm create as an example, we have many help examples shown to users, while they all have different scenarios.

    Create a default Ubuntu VM with automatic SSH authentication.
        az vm create -n MyVm -g MyResourceGroup --image UbuntuLTS


    Create a default RedHat VM with automatic SSH authentication using an image URN.
        az vm create -n MyVm -g MyResourceGroup --image RedHat:RHEL:7-RAW:7.4.2018010506


    Create a default Windows Server VM with a private IP address.
        az vm create -n MyVm -g MyResourceGroup --public-ip-address "" --image Win2012R2Datacenter


    Create a VM from a custom managed image.
        az vm create -g MyResourceGroup -n MyVm --image MyImage


    Create a VM from a specialized image version.
        az vm create -g MyResourceGroup -n MyVm --image $id --specialized


    Create a VM by attaching to a managed operating system disk.
        az vm create -g MyResourceGroup -n MyVm --attach-os-disk MyOsDisk --os-type linux


    Create an Ubuntu Linux VM using a cloud-init script for configuration. See:
    https://docs.microsoft.com/azure/virtual-machines/linux/using-cloud-init.
        az vm create -g MyResourceGroup -n MyVm --image debian --custom-data MyCloudInitScript.yml

chenlomis · 2020-12-16T04:38:40Z

Quick questions:
What's the diff for the following 2 commands aside from the fact that one has optional tag and the other doesn't?
Is there an easy way to dedupe them by any chance?

Thanks @chenlomis for the comments. These are the recommended commands from Aladdin service, @christopher-o-toole may want to say somthing here?

For me, I think they are not duplicate commands for users. Because for every command, it always has a minimum set of arguments that could make it work. And all other argument combinations need to include these necessary arguments. We just do more advanced customizations with more arguments.

Take vm create as an example, we have many help examples shown to users, while they all have different scenarios.
    Create a default Ubuntu VM with automatic SSH authentication.
        az vm create -n MyVm -g MyResourceGroup --image UbuntuLTS


    Create a default RedHat VM with automatic SSH authentication using an image URN.
        az vm create -n MyVm -g MyResourceGroup --image RedHat:RHEL:7-RAW:7.4.2018010506


    Create a default Windows Server VM with a private IP address.
        az vm create -n MyVm -g MyResourceGroup --public-ip-address "" --image Win2012R2Datacenter


    Create a VM from a custom managed image.
        az vm create -g MyResourceGroup -n MyVm --image MyImage


    Create a VM from a specialized image version.
        az vm create -g MyResourceGroup -n MyVm --image $id --specialized


    Create a VM by attaching to a managed operating system disk.
        az vm create -g MyResourceGroup -n MyVm --attach-os-disk MyOsDisk --os-type linux


    Create an Ubuntu Linux VM using a cloud-init script for configuration. See:
    https://docs.microsoft.com/azure/virtual-machines/linux/using-cloud-init.
        az vm create -g MyResourceGroup -n MyVm --image debian --custom-data MyCloudInitScript.yml

Thank you for the elaboration! @houk-ms
I like your vm examples. It's interesting though because all these vm variations have different descriptions... which do help me as the end user to understand that each command fulfills a different need and that each additional parameter would alter what the original core vm command does

In the case of the screenshot though, despite the 2nd command having an additional --tags param, the descriptions are the same

So imagine someone coming in genuinely seeking for next steps, how would (s)he know which one to pick....(?) And if (s)he doesn't know and can't follow through, then doesn't having both of them there cause additional confusion?

Am actually open to keeping both as long as they mean and do different things, and that the differences are reflected in the description

maybe that's a Q for @christopher-o-toole ?

jiasli · 2020-12-16T05:53:25Z

src/azure-cli-core/azure/cli/core/azclierror.py

                    print(recommendation, file=sys.stderr)

+            if self.aladdin_recommendations:
+                print('\nTRY THIS:', file=sys.stderr)


We can use print_styled_text("\nTRY THIS:") or print_styled_text((Style.PRIMARY, "\nTRY THIS:")) once #16258 is merged.

jiasli · 2020-12-16T05:56:54Z

src/azure-cli-core/azure/cli/core/azclierror.py

+            if self.aladdin_recommendations:
+                print('\nTRY THIS:', file=sys.stderr)
+                for recommendation, description in self.aladdin_recommendations:
+                    print_styled_text(recommendation, file=sys.stderr)


sys.stderr is now the default value for file.

jiasli · 2020-12-16T05:58:37Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+            return [candidate['recommendation'] for candidate in candidates]

-            return candidates
+        def decorate_command(raw_command):


The functionality for colorization can be extracted and put in style.py as a common util.

houk-ms · 2020-12-17T03:46:31Z

@chenlomis If we simply replace the contextual values, there are cases that we will make users confused because the description are not changed accordingly.What do you think about this?

@christopher-o-toole and I tend to keep the original placeholders in the recommendations before we could find a proper method to solve it.

chenlomis · 2020-12-17T05:22:57Z

One more comment on why I insisted on "deduping the recommendations"

Below are some customer interview raw note, folks did have a reaction to seeing seemingly similar recommendations:

if the 2nd rec with --tags actually do offer something more unique, then we shall totally keep it
if it doesn't... it'd be absolutely amazing if there's a way to dedupe the top hand curated rec and Aladdin's rec

houk-ms · 2020-12-18T03:07:43Z

@houk-ms

Hrmm that's some interesting call out...
May I ask how the descriptions are generated?
Would it be possible to replace "West US region" in the description to "a region"?
I prefer keeping the command specific over keeping the description specific...Also think the more we can contextualize the recommendations the better... but up for debate

And if the above is not possible... is it going to show --location {samplelocations} where the description still shows "West US region"? because that doesn't seem too accurate either...

It's difficult to adjust the description, since it's more about semantic annlysis than a simple replacement. Let me give a more complex case

The original recommendations

When we have replacement

Anyway, i think there are even more complicated cases ...
Ideas on this? @chenlomis

houk-ms · 2020-12-18T03:10:57Z

One more comment on why I insisted on "deduping the recommendations"

For the commands deduping, i think @christopher-o-toole will keep improving this

chenlomis · 2020-12-18T20:46:13Z

@houk-ms
Hrmm that's some interesting call out...
May I ask how the descriptions are generated?
Would it be possible to replace "West US region" in the description to "a region"?
I prefer keeping the command specific over keeping the description specific...Also think the more we can contextualize the recommendations the better... but up for debate
And if the above is not possible... is it going to show --location {samplelocations} where the description still shows "West US region"? because that doesn't seem too accurate either...

It's difficult to adjust the description, since it's more about semantic annlysis than a simple replacement. Let me give a more complex case

The original recommendations

When we have replacement

Anyway, i think there are even more complicated cases ...
Ideas on this? @chenlomis

Oh I see how the the description is "locked" with the original recommendation now...
and the variables in the descriptions aren't contextual like the command's...

Hmm I don't know how easy it is to implement, and it's probably a conversation for future releases, but is it possible to run the description through some form of syntactic analyzer and identifies the corresponding matching words with the command's values and set those as variables before displaying it here? That way when the values of the commands are replaced/updated, the descriptions will also get updated

Otherwise I'd upvote for keeping the original description more general, so that it doesn't create confusion even if we replace/update the command values
Similar to the description "Read more about the command in reference docs" for the reference link

If that's not possible either.. (since it also requires updates to the description), then perhaps we should go with what you suggested earlier which is to keep the values non contextual for this release, until we figure out a workaround to update the description as well (maybe next release?). Because otherwise there'd be a mismatch between the the commands and the description

@houk-ms @christopher-o-toole thoughts?

houk-ms · 2020-12-21T02:55:38Z

Oh I see how the the description is "locked" with the original recommendation now...
and the variables in the descriptions aren't contextual like the command's...

Hmm I don't know how easy it is to implement, and it's probably a conversation for future releases, but is it possible to run the description through some form of syntactic analyzer and identifies the corresponding matching words with the command's values and set those as variables before displaying it here? That way when the values of the commands are replaced/updated, the descriptions will also get updated

Otherwise I'd upvote for keeping the original description more general, so that it doesn't create confusion even if we replace/update the command values
Similar to the description "Read more about the command in reference docs" for the reference link

If that's not possible either.. (since it also requires updates to the description), then perhaps we should go with what you suggested earlier which is to keep the values non contextual for this release, until we figure out a workaround to update the description as well (maybe next release?). Because otherwise there'd be a mismatch between the the commands and the description

@houk-ms @christopher-o-toole thoughts?

Synched with @christopher-o-toole , seems both of the methods related with description adjustments are not easy. Let's keep the original values in this release and leave more time to figure out the better way.

fengzhou-msft · 2020-12-22T05:38:45Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+            error_type = AladdinUserFaultType.StorageAccountNotFound
+        elif 'resource_group' in error_msg or 'resource group' in error_msg:
+            error_type = AladdinUserFaultType.ResourceGroupNotFound
+    elif 'pattern' in error_msg or 'is not a valid value' in error_msg or 'invalid' in error_msg:


Is it enough to just match pattern instead of somethign like invalid pattern

This are the rules provided by Aladdin Team for their internal error categories used to train model. I think it's ok as long as it makes sense to them.

jiasli · 2020-12-23T06:08:35Z

src/azure-cli-core/azure/cli/core/style.py

    return ''.join(formatted_parts)
+
+
+def get_styled_command(raw_command):


We can call this function highlight_command, just as pygments does in Knack:

https://github.com/microsoft/knack/blob/4cb4c2e0516f760b08defa8b9afacc004f7e4fac/knack/output.py#L44-L46

def format_json_color(obj): from pygments import highlight, lexers, formatters return highlight(format_json(obj), lexers.JsonLexer(), formatters.TerminalFormatter()) # pylint: disable=no-member

I agree with jiasli.

Moreover, as far as I know that the UX design from Lomis adds the <> to wrap the parameter values. Such as

az webapp log config --name <WebappName> --resource-group <ResourceGroup> --detailed-error-messages <True or False>

Because only the color of each part is returned here, and the style content of text is not processed, I think the function of the style content of text processing also needs another separate and general method. (That method might call this method to get the color and then modify the content style of text)

So the naming of the method should be more accurate to distinguish the two methods with different processing degrees.

Let's move this discussion to #16349 which is solely for the highlighting functionality.

jiasli · 2020-12-23T06:11:32Z

src/azure-cli-core/azure/cli/core/style.py

+    styled_command = []
+    argument_begins = False
+
+    for index, arg in enumerate(raw_command.split()):


While str.split works here, shlex.split is safer which doesn't splits spaces within quoted text, like "some quoted text".

azure-cli/src/azure-cli-testsdk/azure/cli/testsdk/base.py

Line 289 in 0c85233

self.exit_code = cli_ctx.invoke(shlex.split(command), out_file=stdout_buf) or 0

str.split works fine here, except for the cases where we have space in the command name. shlex.split will remove the quotation marks in the command thus bring more complexity when we join the command args.

Let's use str.split here @zhoxing-ms for awareness

jiasli · 2020-12-23T06:19:35Z

src/azure-cli-core/azure/cli/core/style.py

+        spaced_arg = ' {}'.format(arg) if index > 0 else arg
+        style = Style.PRIMARY
+
+        if arg.startswith('-') and '=' not in arg:


The logic of = handling is a little bit tricky. It works for az config set a=b, but it doesn't work for az config unset output. It is the nature of positional argument that we can NOT decide whether an arg is

part of a command, or

a positional argument

until semantic analysis is involved.

Let's keep it simple and treat it as part of the command by now.

agree. @zhoxing-ms for awareness.

jiasli · 2020-12-23T06:20:58Z

src/azure-cli-core/azure/cli/core/style.py

    return ''.join(formatted_parts)
+
+
+def get_styled_command(raw_command):


How about we add some function doc and give some examples about the behavior?

Sure, moved to #16349

zhoxing-ms · 2020-12-24T07:39:14Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+    if 'unrecognized' in error_msg:
+        error_type = AladdinUserFaultType.UnrecognizedArguments
+    elif 'expected one argument' in error_msg or 'expected at least one argument' in error_msg \
+            or 'value required' in error_msg:
+        error_type = AladdinUserFaultType.ExpectedArgument
+    elif 'misspelled' in error_msg:
+        error_type = AladdinUserFaultType.UnknownSubcommand
+    elif 'arguments are required' in error_msg or 'argument required' in error_msg:
+        error_type = AladdinUserFaultType.MissingRequiredParameters
+        if '_subcommand' in error_msg:
+            error_type = AladdinUserFaultType.MissingRequiredSubcommand
+        elif '_command_package' in error_msg:
+            error_type = AladdinUserFaultType.UnableToParseCommandInput
+    elif 'not found' in error_msg or 'could not be found' in error_msg \
+            or 'resource not found' in error_msg:
+        error_type = AladdinUserFaultType.AzureResourceNotFound
+        if 'storage_account' in error_msg or 'storage account' in error_msg:
+            error_type = AladdinUserFaultType.StorageAccountNotFound
+        elif 'resource_group' in error_msg or 'resource group' in error_msg:
+            error_type = AladdinUserFaultType.ResourceGroupNotFound


Could we make the mapping relationship between these error messages and error codes into a dict, and then match the error types directly through the dict?
So we don't need to write so many judgment statements, and the code may be more readable
Such as:

error_type_dict = { 'not found': AladdinUserFaultType.UnableToParseCommandInput, 'misspelled': AladdinUserFaultType.UnknownSubcommand, 'arguments are required': { '_subcommand': AladdinUserFaultType.MissingRequiredSubcommand, '_command_package': AladdinUserFaultType.UnableToParseCommandInput } }

It's a good idea!

But i am afraid it may bring extra complexity. For example, the statements here need 15 key-value pairs?

elif 'not found' in error_msg or 'could not be found' in error_msg \ or 'resource not found' in error_msg: error_type = AladdinUserFaultType.AzureResourceNotFound if 'storage_account' in error_msg or 'storage account' in error_msg: error_type = AladdinUserFaultType.StorageAccountNotFound elif 'resource_group' in error_msg or 'resource group' in error_msg: error_type = AladdinUserFaultType.ResourceGroupNotFound

Besides, can we deal with the default logic? E.g, the error_type = AladdinUserFaultType.AzureResourceNotFound above?

For example, the statements here need 15 key-value pairs?

Yes, this makes it easier to find and modify the error type corresponding to an error message

error_type_dict = { 'not found': { "default error": AladdinUserFaultType.UnableToParseCommandInput, // default error type "storage_account": AladdinUserFaultType.StorageAccountNotFound, "storage account": AladdinUserFaultType.StorageAccountNotFound, "resource_group": AladdinUserFaultType.ResourceGroupNotFound, "resource group": AladdinUserFaultType.ResourceGroupNotFound }, ... }

In fact, the essential difference between the two ways is that one is aggregated according to the error message and the other is aggregated according to the error type returned.
I personally think that we are more concerned about how to get the error type according to error_msg rather than what error_msg corresponds to an error type.

zhoxing-ms · 2020-12-24T08:04:01Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+            The sorting rules below are applied in order:
+                1. Commands starting with the user's input command name are ahead of those don't
+                2. Commands having more matched arguments are ahead of those having less
+                3. Commands having less arguments are ahead of those having more


Does Aladdin recommendation service have its own ranking rules?
I personally think that the content of relevance ranking might be better implemented by the recommendation server side.

Yeah, we discussed with Aladdin team about this. We agreed that before the service could return better ordered recommendtions, we would control the orders in client side.

What's more, the rules will also be applied to example recommendations.

zhoxing-ms · 2020-12-24T09:04:51Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+    param_mappings = {
+        '-h': '--help',
+        '-o': '--output',
+        '--only-show-errors': None,
+        '--help': None,
+        '--output': None,
+        '--query': None,
+        '--debug': None,
+        '--verbose': None
+    }


What kind of parameters need to be put here?
How about --yes, --no-wait?

It's a good catch, let me confirm. I think we need to add all the global parameters here.

yungezz · 2020-12-24T09:11:06Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+        if self.aladdin_recommendations:
+            telemetry.set_debug_info('AladdinRecommendCommand', ';'.join(raw_commands))
+        else:
+            telemetry.set_debug_info('ExampleRecommendCommand', ';'.join(raw_commands))


will this contain user input value?

No, these are the raw recommendations.

yungezz · 2020-12-24T09:12:26Z

src/azure-cli-core/azure/cli/core/command_recommender.py

+            3. In testing environments
+
+        :return: whether Aladdin service need to be disabled or not
+        :type: bool


in 2,3, we could still provide recommendation from help right? Seems need to separate it in command_recommender

They have already been separated, we could provide recommendations from help examples even though Aladdin service is disabled.

houk-ms added 3 commits December 8, 2020 17:17

initial version for new error output

2d016aa

Merge branch 'dev' into remove-error-type-prefix

6caf27d

apply style in recommendations

74e9297

houk-ms added do-not-merge Error Handling labels Dec 14, 2020

houk-ms requested review from Juliehzl, evelyn-ys, fengzhou-msft, haroldrandom, jiasli and jsntcy as code owners December 14, 2020 07:11

yungezz assigned fengzhou-msft Dec 15, 2020

sort the recommendations

890072f

houk-ms changed the title ~~[Core] {Draft} New error output~~ [Core] New error output Dec 15, 2020

support config to disable error recommendations

7f3aad1

houk-ms requested a review from yungezz December 15, 2020 09:13

jiasli reviewed Dec 16, 2020

View reviewed changes

command recommender refactoring

98595c9

code style refining

75ae600

houk-ms requested a review from jiasli December 21, 2020 09:27

fengzhou-msft reviewed Dec 22, 2020

View reviewed changes

houk-ms mentioned this pull request Dec 23, 2020

{Error Improvement} Remove error type in error message #15853

Closed

jiasli reviewed Dec 23, 2020

View reviewed changes

jiasli mentioned this pull request Dec 24, 2020

{Core} Add util function highlight_command #16349

Merged

3 tasks

zhoxing-ms reviewed Dec 24, 2020

View reviewed changes

yungezz approved these changes Dec 24, 2020

View reviewed changes

houk-ms added 3 commits January 20, 2021 16:50

merge dev branch

713f62d

code refining

3e58891

resolve merge conflicts

62e85fd

houk-ms removed the do-not-merge label Jan 20, 2021

houk-ms added 3 commits February 2, 2021 13:23

merge with dev

5e04dad

use core.error_recommendation=on/off as error recommmendation switch

0680f50

resolve conflicts

6db67e7

fengzhou-msft approved these changes Feb 2, 2021

View reviewed changes

houk-ms merged commit b32fffa into Azure:dev Feb 2, 2021

jiasli mentioned this pull request Mar 18, 2021

Fix incorrect error message for arguments with choices microsoft/knack#237

Closed

houk-ms mentioned this pull request May 11, 2021

Error Output Improvement: Requests to be confirmed #15693

Closed

		return ''.join(formatted_parts)


		def get_styled_command(raw_command):

[Core] New error output #16257

[Core] New error output #16257

Uh oh!

Conversation

houk-ms commented Dec 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houk-ms commented Dec 14, 2020

Uh oh!

yonzhan commented Dec 14, 2020

Uh oh!

houk-ms commented Dec 15, 2020

Uh oh!

houk-ms commented Dec 15, 2020

Uh oh!

houk-ms commented Dec 15, 2020

Uh oh!

houk-ms commented Dec 15, 2020

Uh oh!

azure-pipelines bot commented Dec 15, 2020

Uh oh!

chenlomis commented Dec 15, 2020

Uh oh!

houk-ms commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chenlomis commented Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jiasli Dec 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houk-ms commented Dec 17, 2020

Uh oh!

chenlomis commented Dec 17, 2020

Uh oh!

houk-ms commented Dec 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

houk-ms commented Dec 18, 2020

Uh oh!

chenlomis commented Dec 18, 2020

Uh oh!

houk-ms commented Dec 21, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houk-ms Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zhoxing-ms Dec 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

houk-ms commented Dec 14, 2020 •

edited

Loading

houk-ms commented Dec 16, 2020 •

edited

Loading

chenlomis commented Dec 16, 2020 •

edited

Loading

jiasli Dec 16, 2020 •

edited

Loading

houk-ms commented Dec 18, 2020 •

edited

Loading

houk-ms Dec 22, 2020 •

edited

Loading

zhoxing-ms Dec 24, 2020 •

edited

Loading

zhoxing-ms Dec 28, 2020 •

edited

Loading

zhoxing-ms Dec 24, 2020 •

edited

Loading