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

Add contextual logging where applicable #124

Merged
merged 2 commits into from
Nov 17, 2020
Merged

Add contextual logging where applicable #124

merged 2 commits into from
Nov 17, 2020

Conversation

postlund
Copy link
Collaborator

@postlund postlund commented Oct 28, 2020

I have tried adding device id to all log points now. Not sure if it's the best way, but it's a try. This PR is however very important as many issues are hard to debug without it.

Fixes #95

@postlund postlund added the enhancement New feature or request label Oct 28, 2020
@postlund postlund requested a review from rospogrigio October 28, 2020 10:01
@rospogrigio
Copy link
Owner

OK please explain how it should work now...
One more thing: I think it should lighten the logs if we compress the deviceId in the logs, such as:
[ab0c..dd2f] instead of [ab0cd4552ef3ff1dd2f] , what do you think? It would also help align device20 and device22 loggings...

@postlund
Copy link
Collaborator Author

The idea is to wrap logger objects with an adapter that adds the device id:

https://docs.python.org/3/howto/logging-cookbook.html#using-loggeradapters-to-impart-contextual-information

It's basically what I did in pytuya before, but I've added a convenience class (ContextualLogger) so we can write self.debug instead of self.log.debug and simplify initialization (code re-use).

Sure, we can compress the id assuming we can do it in such a way it we don't get conflicts. Or we can assume no conflicts and hope for the best...

@ultratoto14
Copy link
Collaborator

I like the idea of having device id in each log, i'm used to do a tail | grep to identify the log.

As i use many light bulbs, some of them are physically powered off. When log level is info or debug, they all raise exceptions that mess up the logs and make them complex to read.

@postlund
Copy link
Collaborator Author

Yeah, all logs about connection errors and such are not very convenient. We need to do something about them (in another PR). In #106 I'm experimenting with only making a connection whenever broadcast messages are received. This removes the need for a re-connect loop, which in turn generate lots of noise when devices are offline. I kinda feel like I want to make that behavior default and using a connect loop (default behavior today) an option. In theory we would only need the broadcast version, but if someone has devices on a different network (broadcast domain), that will not work. So we need to have both.

@rospogrigio
Copy link
Owner

rospogrigio commented Nov 2, 2020

@postlund , I just tried to give a spin to this. Config flow does not work, I believe discovery is broken and I am getting this logs:
Edit: sorry, my fault, my main HA instance was still running, even if I had stopped it.

Anyway, it's still not very clear to me what I should test, and also I still believe it would be convenient to compress the deviceIDs for readability,
Let me know...

@postlund
Copy link
Collaborator Author

postlund commented Nov 2, 2020

Yeah, address already in use should be a give-away 😉 Should perhaps add better error output when this happens though.

Basically just look a bit at the outputs and make sure they seem to have decent logging. Also scan the code a bit in case I missed adding contextual logging anywhere.

I can compress output like in your first example. Should work OK.

Copy link
Owner

@rospogrigio rospogrigio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, even if I still would prefer the device IDs to be compressed in some way, I find it useless to have such long strings.
And also there are conflicts to be fixed... the good ol' fan.py ;-)

@postlund
Copy link
Collaborator Author

Ah, lol, I actually did implement device id compression but never pushed it... Should be in place now together with conflict fix.

@postlund postlund merged commit 0531a28 into master Nov 17, 2020
@postlund postlund deleted the context_logging branch November 17, 2020 11:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add contextual logging to platforms
3 participants