Better unexpected exception msgs #2883

a-dubs · 2023-12-06T21:01:28Z

Why is this needed?

The unexpected error message does not reflect accurate log location when run by non root, and also the error message obfuscates the original error message so the end user has no context for what actually went wrong. This PR fixes that.

Changes messages.UnexpectedError from a NamedMessage to a FormattedNamedMessage so that it must be formatted with a string representing an exception/error message to give better context on what errors actually occurred.

Please Squash this PR with this commit message

cli: improved cli/log message for unexpected errors

Fixes: #2600

Test Steps

Checklist

I have updated or added any unit tests accordingly
I have updated or added any integration tests accordingly
Changes here need to be documented, and this was done in:

Does this PR require extra reviews?

Yes
No

github-actions · 2023-12-06T21:03:16Z

Jira: This PR is not related to a Jira item. (The PR title does not include a SC-#### reference)

GitHub Issues:

Fixes: Improve the default error message when uncaught exceptions happen #2600

Launchpad Bugs: No Launchpad bugs are fixed by this PR. (No commits have LP: #### references)

Documentation: The changes in this PR do not require documentation changes.

👍 this comment to confirm that this is correct.

github-actions · 2023-12-06T21:03:17Z

🌎 This PR changes translatable messages. 🌏

Please select which scenarios apply. For further explanation, please read our policy on message changes.

New messages are being added.
- We will ask translators to take a look and add translations if they have time, but it will not block this PR.
Existing messages are being changed.
- ⚠️ Please add a comment with justification of why messages are being altered.
- If the changes are trivial (e.g. a typo fix), then translations must be preserved.
- If the changes are substantial, then we will ask translators to take a look and update translations if they have time, but it will not block this PR.
Existing messages are being deleted.
- No special action needed.

lucasmoura · 2023-12-07T13:15:47Z

uaclient/util.py

+
+def get_user_or_root_log_path() -> str:
+    if we_are_currently_root():
+        return "/var/log/ubuntu-advantage.log"


We can extract that information directly through the UAConfig object. Even though users shouldn't change that option, it is still safer to consume that from an UAConfig objectt.

Thank you for this suggestion! I was wondering if there was somewhere I could consume this info from, so thank you for pointing me in the right direction

lucasmoura · 2023-12-07T13:19:19Z

uaclient/util.py

+    if we_are_currently_root():
+        return "/var/log/ubuntu-advantage.log"
+    else:
+        return "~/.cache/ubuntu-pro/ubuntu-pro.log"


I think we can at least define part of this path on the defaults module and reuse it here.
This would even be useful for other places where we specify the non-root log path, like the log module

I think the log module should have a function that returns the non-root log file location - we can probably use that here

uaclient/messages/__init__.py

renanrodrigo · 2023-12-07T21:22:25Z

uaclient/messages/__init__.py

-To file a bug run: ubuntu-bug ubuntu-advantage-tools"""
+Unexpected error(s) occurred: {error_msg}
+For more details, see the log: {log_path}
+First, try searching online for solutions to this error.


But if your point is that we could have more.. ahem, sensible wording, then yes we could

Brb gonna go run this through chatGPT to make it more friendly.

It could be as simple as adding an "if", e.g. "If you think this is a bug, please run: ubuntu-bug ubuntu-advantage-tools"

I was under the impression that the "First, try searching online for solutions to this error." was the issue not the "Otherwise, file a bug using the command: ubuntu-bug ubuntu-advantage-tools"?
(Although i do agree that even the latter could use less imperative wording.)

Putting this all together. How does the following sound?

Consider searching online for solutions to this error. Or, if you think this is a bug, please run: ubuntu-bug ubuntu-advantage-tools

Sorry I didn't fully explain my suggestion well.
I meant that I think we don't need to explicitly ask the user to search online at all (even though I'm the one who suggested this). Seeing it written out feels a little too much like we're pushing too much responsibility on the user.
I do still think that the wording "To file a bug run: ubuntu-bug ubuntu-advantage-tools" or similar is too imperative though. So my idea is to only add a conditional ("if") to that suggestion.
I think the "if" suggests enough that it isn't necessarily a bug and could be worth searching online for.
So I think in total it would be:

Unexpected error(s) occurred: {error_msg} For more details, see the log: {log_path} If you think this is a bug, please run: ubuntu-bug ubuntu-advantage-tools

Okay if you are taking that requirement away, then yes I agree the "if you think this is a bug" preamble helps a lot. The search online message did feel quite pushy so I'm good with this!

Thank you! And sorry for flip-flopping on that idea 🙃

No worries! Thank you for re-evaluating this change's requirements :)

orndorffgrant · 2023-12-11T18:35:28Z

uaclient/messages/__init__.py

-Unexpected error(s) occurred.
-For more details, see the log: /var/log/ubuntu-advantage.log
-To file a bug run: ubuntu-bug ubuntu-advantage-tools"""
+Unexpected error(s) occurred: {error_msg}


While we're editing this - do we need the "(s)" after "error" here @lucasmoura @renanrodrigo? (Is there any chance we'll display more than one error?)

The only place I saw that used this error message and has multiple errors it reports is in contract.py. But it seems like it generates a separate error for each unexpected exception, instead of combining all the messages into one singular error?

yeah we can use the singular I guess
If many errors occur, then one error occurred

a-dubs · 2024-01-03T18:07:43Z

The error message is being changed per the requirements of issue #2600. This means that translations will need to be updated for the new error message.

orndorffgrant

Looking good! Some small nits

orndorffgrant · 2024-01-11T16:23:07Z

uaclient/cli/tests/test_cli.py

@@ -508,6 +511,7 @@ def test_errors_handled_gracefully(
        m_log_exception,
        m_event_info,
        m_delete_cache_key,
+        # _m_get_user_or_root_log_path,


This looks left behind by mistake

Suggested change

# _m_get_user_or_root_log_path,

orndorffgrant · 2024-01-11T16:24:10Z

uaclient/cli/tests/test_cli_attach.py

@@ -534,6 +537,7 @@ def test_attach_when_one_service_fails_to_enable(
        m_process_entitlement_delta,
        m_enable_order,
        _m_attachment_data_file_write,
+        # _m_get_user_or_root_log_path,


same here

Suggested change

# _m_get_user_or_root_log_path,

orndorffgrant · 2024-01-11T19:39:01Z

uaclient/log.py

+    if util.we_are_currently_root():
+        return UAConfig().log_file
+    else:
+        return "~/.cache/" + defaults.USER_CACHE_SUBDIR + "/ubuntu-pro.log"


Since we only call this when non-root, let's re-use the function below

Suggested change

return "~/.cache/" + defaults.USER_CACHE_SUBDIR + "/ubuntu-pro.log"

return get_user_log_file()

orndorffgrant · 2024-01-11T19:48:39Z

uaclient/tests/test_log.py

+
+
+class TestLogHelpers:
+    def test_get_user_or_root_log_file_path(self):


This looks like a good set of tests, but let's use parametrization and mock decorators to follow the patterns in the rest of the codebase. Something like this:

Suggested change

def test_get_user_or_root_log_file_path(self):

@pytest.mark.parametrize(

[

"we_are_currently_root",

"expected",

],

[

(True, "cfg_log_file"),

(False, "user_log_file"),

]

)

@mock.patch("uaclient.log.get_user_log_file", return_value="user_log_file")

@mock.patch("uaclient.config.UAConfig.log_file", new_callable=mock.PropertyMock, return_value="cfg_log_file")

@mock.patch("uaclient.util.we_are_currently_root")

def test_get_user_or_root_log_file_path(

self,

m_we_are_currently_root,

m_cfg_log_file,

m_get_user_log_file,

we_are_currently_root,

expected

):

@a-dubs sorry, I just noticed a confusing error in my suggestion above - the mock decorators were in the wrong order. It should be fixed now

thank you! will implement these changes now. Will break the last portion of the test out into a different function because it does not make sense with the parameterization.

nevermind I'm just going to get rid of that test case because it is redundant.

Message for Unexpected errors is now a formatted string that takes in an error message so that the original error message is no longer hidden from the end user. Fixes: #2600

added util function to get proper log path based on if currently root or not

orndorffgrant

This looks good - thank you!

a-dubs force-pushed the better-unexpected-exception-msgs branch 4 times, most recently from 0b86695 to e2415ea Compare December 6, 2023 22:32

lucasmoura reviewed Dec 7, 2023

View reviewed changes

renanrodrigo reviewed Dec 7, 2023

View reviewed changes

orndorffgrant reviewed Dec 11, 2023

View reviewed changes

a-dubs force-pushed the better-unexpected-exception-msgs branch 3 times, most recently from 808ffa9 to 6f35816 Compare January 3, 2024 01:05

a-dubs force-pushed the better-unexpected-exception-msgs branch 5 times, most recently from abf9c74 to f66c48a Compare January 9, 2024 14:23

orndorffgrant changed the base branch from main to next-v32 January 11, 2024 18:54

orndorffgrant reviewed Jan 11, 2024

View reviewed changes

a-dubs force-pushed the better-unexpected-exception-msgs branch 2 times, most recently from 31df360 to e1292ae Compare January 16, 2024 13:53

a-dubs added 4 commits January 16, 2024 08:56

cli: improved cli/log message for unexpected errors

16263d0

Message for Unexpected errors is now a formatted string that takes in an error message so that the original error message is no longer hidden from the end user. Fixes: #2600

feat: unxpected error messages point to correct log path if not root

d346ff2

added util function to get proper log path based on if currently root or not

added orndorffgrant's suggestions from PR

1a5230a

po: ran update-pos script yet again

bb3535f

a-dubs force-pushed the better-unexpected-exception-msgs branch from e1292ae to bb3535f Compare January 16, 2024 13:57

renanrodrigo approved these changes Jan 16, 2024

View reviewed changes

orndorffgrant approved these changes Jan 16, 2024

View reviewed changes

orndorffgrant merged commit 07ad714 into next-v32 Jan 16, 2024
23 checks passed

orndorffgrant deleted the better-unexpected-exception-msgs branch January 16, 2024 19:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better unexpected exception msgs #2883

Better unexpected exception msgs #2883

a-dubs commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited by a-dubs

Loading

lucasmoura Dec 7, 2023

a-dubs Dec 8, 2023

lucasmoura Dec 7, 2023

orndorffgrant Dec 11, 2023

renanrodrigo Dec 7, 2023

a-dubs Dec 8, 2023

orndorffgrant Dec 11, 2023

a-dubs Dec 11, 2023

a-dubs Dec 11, 2023

orndorffgrant Dec 11, 2023 •

edited

Loading

a-dubs Dec 11, 2023

orndorffgrant Dec 11, 2023

a-dubs Dec 11, 2023

orndorffgrant Dec 11, 2023 •

edited

Loading

a-dubs Dec 11, 2023

renanrodrigo Dec 11, 2023

a-dubs commented Jan 3, 2024

orndorffgrant left a comment

orndorffgrant Jan 11, 2024

orndorffgrant Jan 11, 2024

orndorffgrant Jan 11, 2024

orndorffgrant Jan 11, 2024 •

edited

Loading

orndorffgrant Jan 12, 2024

a-dubs Jan 12, 2024

a-dubs Jan 12, 2024

orndorffgrant left a comment

	return "~/.cache/" + defaults.USER_CACHE_SUBDIR + "/ubuntu-pro.log"
	return get_user_log_file()



		class TestLogHelpers:
		def test_get_user_or_root_log_file_path(self):

-    def test_get_user_or_root_log_file_path(self):
+    @pytest.mark.parametrize(
+        [
+            "we_are_currently_root",
+            "expected",
+        ],
+        [
+            (True, "cfg_log_file"),
+            (False, "user_log_file"),
+        ]
+    )
+    @mock.patch("uaclient.log.get_user_log_file", return_value="user_log_file")
+    @mock.patch("uaclient.config.UAConfig.log_file", new_callable=mock.PropertyMock, return_value="cfg_log_file")
+    @mock.patch("uaclient.util.we_are_currently_root")
+    def test_get_user_or_root_log_file_path(
+        self,
+        m_we_are_currently_root,
+        m_cfg_log_file,
+        m_get_user_log_file,
+        we_are_currently_root,
+        expected
+    ):

Better unexpected exception msgs #2883

Better unexpected exception msgs #2883

Conversation

a-dubs commented Dec 6, 2023 • edited Loading

Why is this needed?

Please Squash this PR with this commit message

Test Steps

Checklist

Does this PR require extra reviews?

github-actions bot commented Dec 6, 2023 • edited Loading

github-actions bot commented Dec 6, 2023 • edited by a-dubs Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orndorffgrant Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orndorffgrant Dec 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

a-dubs commented Jan 3, 2024

orndorffgrant left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orndorffgrant Jan 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orndorffgrant left a comment

Choose a reason for hiding this comment

a-dubs commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited

Loading

github-actions bot commented Dec 6, 2023 •

edited by a-dubs

Loading

orndorffgrant Dec 11, 2023 •

edited

Loading

orndorffgrant Dec 11, 2023 •

edited

Loading

orndorffgrant Jan 11, 2024 •

edited

Loading