Allow interceptors to implement their own CallOptions

[mwitkow]

grpc-ecosystem/go-grpc-middleware implements retry logic and other client-side interceptors.
See an example here:
https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/retry/options.go#L94

Now, it implements it by relying on an anonymous instantiation of grpc.CallOptions without providing an implementation. The client-side Unary and Stream interceptors then strip them before passing them to grpc.invoke.

However, this causes panics when users use the CallOptions without the interceptors, see:
grpc-ecosystem/go-grpc-middleware#37

I propose adding a grpc.NoOpCallOption that CallOptions implemented in middleware. This would implement both the private before and after methods without doing anything, keeping the callInfo private.

[mehrdada]

Have you considered carrying additional data in context (Context.WithValue) to accomplish what you need?

[mwitkow]

@mehrdada yes, I've considered it, and I do think such per-call option tweaks should be achieved by.. well a CallOption. It is a significantly more type-safe way of doing it.

This really doesn't seem like a massive ask, and actually would breed a nicer interface for the community of grpc-go interceptor authors to use.

[mehrdada]

We discussed this internally and some of us have mixed feelings about using a CallOption in that way, so we decided we are not ready to embrace expansion of the CallOption API, but we will keep that in mind as the situation evolves—I have tagged the issue as "suggestion" so we can continue thinking about it. Thank you for the suggestion.

[mwitkow]

@mehrdada can you provide the reasoning as to why this is bad? Not having it makes it hard to add extensions to grpc-go and build a viable ecosystem.

[mehrdada]

@mwitkow We do not think it is necessarily "bad" as you put it. Rather, it is not how CallOption were originally intended to be used. For what it's worth, a CallOption from a user's perspective looks like not much more than an enum-esque thing, and that's why the before/after methods are private in the first place. Right now, we have the freedom to completely change how CallOptions are implemented internally with very few restrictions without breaking users.

If I understand correctly, what you really want is an extensibility mechanism for adding custom hooks before and after a call. Right now, carrying additional data with context.WithValue that an interceptor can look into and check if it is of a certain type is one way to accomplish this†.

That said, it might indeed be a worthy goal to consider introducing more generic custom before/after hooks and we should give it some deep thought when we get a chance, and compare the various possible designs (and whether going the shortest path and making CallOption the mechanism to do so is the best option). I don't think we should jump through it and make this decision lightly simply because it is easy to add such feature with minimal code changes. Please also note that adding an API may be easy, but it is very hard for us to change or remove bad APIs.

[†] It's not clear to me how it would have negative type-safety implications relative to the suggested solution, as both options seem to have to basically deal with an interface{} object anyway.

[mwitkow]

Just to clarify, I'm not advocating exposing the before and after methods of the CallOption, interceptors are very much capable of that. In fact, that's exactly what the retry middleware does: it strips out its own CallOptions that drive the it's own before/after behaviours:
https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/retry/retry.go#L31

In terms of type safety, I do think that having your own CallOption type inherit from grpc.CallOption is much more type safe. It's narrowing down what the user can pass you trivial to separate the two:
https://github.com/grpc-ecosystem/go-grpc-middleware/blob/master/retry/options.go#L111

Moreover, this allows for much nicer call patterns that are more obvious to the user when they see CallOption:

stream, err := s.Client.PingList(parentCtx, goodPing, grpc_retry.WithMax(5))

instead of

stream, err := s.Client.PingList(grpc_retry.ContextOption(ctx, grpc_retry.WithMax(5)), goodPing)

Just to clarify: it is already possible for external developers to rely on CallOption anonymously, like I do. The reason why I'm proposing implementing:

type NoOpCallOption struct {}
func (o NoOpCallOption) before(c *callInfo) error { return nil }
func (o NoOpCallOption) after(c *callInfo)        {  }

is to make that already-possible practice safer (and avoid things lke grpc-ecosystem/go-grpc-middleware#37). The only way to avoid this practice to be used is through making CallOption private, but that would break existing code of end users anyway.