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

Don't hide handler exceptions behind debug #108

Draft
wants to merge 2 commits into
base: dev
Choose a base branch
from
Draft

Conversation

elupus
Copy link
Contributor

@elupus elupus commented Jul 29, 2024

Don't swallow exceptions during configure of clusters in debug logs.

Catching generic Exceptions will hide programming errors. We could hide specific exceptions if we know their cause, other exceptions should be fixed.

@puddly
Copy link
Contributor

puddly commented Jul 29, 2024

I would suggest DeliveryError and asyncio.TimeoutError be hidden. Otherwise, logs get very spammy.

@elupus
Copy link
Contributor Author

elupus commented Jul 30, 2024

DeliveyError is a real error. We failed at doing something. Same with timeout. Hiding these type of things is why zha is so flakey at times.

@puddly
Copy link
Contributor

puddly commented Jul 30, 2024

I don't think that results in anything actionable, though. What does a user do if they get a delivery error during startup? This is where async_initialize is called. All of this is device state polling and nothing breaks if the (likely identical) state of an entity isn't read at startup.

@puddly
Copy link
Contributor

puddly commented Jul 30, 2024

That being said, more logging during device joining (and increased retries) would be good. We actually swapped it from a warning to a debug statement a few months ago because of log spam.

@elupus
Copy link
Contributor Author

elupus commented Jul 30, 2024

This is also used during the configure step which can be triggered by user on a button. Manual configure should be verbose to indicate what worked and what failed.

It should be actionable to create a bug report on the issue.

I do agree that during normal startup: IF we have cached values, we could ignore some errors like timeouts. But we can only do it if we have cached values and user have not requested a reconfigure.

Copy link

codecov bot commented Jul 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.65%. Comparing base (3a1e0d6) to head (f9279cd).
Report is 6 commits behind head on dev.

Additional details and impacted files
@@            Coverage Diff             @@
##              dev     #108      +/-   ##
==========================================
+ Coverage   95.64%   95.65%   +0.01%     
==========================================
  Files          61       61              
  Lines        9232     9255      +23     
==========================================
+ Hits         8830     8853      +23     
  Misses        402      402              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@puddly
Copy link
Contributor

puddly commented Jul 30, 2024

It should be actionable to create a bug report on the issue.

Definitely not. Delivery errors are unavoidable. Some devices are aggressive with sleeping after joining ad just are hard to reach. Some devices are hard to reach because the network has poor connectivity and the delivery error is genuine.

I do agree that during normal startup: IF we have cached values, we could ignore some errors like timeouts.

It would be best to isolate this change then to just those scenarios. Otherwise, such verbose and user-visible logging creates a support burden.

@elupus elupus marked this pull request as draft August 1, 2024 11:37
@elupus
Copy link
Contributor Author

elupus commented Aug 1, 2024

Converted to draft until we have some consensus. Also trying out #120 which might be a better alternative (can't decide yet).

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.

2 participants