Handled unhandled exception to show bug report message

[hawflau]

Which issue(s) does this change fix?

N/A

Why is this change necessary?

To show a message that guides customers to create a bug report in case of unhandled exceptions

How does it address the issue?

• Handle the "unhandled exception" by printing the stack trace and the message with a github url
• Include handling of click.BadOptionUsage, click.BadArgumentUsage, click.BadParameter and click.UsageError into the handling of UserException

Example:

> samdev build
.
.
.
'BuildProperties': {'Minify': False, 'Target': 'es2020', 'Sourcemap': True, 'External': ['./my-module-a.js', './my-module-b.js'], 'EntryPoints': ['app.js']}} architecture: x86_64 functions: HelloWorldFunction

Traceback:
  File "/Volumes/workplace/gh/aws-sam-cli/.venv39/lib/python3.9/site-packages/click/core.py", line 1055, in main
.
.
.
  File "/Volumes/workplace/gh/aws-sam-cli/samcli/lib/build/workflow_config.py", line 70, in get_selector
    raise UnsupportedBuilderException("'{}' does not have a supported builder".format(specified_workflow))

While trying to execute "samdev build", we encountered an unexpected error.
To create a bug report, follow the Github issue template below:
https://github.com/aws/aws-sam-cli/issues/new?template=Bug_report.md&title=samdev%20build%20-%20UnsupportedBuilderException

What side effects does this change have?

The actual exitcode of the execution in case of UnhandledException is still 1. This is to not break existing behavior. We only report 255 to Telemetry.

Mandatory Checklist

PRs will only be reviewed after checklist is complete

• Add input/output type hints to new functions/methods
• Write design document if needed (Do I need to write a design document?)
• Write/update unit tests
• Write/update integration tests
• Write/update functional tests if needed
• make pr passes
• make update-reproducible-reqs if dependencies were changed
• Write documentation

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[torresxb1]

I feel like this could make it easier to have duplicate issues (multiple issues asking about the same problem). Could we first give them a link to search issues with the same title that this issue would be auto-populated?
For example, for the example in your PR description it would be:

https://github.com/aws/aws-sam-cli/issues?q=is%3Aissue+is%3Aopen+samdev+build+-+UnsupportedBuilderException

[hawflau]

Updated the messaging

[torresxb1]

why did we add more exceptions to catch?

[torresxb1]

I think I understand now, but why did we add only these? Should we just catch all click exceptions? Or at least a few others that are here: https://click.palletsprojects.com/en/8.1.x/api/#exceptions

[hawflau]

Good catch. I missed click.UsageError.

The idea of grouping these click exceptions together is that these exceptions should be fixable by users. Any user-fixable-exception that is thrown within the call stacks wrapped by @track_command should be caught by this line. I don't want to catch click.ClickException in general because that could be over-catching. I'd rather an exception to be an Unhandled Exception by default.

[torresxb1]

Gotcha. What about NoSuchOption?

[hawflau]

I don't see any reason we should use NoSuchOption in our own code. So don't think we should include it.

[torresxb1]

Maybe I'm misunderstanding but wouldn't NoSuchOption be raised by click when a user provides an option that doesn't exist? Or are you saying that wouldn't even reach this code anyway?

[hawflau]

It won't reach this code. We should have all @option declared before @track_command

[torresxb1]

Why did this change from ex.exit_code to 1?

[hawflau]

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.

[torresxb1]

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

[hawflau]

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.

[torresxb1]

Left a few questions and suggestions

[jfuss]

Looks good to me. Thanks for handling the feedback.

[sriram-mv]

Thanks! do we have an idea of what kind of use cases would cause Unhandled Exceptions today?

[hawflau]

@sriram-mv

do we have an idea of what kind of use cases would cause Unhandled Exceptions today?

There are two cases from my observation:

• Handled errors that are not raised as UserException or subclass of UserException
  • in somewhere we throw click exceptions (e.g. click.BadOptionUsage) but we did not handle it the same as UserException. This PR now includes certain click exceptions in the handling of UserException.
• Bugs that we did not catch