Add exception translations to ring integration

[sdb9696]

Proposed change

SSIA

Type of change

• Dependency upgrade
• Bugfix (non-breaking change which fixes an issue)
• New integration (thank you!)
• New feature (which adds functionality to an existing integration)
• Deprecation (breaking change to happen in the future)
• Breaking change (fix/feature causing existing functionality to break)
• Code quality improvements to existing code or addition of tests

Additional information

• This PR fixes or closes issue: fixes #
• This PR is related to issue:
• Link to documentation pull request:

Checklist

• The code change is tested and works locally.
• Local tests pass. Your PR cannot be merged unless tests pass
• There is no commented out code in this PR.
• I have followed the development checklist
• I have followed the perfect PR recommendations
• The code has been formatted using Ruff (ruff format homeassistant tests)
• Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

• Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

• The manifest file has all fields filled out correctly.
  Updated and included derived files by running: python3 -m script.hassfest.
• New or updated dependencies have been added to requirements_all.txt.
  Updated by running python3 -m script.gen_requirements_all.
• For the updated dependencies - a link to the changelog, or at minimum a diff between library versions is added to the PR description.

To help with the load of incoming pull requests:

• I have reviewed two other open pull requests in this repository.

[andrewsayre]

Exception translations are meant to provide a user-friendly message back to the user on why something went wrong. I think you're mixing up the purpose of that with the desire to capture the technical reason that something failed. Log that instead.

You can also raise the HomeAssistantError (and derivatives) from an existing exception, which will capture the stack traceback and message in the logs.

[home-assistant]

Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍

Learn more about our pull request process.

[sdb9696]

Exception translations are meant to provide a user-friendly message back to the user on why something went wrong. I think you're mixing up the purpose of that with the desire to capture the technical reason that something failed. Log that instead.

You can also raise the HomeAssistantError (and derivatives) from an existing exception, which will capture the stack traceback and message in the logs.

Hey @andrewsayre, have responded on this, let me know your thoughts. I have assumed the point you're making applies to all the exceptions, and not just the auth one in coordinator, but let me know if I've misunderstood that.

[joostlek]

And what does that mean?

[sdb9696]

This is re-worded to "Error negotiating stream for {device}"

[joostlek]

But like, reading this sounds like something was required, but what does that mean and how does someone get this? Like, I am no expert on camera streams, but if the technical reason is that something is required, that sounds like something that can be provided and then work?

[edenhaus]

To give Joost some context, the sdp_m_line_index is the index of the sdp m config in the WebRTC offer.
The frontend will include it, when sending the candidates to HA. The spec defines that the default value is None as so it's possible that None is received

[andrewsayre]

Hey @andrewsayre, have responded on this, let me know your thoughts. I have assumed the point you're making applies to all the exceptions, and not just the auth one in coordinator, but let me know if I've misunderstood that.

Thanks for your responses, @sdb9696! Yes, that applies to anything that would be user-facing in the UI. 😄 Here's some thoughts on what I'd like to see changed to move this PR forward:

• You don't have to correct other instances where developer/implementation details are included in user-facing messages (such as in entity.py), but let's not continue that practice here.
• I bet you can anticipate most of the common failure scenarios and provide a specific translation, which would negate the need to include or log further information. Everything else would be truly exceptional and the translation can be generic, like, "Unexpected error authenticating". To capture the details, log at debug so it will only show up when integration debugging is enabled, which I think is fine to bypass log suppression in the debugging context.
• If the exceptions raised in the coordinator aren't shown to the user, then it doesn't really matter what it includes, and [probably] a translation doesn't add value. However, one consideration is that those exceptions might be shown in the future, so would be more future-proof to make them user-friendly now.

@joostlek any thing you want to change or add to what I said above? :)

[joostlek]

To capture the details, log at debug so it will only show up when integration debugging is enabled

Well if something is truly exceptional, I don't expect it to be easy to reproduce, so I would opt to log with LOGGER.exception, so it will just put out the full exception in the logs

If the exceptions raised in the coordinator aren't shown to the user

I believe they are but I don't have any example at hand. But if they didn't they should

[sdb9696]

I did some testing and the coordinator error does appear in the UI during setup.

I have updated the PR to exclude the technical details from the exception translations and instead use LOGGER.exception. To prevent the coordinator spamming the logs I moved _call_api to be an instance method of the coordinator so it can check last_update_success.

[edenhaus]

I have updated the PR to exclude the technical details from the exception translations and instead use LOGGER.exception.

This will log the error twice, which should be avoided. I saw that most exceptions will be created with from err, so the original error is included in the stacktrace

[sdb9696]

There is no stack trace in the log for the base coordinator error. This is the log output from one of the test cases:

2025-01-28 13:36:56.879 ERROR    MainThread homeassistant.components.ring.coordinator:coordinator.py:105 Error calling async_update_devices: 
Traceback (most recent call last):
  File "/home/username/Code/sdb9696/core/homeassistant/components/ring/coordinator.py", line 80, in _call_api
    return await target(*args)
           ^^^^^^^^^^^^^^^^^^^
  File "/home/username/.local/share/uv/python/cpython-3.13.0-linux-x86_64-gnu/lib/python3.13/unittest/mock.py", line 2310, in _execute_mock_call
    raise effect
ring_doorbell.exceptions.RingError: Some internal error info
2025-01-28 13:36:56.879 ERROR    MainThread homeassistant.components.ring.coordinator:update_coordinator.py:405 Error fetching devices data: Error communicating with Ring API
2025-01-28 13:36:56.879 DEBUG    MainThread homeassistant.components.ring.coordinator:update_coordinator.py:453 Finished fetching devices data in 0.000 seconds (success: False)

The error from the base coordinator just contains the translation information.

[andrewsayre]

Great job, @sdb9696! Two smaller things to look at and this is good to go!

[andrewsayre]

Asserting the negative isn't great because the code could have not ran at all, or the translation strings could have been changed, and this would still pass. 😃

What about checking that the mocked API method was called and the coordinator's last_update_success==True or last_exception is None?

Change in the test below too.

[sdb9696]

Agree with checking the mocked api method was called. Not sure how to get hold of the coordinator in the test though, never done that before.

[andrewsayre]

You should be able to get to it from mock_config_entry.runtime_data.devices_coordinator. Type the entry to RingConfigEntry to get type hinting.

[sdb9696]

Ok cool will try that out tomorrow, thanks!

[sdb9696]

Asserting the negative isn't great because the code could have not ran at all, or the translation strings could have been changed, and this would still pass. 😃

What about checking that the mocked API method was called and the coordinator's last_update_success==True or last_exception is None?

Updated both tests to check that the method was called and check the coordinator.last_exception is correctly set. Have still kept the negative log check as checking last_update_success==True doesn't work, because the subsequent updates should not be succeeding for this test.

[edenhaus]

There is no stack trace in the log for the base coordinator error. This is the log output from one of the test cases:

2025-01-28 13:36:56.879 ERROR    MainThread homeassistant.components.ring.coordinator:coordinator.py:105 Error calling async_update_devices: 
Traceback (most recent call last):
  File "/home/username/Code/sdb9696/core/homeassistant/components/ring/coordinator.py", line 80, in _call_api
    return await target(*args)
           ^^^^^^^^^^^^^^^^^^^
  File "/home/username/.local/share/uv/python/cpython-3.13.0-linux-x86_64-gnu/lib/python3.13/unittest/mock.py", line 2310, in _execute_mock_call
    raise effect
ring_doorbell.exceptions.RingError: Some internal error info
2025-01-28 13:36:56.879 ERROR    MainThread homeassistant.components.ring.coordinator:update_coordinator.py:405 Error fetching devices data: Error communicating with Ring API
2025-01-28 13:36:56.879 DEBUG    MainThread homeassistant.components.ring.coordinator:update_coordinator.py:453 Finished fetching devices data in 0.000 seconds (success: False)

The error from the base coordinator just contains the translation information.

It's currently implemented that on a UpdateFailed Exception, we are only logging the error message. Please see the base implementation of the coordinator to see what is logged on which exception.

core/homeassistant/helpers/update_coordinator.py

Lines 401 to 406 in 600fe87

	except UpdateFailed as err:
	self.last_exception = err
	if self.last_update_success:
	if log_failures:
	self.logger.error("Error fetching %s data: %s", self.name, err)
	self.last_update_success = False

core/homeassistant/helpers/update_coordinator.py

Lines 441 to 444 in 600fe87

	except Exception as err:
	self.last_exception = err
	self.last_update_success = False
	self.logger.exception("Unexpected error fetching %s data", self.name)

Will put this PR on the agenda of the next core meeting as I don't like to log errors twice. Please keep the PR in draft until I come back with a decision

[sdb9696]

imo fwiw, I agree that it would be better to only log once and do logger.exception for the UpdateFailed exceptions. This satisfies the requirement highlighted in this PR not to include the exception in the user facing message, but also ensure the logs have useful information. This prevents the need to log twice, and also removes the need to duplicate the log spam suppression logic already present in the "only log once" logic in the update_coordinator.

[edenhaus]

We discussed this PR internally and concluded the following:

• the "golden" Rule is log or rethrow. Therefore, we will not accept that the exception is logged before raising. Debug logs are fine if they add additional value/context. Only logging the error on debug level is not required as this will implemented.
• Known/expected exceptions like AuthenticationError, Timeout should not log any stacktrace
• I will create a separate PR, where I will update the update coordinator to log stacktrace of UpdateFailed on a debug level. So you have the stacktrace available during debugging.
• Unknown errors should not be translated and not wrapped in any HA error. Instead they should bubble up, where they than will be logged with the stacktrace

I'm not familiar with the ring lib, so I'm unsure if RingError is a unexpected/unknown error or it that one will be also raised for expected ones.
Please adopt the PR

[sdb9696]

That's all done. RingError is raised from the library for a mix of known and some unknown errors.

[andrewsayre]

Thanks for making the changes! Just a few smaller items.

[andrewsayre]

Thank you, looks good! I like how much more straightforward this code became! Nice work!

Requested changes implemented; reviewed and approved by other member.

[sdb9696]

Thanks @andrewsayre !