Skip to content

RSDK-12499: better error log when host is offline #505

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.

Sign up for GitHub

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

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
+10 −6
Open

RSDK-12499: better error log when host is offline #505

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
cec05c7
change logger
allisonschiang Oct 13, 2025
13c87f2
Delete panic_test.go
allisonschiang Oct 16, 2025
a189af2
warning remove
allisonschiang Nov 11, 2025
4cd064c
Revert "change logger"
allisonschiang Nov 11, 2025
e1acc46
j
allisonschiang Nov 12, 2025
6410152
Merge branch 'viamrobotics:main' into RSDK-12499
allisonschiang Nov 12, 2025
a91b8a2
Revert "j"
allisonschiang Nov 12, 2025
ff27d9d
Revert "warning remove"
allisonschiang Nov 12, 2025
3e9cabe
lower to debug
allisonschiang Nov 12, 2025
054142e
Update wrtc_signaling_answerer.go
allisonschiang Nov 12, 2025
def83ea
Revert "Update wrtc_signaling_answerer.go"
allisonschiang Nov 12, 2025
b916a0c
Update wrtc_call_queue_mongodb.go
allisonschiang Nov 14, 2025
8f57c4a
Update wrtc_call_queue_mongodb.go
allisonschiang Nov 14, 2025
359b70b
Update wrtc_call_queue_mongodb.go
allisonschiang Nov 14, 2025
ec32962
Update wrtc_call_queue_mongodb.go
allisonschiang Nov 14, 2025
8ed9569
changes
allisonschiang Nov 18, 2025
6db000f
ipdate
allisonschiang Nov 18, 2025
c3f42ec
Merge branch 'viamrobotics:main' into RSDK-12499
allisonschiang Nov 18, 2025
07734dd
Merge branch 'RSDK-12499' of github.com:allisonschiang/goutils into R…
allisonschiang Nov 18, 2025
53de733
oop
allisonschiang Nov 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion rpc/dial.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,9 @@ func listMulticastInterfaces() []net.Interface {
return interfaces
}

// ErrMDNSQueryFailed is returned when a mDNS query fails to find a candidate.
var ErrMDNSQuery = errors.New("mDNS query failed to find a candidate")
Copy link
Member

@cheukt cheukt Nov 18, 2025

Choose a reason for hiding this comment

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

this is not a very descriptive name - can we name this ErrMDNSNoCandidatesFound or something in that vein?


func lookupMDNSCandidate(ctx context.Context, address string, logger utils.ZapCompatibleLogger) (*zeroconf.ServiceEntry, error) {
candidates := []string{address, strings.ReplaceAll(address, ".", "-")}
// RSDK-8205: logger.Desugar().Sugar() is necessary to massage a ZapCompatibleLogger into a
Expand Down Expand Up @@ -356,7 +359,7 @@ func lookupMDNSCandidate(ctx context.Context, address string, logger utils.ZapCo
if ctx.Err() != nil {
return nil, ctx.Err()
}
return nil, errors.New("mDNS query failed to find a candidate")
return nil, ErrMDNSQuery
}

func dialMulticastDNS(
Expand Down
7 changes: 4 additions & 3 deletions rpc/wrtc_call_queue_mongodb.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1132,7 +1132,8 @@ func (queue *mongoDBWebRTCCallQueue) checkHostQueueSize(ctx context.Context, for
return errTooManyConns
}

var errOffline = status.Error(codes.Unavailable, "host appears to be offline; ensure machine is online and try again")
// ErrOffline is returned when a host appears to be offline.
var ErrOffline = errors.New("host appears to be offline; ensure machine is online and try again")
Copy link
Member

@benjirewis benjirewis Nov 14, 2025

Choose a reason for hiding this comment

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

Why remove the "unavailable" status?

Copy link
Member

@benjirewis benjirewis Nov 14, 2025

Choose a reason for hiding this comment

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

Ah I see the comment from your other PR.

Copy link
Member Author

@allisonschiang allisonschiang Nov 14, 2025

Choose a reason for hiding this comment

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

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^!

Copy link
Member

@cheukt cheukt Nov 14, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@allisonschiang allisonschiang Nov 17, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@cheukt cheukt Nov 18, 2025

Choose a reason for hiding this comment

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

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


// checkHostOnline will check if there is some operator for all the managed hosts that
// claims to have an answerer online for that host. It does this by running an aggregation
Expand Down Expand Up @@ -1167,7 +1168,7 @@ func (queue *mongoDBWebRTCCallQueue) checkHostOnline(ctx context.Context, hosts
return err
}
if len(ret) == 0 {
return errOffline
return status.Error(codes.Unavailable, ErrOffline.Error())
}
return nil
}
Expand All @@ -1188,7 +1189,7 @@ func (queue *mongoDBWebRTCCallQueue) incrementConnectionEstablishmentExpectedFai
// error (internal error from MDB query, e.g.). We can tell from the passed in error.

reason := "other"
if errors.Is(err, errOffline) {
if errors.Is(err, ErrOffline) {
reason = "answerers_offline"
} else if errors.Is(err, errTooManyConns) {
reason = "too_many_callers"
Expand Down
4 changes: 2 additions & 2 deletions rpc/wrtc_client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -389,7 +389,7 @@ func dialWebRTC(

utils.PanicCapturingGo(func() {
if err := exchangeCandidates(); err != nil {
logger.Warnw("Failed to exchange candidates", "err", err)
logger.Debugw("Failed to exchange candidates", "err", err)
exchangeCancel(err)
}
})
Expand All @@ -412,7 +412,7 @@ func dialWebRTC(
Error: ErrorToStatus(exchangeErr).Proto(),
},
}); err != nil {
logger.Warnw("Problem sending error to signaling server", "err", err)
logger.Debugw("Problem sending error to signaling server", "err", err)
}
})
return nil, exchangeErr
Expand Down
Loading