metadata for client interceptors, similar to what is available to server interceptors

[jhump]

The ServiceDesc struct contains a Metadata field that protoc-gen-go populates with the name of the file in which the service was defined. Through round-about mechanisms, this in turn allows server code to access proto descriptors for the services/methods, which enables data-driven interceptors where the data that drives policy is in the form of custom options on services and methods (as defined in proto source).

There is no analog in the client. Ideally, there would be some mechanism for client code to also get that same metadata (e.g. the name of the file in which service is defined). Currently, a streaming client interceptor can get access to a *grpc.StreamDesc. So the metadata could possibly be stored there. But the unary interceptor has access to nothing of value for this. So it would have to be supplied, instead, via context value.

This is related to #1526.

[dfawley]

This SGTM as a short-term fix to this problem with the caveat that the interceptor API is marked as experimental, and I fully intend to redesign it in the near future. Let's track that effort in #1805.

[jhump]

@dfawley, curious what you think about this approach:
master...jhump:jh/supply-service-desc-to-interceptors

It provides a uniform way for interceptors (both server and client) as well as server handlers to access the metadata, using the existing grpc.ServerInfo type. Since it would be a breaking/incompatible change to modify the unary interceptor signature, the info must be supplied via context and also must come from generated code.

The above diff shows two commits -- the first is small API addition for querying the grpc.ServerInfo from context (as well as putting it into context for server calls), and the second API is re-generating all of the protos using a modified protoc-gen-go. The change to protoc-gen-go is what actually puts the grpc.ServerInfo into context for client-side stuff. You can see that diff here:
golang/protobuf@master...jhump:jh/supply-service-desc-to-interceptors

What do you think? Should I put these out as pull requests, or can you think of better ways to do this?

I think what I would prefer (but it would be incompatible) would be if ServiceInfo were an interface, that *ServiceDesc implemented. That way, that could be used to expose the information in various contexts without the risk of user code ending up with a mutable reference to a global/package variable. It would also consolidate a bit, since it feels like there are multiple ways to supply almost-the-same information to things like interceptors or consumers of grpc.Server, just with various slight differences.

I'll freely admit that it feels gross to stuff more junk into context. I just don't see any other way to do it and still maintain API-compatibility.

[jhump]

@dfawley, ping (please see previous comment)

[dfawley]

@jhump sorry for the delay. I talked about this for a long time yesterday with others on my team. This limitation seems somewhat related to the general problem in Go's proto library of the service descriptor protos being hard to access (golang/protobuf#489). The only way to get them is via the filename containing the service. If there was another way to look them up using the service or package name, then I don't think we would need anything special in the grpc codegen for this at all. (Does that sound correct to you?)

As a similar approach, but without waiting for the proto redesign in golang/protobuf#364, perhaps we could add a grpc service registry in the same vein. At init() time, we would register a map from service (or service/method) name to the parent FileDescriptorProto. This would avoid the need to store the data in the context on the critical RPC path. (This is still just a brainstormed idea / strawman and not necessarily the approach I think we should take.)

[jhump]

@dfawley, I do like that idea: having a registry, keyed by fully-qualified service name, could be generally useful for reflective operations around RPCs. At Square, the custom proto-based RPC we built worked this way (via a fork of protoc-gen-go, written before grpc existed so the fork also generated the service stubs and interfaces).

Thinking about it more, I believe this does indeed satisfy this issue quite well. It does not need any changes to the existing stub/interceptor signatures and does not need any more cruft stashed away in the context.

Do you know if there is already an issue in the protobuf project about this (registry of known services)? If not, I can add one. (Seems like a protobuf project thing, not grpc-go, since that is where protoc-gen-go lives; it seems appropriate to keep the registry in the same package as the existing message, enum, and extension registries.)

[dfawley]

Do you know if there is already an issue in the protobuf project about this (registry of known services)?

I added a question in this old/closed issue (golang/protobuf#489 (comment)) to see if there is already something like that planned for the v2 API. I believe the answer to that question will be "no", at which point we could reopen the issue or file a feature request for it.

We can see what happens from there, then? If it can be added to the v1 API, and reasonably soon, then maybe we can close this issue. But if not, or if they prefer not to implement it, we could add a registry to grpc.

[dfawley]

Update: golang/protobuf#489 (comment)

Looks like the v2 API will support exactly this need. Do you still need this functionality before that is ready, and is there a low-impact, temporary solution that could work for you if so?

[jhump]

@dfawley, that sounds good. Nothing really urgent here.

We very well may need something before the v2 protobuf API is done, but we can do something custom. We already use an alternate protoc plugin to generate stubs: protoc-gen-grpchan. So we could just do something in that generated code to make descriptors available to client interceptors (likely in an internal fork -- doesn't seem worth open-sourcing if a better solution from the official protobuf library isn't far off).

[dfawley]

Given that proto API v2 will make this change unnecessary, and I believe that's ultimately the "right" fix, I'm going to close this. Please feel free to comment if you have any other concerns.