Skip to content

{Error Improvement} Adjust error types#15865

Merged
houk-ms merged 8 commits intoAzure:devfrom
houk-ms:adjust-error-types
Nov 12, 2020
Merged

{Error Improvement} Adjust error types#15865
houk-ms merged 8 commits intoAzure:devfrom
houk-ms:adjust-error-types

Conversation

@houk-ms
Copy link
Contributor

@houk-ms houk-ms commented Nov 11, 2020

Description

This PR adjsut some error type names for error handling.

  • UnclassifiedUserFault refers to the fallback error type of UserFault. Developers could use this error type when they can not find other specific UserFault types for their error cases.
  • UnknonwError refers to an error that we could not know which category(UserFault, ServiceError, ClientError) it belongs to. This will be only used in core's exception handler for error categorizing.

Testing Guide

Developer related. No changes in user experiences.

@houk-ms
Copy link
Contributor Author

houk-ms commented Nov 11, 2020

Need adjustments for the changes in #15776

@qwordy
Copy link
Member

qwordy commented Nov 12, 2020

What's the difference between unclassified and unknown? The naming can be discussed.
E.g.

azure.core.exceptions.HttpResponseError: (InvalidParameter) The value of parameter type is invalid.

Will the error type be erased? I strongly suggest reserve the original error type. Otherwise, I don't know what's wrong given an unknown error.

@houk-ms
Copy link
Contributor Author

houk-ms commented Nov 12, 2020

What's the difference between unclassified and unknown? The naming can be discussed.
E.g.

azure.core.exceptions.HttpResponseError: (InvalidParameter) The value of parameter type is invalid.

Will the error type be erased? I strongly suggest reserve the original error type. Otherwise, I don't know what's wrong given an unknown error.

Please see the PR description for their difference.

Yes, for PM's request, we may need remove the error type info. But the changes won't be applied in this release, so that we can have more time for discussion

Co-authored-by: Yishi Wang <yishiwang@microsoft.com>
# TODO: Fine-grained analysis here for Unknown error
az_error = azclierror.UnknownError(error_msg)
# TODO: Fine-grained analysis here
az_error = azclierror.UnclassifiedUserFault(error_msg)
Copy link
Member

Choose a reason for hiding this comment

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

Why CLIError here is UserFault?

Copy link
Contributor Author

@houk-ms houk-ms Nov 12, 2020

Choose a reason for hiding this comment

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

Ideally, CLIError will not show up here any more if it is deprecated.

Temporarily count the CLIError here as UserFaults for better Telemetry analysis. Because we could know almost all of the CLIErrors are UserFaults though may not 100%.

This is much better than counting CLIErrors as UnknownError. If so, a lot of userfaults are mis-categorized.


class ArgumentParseError(UserFault):
""" Fallback of the argument parsing related errors.
class ArgumentUsageError(UserFault):
Copy link
Member

Choose a reason for hiding this comment

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

ArgumentParseError seems making more sense.

Copy link
Contributor Author

@houk-ms houk-ms Nov 12, 2020

Choose a reason for hiding this comment

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

Want use this type to cover the original UsageError, which usually are the across arguement errors. ArgumentParseError seems more like parsing issues by the name.

Copy link
Contributor

@Juliehzl Juliehzl left a comment

Choose a reason for hiding this comment

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

LGTM

@qwordy
Copy link
Member

qwordy commented Nov 12, 2020

What's the difference between unclassified and unknown? The naming can be discussed.
E.g.

azure.core.exceptions.HttpResponseError: (InvalidParameter) The value of parameter type is invalid.

Will the error type be erased? I strongly suggest reserve the original error type. Otherwise, I don't know what's wrong given an unknown error.

Please see the PR description for their difference.

Yes, for PM's request, we may need remove the error type info. But the changes won't be applied in this release, so that we can have more time for discussion

Error type is important. If it is removed, I can't help users resolve their issues. At least, we need to make sure error type and stack trace are available in --debug logs. @chenlomis

@houk-ms houk-ms merged commit 296a85c into Azure:dev Nov 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

Comments