Skip to content

{Error Improvement} Adjust error types #15865

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.

Sign up for GitHub

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

Merged
houk-ms merged 8 commits into from
Nov 12, 2020
Merged

{Error Improvement} Adjust error types #15865

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
6453969
adjust error types
houk-ms Nov 11, 2020
03bc1bb
Merge branch 'dev' into adjust-error-types
houk-ms Nov 11, 2020
b7398ed
adjust error handling guideline
houk-ms Nov 11, 2020
f2b8815
Merge branch 'dev' into adjust-error-types
houk-ms Nov 12, 2020
9258ae4
refine error handling guidelines
houk-ms Nov 12, 2020
500b150
Merge branch 'dev' into adjust-error-types
houk-ms Nov 12, 2020
682bea8
replace UnknownError
houk-ms Nov 12, 2020
309a512
Update .github/pull_request_template.md
houk-ms Nov 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion doc/error_handling_guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ The newly designed error types are provided in [azure/cli/core/azclierror.py](ht
| | | -- FileOperationError
| | | -- ManualInterrupt
| | | -- ... More ...
| | | -- UnknownError # fallback of UserFault related errors
| | | -- UnclassifiedUserFault # fallback of UserFault related errors
Comment thread
houk-ms marked this conversation as resolved.
| | -- ClientError
| | | -- CLIInternalError
| | -- ServiceError
Expand Down
10 changes: 9 additions & 1 deletion src/azure-cli-core/azure/cli/core/azclierror.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ def send_telemetry(self):
telemetry.set_failure(self.error_msg)
if self.exception_trace:
telemetry.set_exception(self.exception_trace, '')


class UnknownError(AzCLIError):
""" Unclear errors, could not know who should be responsible for the errors.
DO NOT raise this error class in your codes. """
def send_telemetry(self):
super().send_telemetry()
telemetry.set_failure(self.error_msg)
# endregion


Expand Down Expand Up @@ -199,7 +207,7 @@ class ValidationError(UserFault):
pass


class UnknownError(UserFault):
class UnclassifiedUserFault(UserFault):
""" Fallback of the UserFault related error types.
Avoid using this class unless the error can not be classified into
the UserFault related specific error types.
Expand Down
4 changes: 2 additions & 2 deletions src/azure-cli-core/azure/cli/core/util.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,8 +96,8 @@ def handle_exception(ex): # pylint: disable=too-many-locals, too-many-statement
az_error = azclierror.ValidationError(error_msg)

elif isinstance(ex, CLIError):
# 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)

@evelyn-ys evelyn-ys Nov 12, 2020

Copy link
Copy Markdown
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?

@houk-ms houk-ms Nov 12, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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.


elif isinstance(ex, AzureError):
if extract_common_error_message(ex):
Expand Down