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

Handled unhandled exception to show bug report message #4565

Merged
merged 22 commits into from
Jan 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .github/ISSUE_TEMPLATE/Bug_report.md
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,14 @@ If you do find an existing Issue, re-open or add a comment to that Issue instead

### Additional environment details (Ex: Windows, Mac, Amazon Linux etc)

<!-- Either provide the following info (for AWS SAM CLI v1.68.0 or before) or paste the output of `sam --info` (AWS SAM CLI v1.69.0 or after). -->

1. OS:
2. `sam --version`:
3. AWS region:

```
# Paste the output of `sam --info` here
```

`Add --debug flag to command you are running`
1 change: 1 addition & 0 deletions .github/workflows/build.yml
Original file line number Diff line number Diff line change
Expand Up @@ -25,5 +25,6 @@ jobs:
- uses: actions/setup-python@v4
with:
python-version: ${{ matrix.python }}
- run: test -f "./.github/ISSUE_TEMPLATE/Bug_report.md" # prevent Bug_report.md from being renamed or deleted
- run: make init
- run: make pr
42 changes: 42 additions & 0 deletions samcli/commands/exceptions.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
"""
Class containing error conditions that are exposed to the user.
"""
import traceback
from urllib.parse import quote
from typing import Optional, IO

import click

Expand All @@ -25,6 +28,45 @@ def __init__(self, message, wrapped_from=None):
click.ClickException.__init__(self, message)


class UnhandledException(click.ClickException):
"""
Exception class to re-wrap any exception that is not a UserException.
Typically this means there is a bug in SAM CLI.
"""

GH_ISSUE_SEARCH_URL = "https://github.com/aws/aws-sam-cli/issues?q=is%3Aissue+is%3Aopen+{title}"
GH_BUG_REPORT_URL = "https://github.com/aws/aws-sam-cli/issues/new?template=Bug_report.md&title={title}"
hawflau marked this conversation as resolved.
Show resolved Hide resolved
# NOTE (hawflau): actual exitcode is 1 to not break existing behavior. Only report 255 to telemetry
exit_code = 1

def __init__(self, command: str, exception: Exception) -> None:
self._command = command
self._exception = exception
self.__traceback__ = self._exception.__traceback__

click.ClickException.__init__(self, type(exception).__name__)

def show(self, file: Optional[IO] = None) -> None:
"""Overriding show to customize printing stack trace and message"""
if file is None:
file = click._compat.get_text_stderr() # pylint: disable=protected-access

tb = "".join(traceback.format_tb(self.__traceback__))
click.echo(f"\nTraceback:\n{tb}", file=file, err=True)

encoded_title = quote(f"Bug: {self._command} - {type(self._exception).__name__}")
lookup_url = self.GH_ISSUE_SEARCH_URL.format(title=encoded_title)
create_issue_url = self.GH_BUG_REPORT_URL.format(title=encoded_title)
msg = (
f'An unexpected error was encountered while executing "{self._command}".\n'
"Search for an existing issue:\n"
f"{lookup_url}\n"
"Or create a bug report:\n"
f"{create_issue_url}"
)
click.secho(msg, file=file, err=True, fg="yellow")


class CredentialsError(UserException):
"""
Exception class when credentials that have been passed are invalid.
Expand Down
22 changes: 15 additions & 7 deletions samcli/lib/telemetry/metric.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
from samcli.cli.context import Context
from samcli.cli.global_config import GlobalConfig
from samcli.commands._utils.experimental import get_all_experimental_statues
from samcli.commands.exceptions import UserException
from samcli.commands.exceptions import UserException, UnhandledException
from samcli.lib.hook.hook_config import HookPackageConfig
from samcli.lib.hook.hook_wrapper import INTERNAL_PACKAGES_ROOT
from samcli.lib.hook.exceptions import InvalidHookPackageConfigException
Expand Down Expand Up @@ -148,16 +148,24 @@ def wrapped(*args, **kwargs):
# Execute the function and capture return value. This is returned back by the wrapper
# First argument of all commands should be the Context
return_value = func(*args, **kwargs)
except UserException as ex:
except (
UserException,
click.BadOptionUsage,
click.BadArgumentUsage,
click.BadParameter,
click.UsageError,
) as ex:
# Capture exception information and re-raise it later so we can first send metrics.
exception = ex
exit_code = ex.exit_code
if ex.wrapped_from is None:
exit_reason = type(ex).__name__
else:
# TODO (hawflau): review exitcode meaning. We now understand exitcode 1 as errors fixable by users.
exit_code = 1
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did this change from ex.exit_code to 1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

click.BadOptionUsage and other click exception caught here come with exitcode=2 by default. In our Telemetry, they were, however, classified as 255 (as we didn't catch any them). Today, we only have 1 and 255 exitcodes in our Telemetry. We should revisit the exitcode meaning someday. But for now, re-classifying them as exitcode 1 seems more correct to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

But why not use whatever actual exit code is thrown, instead of only either 1 or 255?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now I don't want to introduce new exitcode (e.g. 2) into Telemetry, until we have clearer understandings about how we want to use the exitcode. Today, click.BadOptionUsage and click.UsageError (maybe a couple other user-fixable exceptions as well) go into Telemetry with exitcode 255, which is certainly incorrect. I just want to fix this first.

if hasattr(ex, "wrapped_from") and ex.wrapped_from:
exit_reason = ex.wrapped_from
else:
exit_reason = type(ex).__name__
except Exception as ex:
exception = ex
command = ctx.command_path if ctx else ""
torresxb1 marked this conversation as resolved.
Show resolved Hide resolved
exception = UnhandledException(command, ex)
# Standard Unix practice to return exit code 255 on fatal/unhandled exit.
exit_code = 255
exit_reason = type(ex).__name__
Expand Down
33 changes: 33 additions & 0 deletions tests/unit/commands/test_exceptions.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import io

from unittest import TestCase
from unittest.mock import patch, Mock


from samcli.commands.exceptions import UnhandledException


class TestUnhandledException(TestCase):
def test_show_must_print_traceback_and_message(self):
wrapped_exception = None
try:
raise Exception("my_exception")
except Exception as e:
wrapped_exception = e

unhandled_exception = UnhandledException("test_command", wrapped_exception)
f = io.StringIO()
unhandled_exception.show(f)

output = f.getvalue()

self.assertIn("Traceback:", output)
self.assertIn('raise Exception("my_exception")', output)
self.assertIn(
'An unexpected error was encountered while executing "test_command".\n'
"Search for an existing issue:\n"
"https://github.com/aws/aws-sam-cli/issues?q=is%3Aissue+is%3Aopen+Bug%3A%20test_command%20-%20Exception\n"
"Or create a bug report:\n"
"https://github.com/aws/aws-sam-cli/issues/new?template=Bug_report.md&title=Bug%3A%20test_command%20-%20Exception",
output,
)
31 changes: 21 additions & 10 deletions tests/unit/lib/telemetry/test_metric.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
import platform
import time

from parameterized import parameterized

import samcli

from unittest import TestCase
from unittest.mock import patch, Mock, ANY, call

import pytest
import click
from parameterized import parameterized

from samcli.lib.hook.exceptions import InvalidHookPackageConfigException, InvalidHookWrapperException
from samcli.lib.iac.plugins_interfaces import ProjectTypes
Expand All @@ -28,7 +26,7 @@
ProjectDetails,
_send_command_run_metrics,
)
from samcli.commands.exceptions import UserException
from samcli.commands.exceptions import UserException, UnhandledException


class TestSendInstalledMetric(TestCase):
Expand Down Expand Up @@ -184,6 +182,10 @@ def real_fn():
(KeyError("IO Error test"), "KeyError", 255),
(UserException("Something went wrong", wrapped_from="CustomException"), "CustomException", 1),
(UserException("Something went wrong"), "UserException", 1),
(click.BadOptionUsage("--my-opt", "Wrong option usage"), "BadOptionUsage", 1),
(click.BadArgumentUsage("Wrong argument usage"), "BadArgumentUsage", 1),
(click.BadParameter("Bad param"), "BadParameter", 1),
(click.UsageError("UsageError"), "UsageError", 1),
]
)
@patch("samcli.lib.telemetry.metric.Context")
Expand All @@ -194,12 +196,13 @@ def test_records_exceptions(self, exception, expected_exception, expected_code,
def real_fn():
raise exception

with self.assertRaises(exception.__class__) as context:
expected_exception_class = exception.__class__ if expected_code != 255 else UnhandledException
with self.assertRaises(expected_exception_class) as context:
track_command(real_fn)()
self.assertEqual(
context.exception,
exception,
"Must re-raise the original exception object " "without modification",
"Must re-raise the original exception object without modification",
)

run_metrics_mock.assert_called_with(self.context_mock, ANY, expected_exception, expected_code)
Expand Down Expand Up @@ -257,21 +260,29 @@ def func(**kwargs):
(KeyError, 255),
(UserException, 1),
(InvalidHookWrapperException, 1),
(click.BadOptionUsage, 1),
(click.BadArgumentUsage, 1),
(click.BadParameter, 1),
(click.UsageError, 1),
]
)
@patch("samcli.lib.telemetry.metric.Context")
@patch("samcli.lib.telemetry.metric._send_command_run_metrics")
def test_context_contains_exception(self, expected_exception, expected_code, run_metrics_mock, context_mock):
self.context_mock.exception = expected_exception("some exception")
def test_context_contains_exception(self, exception, exitcode, run_metrics_mock, context_mock):
if exception == click.BadOptionUsage:
self.context_mock.exception = exception("--opt", "some exception")
else:
self.context_mock.exception = exception("some exception")
context_mock.get_current_context.return_value = self.context_mock

mocked_func = Mock()

expected_exception = exception if exitcode == 1 else UnhandledException
with self.assertRaises(expected_exception):
track_command(mocked_func)()

mocked_func.assert_not_called()
run_metrics_mock.assert_called_with(self.context_mock, ANY, expected_exception.__name__, expected_code)
run_metrics_mock.assert_called_with(self.context_mock, ANY, exception.__name__, exitcode)


class TestSendCommandMetrics(TestCase):
Expand Down