Log bare exceptions in the config flow #135584
Conversation
home-assistant
bot
added
cla-signed
code-quality
integration: deluge
integration: dexcom
integration: matter
integration: qnap
integration: vilfo
small-pr
|
Hey there @disforw, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @gagebenne, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @home-assistant/matter, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @ManneW, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
|
Hey there @tkdrob, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
| except Exception as ex: | ||
| _LOGGER.exception("Unexpected error") | ||
| if type(ex).__name__ == "BadLoginError": | ||
| return "invalid_auth" | ||
| return "unknown" |
There was a problem hiding this comment.
I am genuinely wondering why we don't do except BadLoginError. I also could not find a reference in the library. @tkdrob Can you maybe give a hint on this?
There was a problem hiding this comment.
Seems maybe it was still supporting py2 at the time this was done.
Could be fixed now I guess.
| except Exception: | ||
| LOGGER.exception("Unexpected exception") | ||
| errors["base"] = "unknown" |
There was a problem hiding this comment.
Opened an issue for this one as it should work since we log the exception
joostlek
requested review from
gjohansson-ST,
frenck,
epenet,
bieniu,
starkillerOG,
emontnemery and
jbouwh
as code owners
|
I'm not entirely sure. Is there really a benefit of the logging instead? |
|
How do you mean This will at least leave more information behind then "🤷🏻 unknown error happened" and will in the end help people catch more exceptions and make them more specific |
Sorry, meant |
| except Exception: # noqa: BLE001 | ||
| _LOGGER.debug("Disconnect error", exc_info=True) | ||
| except Exception: | ||
| _LOGGER.exception("Disconnect error") |
There was a problem hiding this comment.
I think this will add some unnecessary noise to logs, as there's likely nothing we can generally do if the disconnect fails, and IIRC it is or at least was a common phenomenon with some boxes/firmwares/control flows. I'd be fine with logging the exceptions we know we can do nothing about and that are known to be harmless with .debug and others with .exception (I can't tell which of them are such offhand, and don't have time to figure out/work on this any time soon), but I'm not quite convinced this change would be an improvement as is.
There was a problem hiding this comment.
But I think if there are exceptions that we did not catch, which do have a reason, we should be clearer to the user. For example if we suddenly get a TimeoutError, either tell the user we cannot connect, or we timed out, both are better than just "Unexpected error".
These excepts are usually just here to make sure the user is guaranteed not to see a "500 - invalid flow specified" (which I think is also something we can fix in the frontend to be more graceful)
There was a problem hiding this comment.
I agree with Joost.
I also don't expect that the noise to the logs will increase that much as all exceptions are unexpected, and they should be rare.
If an exception is raised often you should catch this one explicit and handle it differently
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
| @@ -86,7 +89,8 @@ async def validate_input(self, user_input: dict[str, Any]) -> str | None: | |||
| await self.hass.async_add_executor_job(api.connect) | |||
| except (ConnectionRefusedError, TimeoutError, SSLError): | |||
| return "cannot_connect" | |||
| except Exception as ex: # noqa: BLE001 | |||
| except Exception as ex: | |||
| _LOGGER.exception("Unexpected error") | |||
There was a problem hiding this comment.
Move this log down to after we handle the BadLoginError
There was a problem hiding this comment.
I can't seem to find a BadLoginError in the code at all, so this is something to look into
|
@joost there is a merge conflict, can you take a look? |
# Conflicts: # homeassistant/components/tado/config_flow.py # homeassistant/components/webdav/config_flow.py
Proposed change
So I found out that BLE001 does not get flagged if the exception is logged using LOGGER.exception, which is a good thing I think. So let's get all the BLE001 noqas out of the codebase!
By logging the exception stacktrace while encountering an unknown exception, we use this to provide more info to the user than just
unknown error. If we want more specific errors, we should just catch more specificType of change
Additional information
Checklist
ruff format homeassistant tests)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest.requirements_all.txt.Updated by running
python3 -m script.gen_requirements_all.To help with the load of incoming pull requests: