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

feat: when handling actions, print uncaught exceptions to stderr #1087

Merged
merged 9 commits into from
Dec 15, 2023

Conversation

tonyandrewmeyer
Copy link
Contributor

When handling an action and an exception is not caught, output basic information about the problem to stderr, and let the user know that a full traceback (that the Charm author probably wants, rather than the user running the action) is in debug-log.

Fixes #1078

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Dec 8, 2023

@benhoyt three things here before this can be properly reviewed:

  • I imagine there's some bikeshedding required for the argument name to setup_root_logging that controls this behaviour.
  • We need to finalise on what the output would look like (Actions: errors are captured to debug-log and are not in stderr #1078 has examples from actual crashes when doing juju run). My preference is on the shorter version, primarily because I think the traceback is more for the Charm author than the action running user. We could switch to the full version, and we can also tweak wording and to some extent spacing.
  • What's your opinion on tests here? I don't think I can test the actual sys.excepthook in the main process. I could add a test like the main ones that have a Charm in a subprocess, get it to crash, and capture stderr. Do you think that's worthwhile? Or have a better idea about how to add an automated test for this?

@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Dec 8, 2023

The framework unit tests and real pebble tests fail because of the macaroonbakery issue. The data charm tests fail since 2.9 it seems, although it must be some change on the other side too or we would have caught it before release - looking into that.

ops/log.py Outdated Show resolved Hide resolved
Co-authored-by: Ben Hoyt <[email protected]>
@tonyandrewmeyer
Copy link
Contributor Author

tonyandrewmeyer commented Dec 11, 2023

Notes from talking in person:

  • I imagine there's some bikeshedding required for the argument name to setup_root_logging that controls this behaviour.

Existing name is fine, setup_root_logging probably ought to have been _setup_root_logging and this shouldn't impact anyone.

  • We need to finalise on what the output would look like (Actions: errors are captured to debug-log and are not in stderr #1078 has examples from actual crashes when doing juju run). My preference is on the shorter version, primarily because I think the traceback is more for the Charm author than the action running user. We could switch to the full version, and we can also tweak wording and to some extent spacing.

Keep as-is other than one small suggestion.

  • What's your opinion on tests here? I don't think I can test the actual sys.excepthook in the main process. I could add a test like the main ones that have a Charm in a subprocess, get it to crash, and capture stderr. Do you think that's worthwhile? Or have a better idea about how to add an automated test for this?

Add a test_main test that has a failing action, check for the core pieces rather than character-exact matching.

@tonyandrewmeyer tonyandrewmeyer marked this pull request as ready for review December 11, 2023 22:55
Copy link
Collaborator

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Looks great to me. One test change suggested.

test/test_main.py Outdated Show resolved Hide resolved
@benhoyt benhoyt merged commit d0f1d3c into canonical:main Dec 15, 2023
18 checks passed
@tonyandrewmeyer tonyandrewmeyer deleted the nicer-action-errors-1078 branch December 15, 2023 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Actions: errors are captured to debug-log and are not in stderr
2 participants