RSDK-12499: better error log when host is offline #505
Conversation
This reverts commit cec05c7.
|
Would we potentially miss any useful info with the new conditional? Also would there be any significant performance improvement if we use the |
For the check if host is offline, no useful info lost because the last message in the SDK/CLI will tell user that host is offline. For the "no active offers for """" I've only ever gotten it when the host was offline (which user will get error for later) but can add another check to confirm the host is actually offline and its not another issue. Also can update to use errors.Is! Think it will be much faster |
It may not be the best idea to create this kind of linkage unless it's absolutely necessary (counting on the user to behave a certain way).
There are some examples in Btw you may want to loop in others who are working more closely on the log improvements project to review these PRs instead of me. We've all seen these errors plenty of times, but they would probably have more thoughts on the best way/place to implement these changes. |
thats a fair point, I can just lower it to debug so it doesn't show up on the CLI + users don't get confused by it but its still there |
This reverts commit 054142e.
| } | ||
|
|
||
| var errOffline = status.Error(codes.Unavailable, "host appears to be offline; ensure machine is online and try again") | ||
| var ErrOffline = errors.New("host appears to be offline; ensure machine is online and try again") |
There was a problem hiding this comment.
Why remove the "unavailable" status?
There was a problem hiding this comment.
Ah I see the comment from your other PR.
There was a problem hiding this comment.
updated it to just check for ErrOffline by making ErrOffline a regular error instead of a status. I think making it status() was double wrapping it down the line because the original error was rpc error: code = Unavailable desc = requestID=99cc1420b3f4e658458dc61ec70cb033: rpc error: code = Unavailable desc = host appears to be offline; ensure machine is online and try again; context deadline exceeded; mDNS query failed to find a candidate
pasting it here so its easier for others to see^!
There was a problem hiding this comment.
what does the error become with this change? you can test this by pointing goutils on app to this change and running the cli/sdk against it. I ask because I would like to keep the Unavailable code if possible, so if we lose that we should think of ways to keep it in
There was a problem hiding this comment.
I'm reverting this change because the new error status has the error as "unknown" vs "unavailable". So something else is re-wrapping this error status but without the "unavailable" status wanted. I think you can get the error message from the status by converting it later on anyways
There was a problem hiding this comment.
if we're exporting this, we should name this var something something more specific ErrHostOffline or something like that. I think it would actually be fine to leave this as a status.Error, and I want to change to code to NotFound, since it matches better and is not duplicated elsewhere in this file https://grpc.io/docs/guides/status-codes/#the-full-list-of-status-codes. Then we can match on the status code in the rdk
|
For security reasons, this PR must be labeled with |
| } | ||
|
|
||
| var errOffline = status.Error(codes.Unavailable, "host appears to be offline; ensure machine is online and try again") | ||
| var ErrOffline = errors.New("host appears to be offline; ensure machine is online and try again") |
There was a problem hiding this comment.
if we're exporting this, we should name this var something something more specific ErrHostOffline or something like that. I think it would actually be fine to leave this as a status.Error, and I want to change to code to NotFound, since it matches better and is not duplicated elsewhere in this file https://grpc.io/docs/guides/status-codes/#the-full-list-of-status-codes. Then we can match on the status code in the rdk
| } | ||
|
|
||
| // ErrMDNSQueryFailed is returned when a mDNS query fails to find a candidate. | ||
| var ErrMDNSQuery = errors.New("mDNS query failed to find a candidate") |
There was a problem hiding this comment.
this is not a very descriptive name - can we name this ErrMDNSNoCandidatesFound or something in that vein?
5 participants
See viamrobotics/rdk#5471 for info