Add logs to Teams connection flow #2914
Conversation
|
Uffizzi Preview |
lib/livebook/teams/connection.ex
Outdated
| @@ -67,9 +71,11 @@ defmodule Livebook.Teams.Connection do | |||
| case WebSocket.send(data.http_conn, data.websocket, data.ref, :ping) do | |||
| {:ok, conn, websocket} -> | |||
| Process.send_after(self(), {:loop_ping, data.ref}, @loop_ping_delay) | |||
| Logger.warning("Teams WebSocket connection - ping with success") | |||
There was a problem hiding this comment.
I was in doubt about logging here, but though it was worth it. The reason is that if a specfic process up in this process supervision tree dies, the Livebook.HubsSupervisor, this is not being restarted and could not see anything in log.
But, if we log here, we'll know if that process died and it was not restarted, if there are no "PING" logs anymore.
Open to suggestions, though.
There was a problem hiding this comment.
Logging a warning every 5 seconds sounds like too much, this may confuse other users that run without problems. Perhaps we should make these info and then we can ask users to set log level to info if we want to debug more precisely?
There was a problem hiding this comment.
Yes, I think we can remove this one and use the absence of logging as an indication of success. We also don't want to fill the user disk with ping reports. :D
There was a problem hiding this comment.
I think we can remove this one and use the absence of logging as an indication of success.
I thought this way too.
One problem is that when Livebook.HubsSupervisor, no message is logged, the system gets not recovered to a working state, and the absence of logging continues... but the system is not working properly.
Any comments/suggestions?
There was a problem hiding this comment.
@hugobarauna I understand but we have no indication right now that pinging is the part that is absent, and we risk making this too verbose.
Co-authored-by: José Valim <[email protected]>
Co-authored-by: José Valim <[email protected]>
Co-authored-by: José Valim <[email protected]>
Co-authored-by: José Valim <[email protected]>
|
@josevalim @jonatanklosko ready to merge? |
Co-authored-by: José Valim <[email protected]>
Co-authored-by: José Valim <[email protected]>
Closes #2910