Skip to content
This repository has been archived by the owner on Aug 15, 2024. It is now read-only.

avoid exceptions raising up to HA core #123

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

krahabb
Copy link

@krahabb krahabb commented Feb 7, 2023

Hello,
This is the second part of my proposal to reduce HA log cluttering.

This PR is rather 'intensive' on the code and I'll try to explain the rationale and the coding choices:

  • When an exception is not handled in the integration code and 'pops' up to HA core, the logging is very verbose comprising the full call stack. Of course, an unhandled exception is likely telling something important and we can't simply silence it but still, the verbosity might scare the 'production user' and might be totally useless. The idea for this PR is to simply catch everything and just log it preventing the stack dump to pollute the HA log for the standard production scenario. Instead, when DEBUG logging is enabled, the behavior is as before: let the exception float up to HA core so to dump the full stack log and help debugging so far.
  • Implementation. I've defined a simple and general context manager readily available wherever we want to trap and 'smartly' log code exceptions. I've already applied this 'exception trapper' wherever the calling code is the HA core so to filter out exceptions.
  • Preservation of code logic. Whenever applying the context I've tried to respect the existent code logic just wrapping the code in the context manager so to expect the less 'disruption' to existing logic. Having not partecipated in design and development of this code I'm aware I'm really ignorant about all the underlying choices and I don't want to disrupt any apparent or non-apparent logic behind it.
  • The integration setup call (async_setup_entry) has received a different approach (the logic was a bit remanaged) so to return more specific exceptions to HA core in order to let it manage the problem (by either signaling an auth error or in general telling the core to try reload the configuration entry at a later stage)
  • Testing. Beside running the pytests, I've succesfully tested the integration behavior with debug logging enabled and disabled and it looks like correctly following the design
  • Rafactoring. An initial approach was based off generating a superclass for the ViCare entities ('ViCareEntity') in order to use a single point of entry for the entity->update() call and put the exception logic there. It was straightful enough since the exceptions blocks in all the implementations were the same. Then, the need to trap exceptions in other parts of the code suggested me a different approach based off context managers and that became the solution but the common super class was a nice idea (in my opinion - very personal declination). Now it still implements the single point of entry for the update() call in every entity even if rather simple but can provide other common behaviors (just the entity->device_info() right now)

@oischinger
Copy link
Owner

Thanks for the contribution.
Generally it makes sense to handle the exceptions. I'm also fine if we do that as a 2st step.

In the long run I was however thinking of not logging some exceptions and instead creating error-type sensors e.g.

  • binary_sensor.vicare_server_rate_limit_exceeded (triggered by PyViCareRateLimitError)
  • binary_sensor.vicare_server_connection_error (triggered by ConnectionError)
  • sensor.vicare_server_invalid_data (triggered by PyViCareInvalidDataError)
  • sensor.vicare_server_value_error (triggered by ValueError)
  • sensor.vicare_server_vicare_error (triggered by ViCareError)

this way we could also trigger the exceptions only on the first occurence. and improve the logging as you mentioned in your other PR

@krahabb
Copy link
Author

krahabb commented Mar 19, 2023

Hello,
Sorry to be that late but I didn't receive any notification since some time ago I've completely disabled them in github (they were too much!). I'll try to follow the development even if, at the moment, I'm pretty busy on my own integration.
Thank you for following up!
Davide

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants