RSDK-12502: add "check network" when fail to dial app #5454
Conversation
viambot
added
the
safe to test
| ctxWithTimeoutCancel() | ||
| if err != nil { | ||
| logger.Debugw("error while dialing app. Could not establish global, unified connection", "error", err) | ||
| logger.Debugw("error while dialing app. Could not establish global, unified connection; Check network connection", "error", err) |
There was a problem hiding this comment.
Debugw
What do you think about the log level here? Should we do something besides Debug? What might be the consequences of bumping it higher than Debug?
There was a problem hiding this comment.
I was trying to recreate this in my CLI/SDK but unable to by turning internet off(can recreate in RDK logger). What situation would people encounter this log except viam developers looking through rdk logger? if its just viam developers, then I think its ok to keep debug/diagnostic.
There was a problem hiding this comment.
I think any Viam user of the RDK might encounter it. I believe we decided to make it debug level when it was first introduced (as opposed to originally being error-level and then lowered or something). @cheukt might remember more.
I will say that you probably should not be adding new, helpful wording to debug level RDK logs as part of this project. Debug-level logs are almost never seen by users, as they have to have debug: true in their JSON config (or another log setting), and debug logs are super noisy when on.
I'd say we should bump this to Error unless doing so is extremely noisy (which it totally could be!)
There was a problem hiding this comment.
I don't think we should bump this to error - this is not a server error and users aren't really expected to do anything (unless they believe the server should be online). We could maybe bump to info for now and group into diagnostic logs. though the another salient point is that there's no avenue for a user to see this error on app while this is pertinent, so they'd have to be tailing syslog to begin with. though this log is probably good to show if you are debugging network issues
ok with bumping to info, but then the wording of the log should be a bit more benign
There was a problem hiding this comment.
ok with bumping to info, but then the wording of the log should be a bit more benign
SGTM. Forgot that the Error level introduces UI elements and basically indicates "user needs to fix this." I'm also fine with not touching this log at all TBH. I think my broader point was that if we're changing log messages to be "helpful and visible to users," doing so on a debug-level log seems incorrect.
There was a problem hiding this comment.
yep I agree with the broader point
There was a problem hiding this comment.
Since it doesn't seem like this log will be that visible to users + not bumping to error I'll just not touch it!
allisonschiang
changed the title
4 participants
started up a viam-server and then turned off wifi to see new log:
2025-11-07T16:35:20.831Z DEBUG rdk.networking.app_connection grpc/app_conn.go:97 error while dialing app. Could not establish global, unified connection; Check network connection {"error":"not connected"}