RSDK-9327: Dynamically check safety_heartbeat_monitored RPC option to decide if Sessions should be used

[aldenh-viam]

The current client implementation checks a blacklist on each RPC call:

rdk/robot/client/client_session.go

Lines 24 to 35 in 3de60f9

	var exemptFromSession = map[string]bool{
	"/grpc.reflection.v1alpha.ServerReflection/ServerReflectionInfo": true,
	"/proto.rpc.webrtc.v1.SignalingService/Call": true,
	"/proto.rpc.webrtc.v1.SignalingService/CallUpdate": true,
	"/proto.rpc.webrtc.v1.SignalingService/OptionalWebRTCConfig": true,
	"/proto.rpc.v1.AuthService/Authenticate": true,
	"/proto.rpc.v1.ExternalAuthService/AuthenticateTo": true,
	"/viam.robot.v1.RobotService/ResourceNames": true,
	"/viam.robot.v1.RobotService/ResourceRPCSubtypes": true,
	"/viam.robot.v1.RobotService/StartSession": true,
	"/viam.robot.v1.RobotService/SendSessionHeartbeat": true,
	}

, and then tries StartSession for everything else:

rdk/robot/client/client_session.go

Lines 122 to 132 in 3de60f9

	startResp, err := rc.client.StartSession(
	reqCtx,
	&startReq,
	grpc_retry.WithMax(5),
	grpc_retry.WithCodes(codes.InvalidArgument),
	)
	if err != nil {
	if s, ok := status.FromError(err); ok && s.Code() == codes.Unimplemented {
	falseVal := false
	rc.sessionsSupported = &falseVal
	rc.logger.CInfow(ctx, "sessions unsupported; will not try again")

This change, based on viamrobotics/viam-python-sdk#857, tests ex-ante if the safety_heartbeat_monitored RPC option is set (example), and will only start try StartSession if so (true = session; false/unspecified = no session). Reminder: one session at most per client (it initiates with the first RPC call that requires one).

Note, useSessionInRequest (and thus the new isSafetyHeartbeatMonitored) is appears to be called more times than necessary. It's called inside both the Unary (every time) and Stream (on first msg) interceptors before they call sessionMetadata that also calls it. I can look into reworking this if needed.

[aldenh-viam]

Tests are failing because MachineStatus no longer starts a session (as expected with this change). Working on updating the tests now.

[benjirewis]

Looks pretty good to me! Just some questions and nits.

[benjirewis]

Why not just use IsSafetyHeartbeatMonitored so you don't have to ignore the first two return values?

[aldenh-viam]

safetyMonitoredTypeAndMethod retrieves the method descriptor from the Robot (via a registry we manage ourselves), while the other gets it from protobuf's registry (populated as side effects of importing). It should be functionally the same, but the rest of the Interceptors code (m.safetyMonitoredResourceFromUnary/m.safetyMonitoredResourceFromStream) reaches into the Robot as well, so if there are no strong opinions either way, I'll leave it for consistency.

[cheukt]

generally looks good, can you also update session/doc.go to reflect current behavior?

[benjirewis]

LGTM. Per offline discussion: do we need sessionStreamClientInterceptor at all anymore? /Are we interested in attaching sessions to streams? Seems like we have no streaming APIs that need "safety" for actuation.

[aldenh-viam]

LGTM. Per offline discussion: do we need sessionStreamClientInterceptor at all anymore? /Are we interested in attaching sessions to streams? Seems like we have no streaming APIs that need "safety" for actuation.

Not for now (the only streaming RPC that currently has safety_heartbeat_monitored is viam.component.testecho.v1.TestEchoService/EchoMultiple). But it's possible in the future - imagine a streaming RPC with infrequent messages, but perhaps staying connected is critical?

[cheukt]

LGTM!