RSDK-9818: Annotate gRPC requests from modules to the viam-server with module names. #4749

dgottlieb · 2025-01-27T20:57:01Z

Pass a VIAM_MODULE_NAME env variable to module processes.

…h module names. Pass a VIAM_MODULE_NAME env variable to module processes.

dgottlieb · 2025-01-27T20:58:10Z

grpc/interceptors.go

@@ -43,3 +44,60 @@ func EnsureTimeoutUnaryClientInterceptor(

 	return invoker(ctx, method, req, reply, cc, opts...)
 }
+
+// The following code is for appending/extracting grpc metadata regarding module names/origins via


Shoving this all inside of grpc/interceptors.go seemed like a good spot, but open to alternatives. At first I tried inside the robot web server or modules directory (can't remember), but hit go import cycles.

This file seems like the appropriate place IMO.

dgottlieb · 2025-01-27T21:01:45Z

module/module.go

+	// Modules compiled against newer SDKs may be running against older `viam-server`s that do not
+	// provide the module name as an env variable.
+	if m.name != "" {
+		connectOptions = append(connectOptions, client.WithModName(m.name))


I can go back to the chaining in the old code -- I thought it looked nicer.

I only did this to make checking for the empty string to be explicit. But in reality, the other end calling GetModuleName copes with "old modules" by returning the empty string.

Maybe it'd be better for GetModuleName to return an error -- I don't know!

I don't have a strong opinion either way; I think what you have here is totally fine. GetModuleName returning an error in the event of no module name on the metadata/context seems odd to me, though, given we expect to be interacting with older modules and inability to find the module name is not really an "error state."

benjirewis

Looks pretty good; just some nits and questions.

benjirewis · 2025-01-28T15:45:23Z

grpc/interceptors.go

@@ -43,3 +44,60 @@ func EnsureTimeoutUnaryClientInterceptor(

 	return invoker(ctx, method, req, reply, cc, opts...)
 }
+
+// The following code is for appending/extracting grpc metadata regarding module names/origins via


This file seems like the appropriate place IMO.

benjirewis · 2025-01-28T15:47:33Z

grpc/interceptors.go

+	}
+
+	return handler(ctx, req)
+}


Do you not need Stream versions of the interceptors? Or do we only care about unary for now?

I only care about unary right now. I'm inclined to believe a stream would never need this? Or at least not in the in the same mold as we have here. I need to identify to module/connection such that I can use webrtc video streaming as opposed to a grpc stream.

But adding it couldn't hurt. Certainly would be surprising if one day someone did try using grabbing a module name from a stream request and failed to get it.

I take this back -- streams don't have contexts. We play games to make that a thing. I'm inclined to punt for now.

I'm inclined to believe a stream would never need this?

Well we certainly don't have any gRPC streaming API between a module and rdk right now. Whether we never will: I'm not sure. I think punting is totally fine for now. I do remember looking at that ServerStreamContextWrapper thing and scratching my head... I guess we'd have to do that in the future if we ever need the same feature for streaming gRPC APIs module <-> rdk 🤷🏻 .

benjirewis · 2025-01-28T15:50:29Z

module/module.go

@@ -175,6 +175,9 @@ type peerResourceState struct {

 // Module represents an external resource module that services components/services.
 type Module struct {
+	// The name of the module as per the robot config.


Suggested change

// The name of the module as per the robot config.

// The name of the module as indicated by `VIAM_MODULE_NAME`.

I realize it is the name from the config, but nice to note that it's derived from the env var?

I think where the name comes from is more important than how it gets here (from a holistic point of view). But it doesn't hurt to add that detail.

benjirewis · 2025-01-28T15:51:49Z

module/module.go

+	// Modules compiled against newer SDKs may be running against older `viam-server`s that do not
+	// provide the module name as an env variable.
+	if m.name != "" {
+		connectOptions = append(connectOptions, client.WithModName(m.name))


I don't have a strong opinion either way; I think what you have here is totally fine. GetModuleName returning an error in the event of no module name on the metadata/context seems odd to me, though, given we expect to be interacting with older modules and inability to find the module name is not really an "error state."

benjirewis · 2025-01-28T15:53:52Z

robot/client/client_options.go

@@ -56,6 +58,14 @@ func newFuncRobotClientOption(f func(*robotClientOpts)) *funcRobotClientOption {
 	}
 }

+// WithModName attaches a unary interceptor that attaches the module name for each outgoing gRPC
+// request.


Suggested change

// request.

// request. Should only be used in Viam module library code.

Don't really care about the wording of the messaging, but maybe just a line to indicate that Golang SDK users (not that there are many) shouldn't really touch this option.

benjirewis · 2025-01-28T15:55:36Z

robot/impl/local_robot_test.go

+				"reading": 42,
+			}, nil
+		},
+		CloseFunc: func(ctx context.Context) error {


Surprised you need a no-op CloseFunc; I assume you get a panic without this?

I copied it from elsewhere. But that was my reasoning -- was that it existed to avoid a panic.

benjirewis · 2025-01-28T15:57:47Z

module/testmodule/main.go

@@ -106,9 +107,21 @@ func mainWithArgs(ctx context.Context, args []string, logger logging.Logger) err
 func newHelper(
 	ctx context.Context, deps resource.Dependencies, conf resource.Config, logger logging.Logger,
 ) (resource.Resource, error) {
+	var dependsOnSensor sensor.Sensor


[opt] Doesn't really matter, but generally I've seen FromDependencies used instead of for loop searches like the one you're doing.

I made a change, but I feel there's still some awkwardness.

I think the trouble arises because this "helper" model is "schema-less". Most tests aren't creating this with dependencies (obviously as per needing this change). And if they did want dependencies, it's not clear they'd want a sensor one? I don't think there's really a way to make this dependency parsing usage for "helper" be forward compatible.

Cool; yeah it's still awkward, sorry about that. What you have seems fine. I think you're right that if we wanted this to "look good" (and I don't really care too much since this is just testing code) we'd need a type for helper configs and probably an associated Validate method.

benjirewis

LGTM! Thanks 🫡

RSDK-9818: Annotate gRPC requests from modules to the viam-server wit…

c3daac7

…h module names. Pass a VIAM_MODULE_NAME env variable to module processes.

dgottlieb requested a review from benjirewis January 27, 2025 20:57

dgottlieb changed the title ~~RSDK-9818: Annotate gRPC requests from modules to the viam-server wit…~~ RSDK-9818: Annotate gRPC requests from modules to the viam-server with module names. Jan 27, 2025

viambot added the safe to test This pull request is marked safe to test from a trusted zone label Jan 27, 2025

dgottlieb commented Jan 27, 2025

View reviewed changes

lint

5c3446a

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

benjirewis reviewed Jan 28, 2025

View reviewed changes

comments

bfda26d

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

lint

c897bd8

dgottlieb requested a review from benjirewis January 28, 2025 17:45

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

lint

8c0a9db

viambot added safe to test This pull request is marked safe to test from a trusted zone and removed safe to test This pull request is marked safe to test from a trusted zone labels Jan 28, 2025

benjirewis approved these changes Jan 28, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RSDK-9818: Annotate gRPC requests from modules to the viam-server with module names. #4749

RSDK-9818: Annotate gRPC requests from modules to the viam-server with module names. #4749

dgottlieb commented Jan 27, 2025 •

edited

Loading

dgottlieb Jan 27, 2025

benjirewis Jan 28, 2025

dgottlieb Jan 27, 2025

benjirewis Jan 28, 2025

benjirewis left a comment

benjirewis Jan 28, 2025

benjirewis Jan 28, 2025

dgottlieb Jan 28, 2025

dgottlieb Jan 28, 2025

benjirewis Jan 28, 2025

benjirewis Jan 28, 2025

dgottlieb Jan 28, 2025

benjirewis Jan 28, 2025

benjirewis Jan 28, 2025

dgottlieb Jan 28, 2025

benjirewis Jan 28, 2025

dgottlieb Jan 28, 2025

benjirewis Jan 28, 2025

dgottlieb Jan 28, 2025

benjirewis Jan 28, 2025

benjirewis left a comment

	// The name of the module as per the robot config.
	// The name of the module as indicated by `VIAM_MODULE_NAME`.

	// request.
	// request. Should only be used in Viam module library code.

RSDK-9818: Annotate gRPC requests from modules to the viam-server with module names. #4749

Are you sure you want to change the base?

RSDK-9818: Annotate gRPC requests from modules to the viam-server with module names. #4749

Conversation

dgottlieb commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjirewis left a comment

Choose a reason for hiding this comment

dgottlieb commented Jan 27, 2025 •

edited

Loading