gRFC: NodeJS Client Interceptors

[Michael Lumish]

A proposal for a client interceptor API for the Node.js library has been created here: https://github.com/grpc/proposal/pull/14.

Please keep discussion about it on this thread.

[Michael Lumish]

I do not like the idea of directly exposing the internal grpc.Call API. It is internal for a reason, and it is not currently guaranteed to be stable. I would strongly prefer that the "outbound interceptor" and "inbound interceptor" APIs be the fundamental building blocks of this interceptor API.

[dd...@netflix.com]

I agree with hiding the grpc.Call API. I'm concerned about the usability of the inbound/outbound interceptor API for use cases like caching. When we need to change how operations are batched or we want to short circuit the remaining interceptor stack, a purely operation-specific API is awkward and difficult to reason about.

Would you consider an abstraction over the grpc.Call API like this?

{

    interceptCall: function(callConstructor, options) {
        var call = callConstructor(options);
        return (new InterceptingCallBuilder(call)).withInterceptBatch(function(batch, next, responder) {

            var cachedResponse = _getCachedResponse(batch);

            if (cachedResponse) {

                var response = (new ResponseBuilder).withMessage(cachedResponse).withStatus(grpc.status.OK).build();

                responder(null, response);

            } else {

                next(batch);

            }

        }).build();

    }

}

`batch` in this example would wrap the internal client_batch with some accessor methods: `hasMetadata`, `getMetadata`, etc.  The returned call's `interceptBatch` would have access to (and the ability to mutate) the full batch and could short circuit the remaining interceptor stack without exposing the internal API.

[dd...@netflix.com]

Looking at this more closely, I think a more flexible outbound/inbound interceptor API will cover the use cases I mentioned without a separate batch-handling API. I'll work on updating the proposal.

[dd...@netflix.com]

The proposal and the PR are now updated with the new API:

https://github.com/grpc/proposal/pull/14

https://github.com/grpc/grpc/pull/9842

[Michael Lumish]

Overall, I think it looks better now, but I have a few nitpicks.

The proposal says "The interceptCall method allows developers to modify the call options". What are the semantics of modifying these options, particularly the ones that apply to an entire client, not a single method (channelCredentials and host)? Is the MethodDescriptor object supposed to be modifiable?

The three advanced examples use "StatusBuilder", but it doesn't seem to be defined anywhere. They also only appear to work for unary calls. This should at least be mentioned.

In the Retry example, the call is retried for any non-OK status code. There are some nuances that should be considered there, like idempotency and the potential to increase traffic to overwhelmed servers that are timing out. We already have a more complete proposal for handling retries in the core at https://github.com/grpc/proposal/blob/master/A6.md, so I think it may not be the best idea to encourage people to retry calls with a relatively simple implementation at a higher level.

Also, the Caching example appears to use "savedMessage" and "storedMessage" interchangeably, and it does the same with "savedListener" and "storedListener". And the proposal should say "Implemented in: Javascript" at the top, instead of "Java, Go".

--
You received this message because you are subscribed to the Google Groups "grpc.io" group.
To unsubscribe from this group and stop receiving emails from it, send an email to grpc-io+u...@googlegroups.com.
To post to this group, send email to grp...@googlegroups.com.
Visit this group at https://groups.google.com/group/grpc-io.
To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/878bc96d-f6a7-4504-b106-6654b70de7d4%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[dd...@netflix.com]

The proposal says "The interceptCall method allows developers to modify the call options". What are the semantics of modifying these options, particularly the ones that apply to an entire client, not a single method (channelCredentials and host)? Is the MethodDescriptor object supposed to be modifiable?

The semantics of modifying the call options are whatever the semantics of modifying the options parameter to `getCall` in client.js are ( https://github.com/grpc/grpc/blob/master/src/node/src/client.js#L332 ) 

I could limit the options exposed to the options exposed in grpc-java's CallOptions class if that's preferrable:

deadline

authority (host)

credentials

customOptions

The MethodDescriptor should not be modified. I've added a note for that.

The three advanced examples use "StatusBuilder", but it doesn't seem to be defined anywhere.

I've added a full example of the StatusBuilder.

 

They also only appear to work for unary calls. This should at least be mentioned.

Each example lists the RPC types it supports:

An example of a caching interceptor for unary RPCs which stores the provided listener for later use (short-circuiting
the call if there is a cache hit):
An example retry interceptor for unary RPCs creates new calls when the status shows a failure:
An example of providing fallbacks to failed requests for unary or client-streaming RPCs:

I'll add comments to the example code to repeat this. 

 

In the Retry example, the call is retried for any non-OK status code. There are some nuances that should be considered there, like idempotency and the potential to increase traffic to overwhelmed servers that are timing out. We already have a more complete proposal for handling retries in the core at https://github.com/grpc/proposal/blob/master/A6.md, so I think it may not be the best idea to encourage people to retry calls with a relatively simple implementation at a higher level.

I can remove the retry example if it's not useful. 

Also, the Caching example appears to use "savedMessage" and "storedMessage" interchangeably, and it does the same with "savedListener" and "storedListener".  And the proposal should say "Implemented in: Javascript" at the top, instead of "Java, Go".

Fixed.

[Michael Lumish]

It's probably fine to leave all of the options as they are now, but options.credentials is a CallCredentials, not a ChannelCredentials. I think that threw me off.

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/0b852ae7-d5da-4302-9596-3d71884bc631%40googlegroups.com.

[dbo...@gmail.com]

Hello,

I do hope general  feedback from community at large is welcome. 

+1 for adding const's and exports for method call types. This is something I feel has been needed and would be very useful. I think it might be better to export this as a separate package altogether, and grpc can depend on it; that way modules can require it and have access to grpc types without having to depend on the whole grpc node library, but that's somewhat a minor point and outside of the scope of this discussion.

I do not have much in depth experience with gRPC in production, and just from my personal development (only with Node and a little with Go, no Java);.so maybe I am a little bit ignorant; but is there really a need for such a complicated and OOP-Y API for this? Javascript is function-first language with capability to pass around functions and objects alike simply and flexibly and most existing API's within the global Node.js ecosystem take this functional approach. We just need some way to expose an API to do pre, post hooks on events and provide means of composing middleware on client prototype functions? I really do not see why it would take 11 lines of code with 3 constructor invocations, and 2 build() calls to provide a mechanism to simply log a message being sent. What we really mean is just do this function before calling the client function.

function(message, next) {

    logger.log(message);

    next(message);

})

Yet we need all this extra work just to accomplish it.

Similarly for the retry example, given the choice of writing something like that code, or just using a single call to async.retry, the more readable code wins, even if you need some kind of custom logic.

Couldn't we just provide some mechanism of passing an object specifying the hooks we want. ie something like:

{

  onStart: function(... whatever) {

    // ...

  },

  onSend: function(message, next) {

  }, ...

}

etc...

If needed we can document the interfaces the passed around objects may have to provide, if you need separate interfaces for listener, interceptor, etc... They can just be simple objects? To me having constructors and builders is adding additional noise that doesn't help readability and just makes things harder to reason about.

I understand that this proposal is trying to expose something at a lower level so it doesn't have to account for different call contexts, etc, but given the ability that we can just pass around functions and arguments easily in javascript, I would think this should be doable.

I haven't dug too much into it, so I might be wrong or maybe I am missing something. Even if conditional or contextual logic is needed, one could argue that that type of work falls precisely inside the implementation of the library providing the API (grpc).

There are already existing examples of similar concepts of simpler functional hooks elsewhere in Node such as modules: kareem-hooks, hooks-fixed, grappling-hooks, and the hooks in mongoose.js for example.

I wouldn't be surprised that if this was implemented eventually it would be abstracted into a higher level API to allow for a (IMO) saner usage. And maybe that's fine or intended.

It's mentioned that a separate proposal for server interceptors using similar concepts and interfaces will be provided at a later date. Given the ubiquity of Express, and to smaller extend Restify and koa, all which provide API for composable middleware using just a simple `use` function and predetermined paradigms / idioms, I think similar API readability issues may be encountered. As a side project I've already created a simple module on top of existing grpc lib that provides similar API and functionality (https://github.com/malijs/mali). Not sure why exposing something complicated is needed. It is a personal side project that I can work on only as my life / time allows, but maybe when I get a chance I'll try and look into RFC process and try and document / propose something for server side. But again maybe the intention is to provide some kind of lower level typed interfaces, still leaving the ability to build on top to others, like what I've done. 

Anyway just my $0.02. Sorry I am not being negative on the proposal and since grpc is not something I use daily professionally at present I am probably somewhat ignorant and also not very invested. I am sure others in the community and this thread know better than me, but I am just trying to provide my objective opinion purely on the API based on my experience within the overall Node ecosystem.

Regards,

Bojan

[dd...@netflix.com]

Bojan,

Thanks for the feedback. I think making wider use of the RPC type enums is a good idea. I don't want to add any more complexity to this PR if I can avoid it but I'd support a follow-up PR.

I'm open to simplifying the API syntax if the same functionality is supported (all the functionality supported by the Java client interceptor API). Ditching the interceptor object and making the `interceptCall` function the 'interceptor' would be a simplification without removing functionality. A simple intercepted call would then look like this:

const interceptor = function(options) {
    return new InterceptingCall(options, {
        sendMessage: function(message, next) {
            next(_doSomething(message));
        }
    });
};
client.myCall(message, { interceptors: [interceptor] });

Thoughts?

[dbo...@gmail.com]

Hello,

+1 If that can be made to work to satisfy all the functionality that you're looking for, personally I think that API is MUCH better and more readable :)

Regards, 

Bojan

[dd...@netflix.com]

That's the same API as proposed, except with the interceptCall function promoted to represent the interceptor.  Maybe the examples are confusing because they use the builders? This is the same example but using the API in the proposal:

const interceptor = {
    interceptCall: function(options) {

        return new InterceptingCall(options, {
            sendMessage: function(message, next) {
                next(_doSomething(message));
            }
        });
    }
};
client.myCall(message, { interceptors: [interceptor] });

[dbo...@gmail.com]

On Monday, 13 March 2017 01:19:21 UTC-3, dd...@netflix.com wrote:

That's the same API as proposed, except with the interceptCall function promoted to represent the interceptor.  Maybe the examples are confusing because they use the builders? This is the same example but using the API in the proposal:

const interceptor = {
    interceptCall: function(options) {
        return new InterceptingCall(options, {
            sendMessage: function(message, next) {
                next(_doSomething(message));
            }
        });
    }
};
client.myCall(message, { interceptors: [interceptor] });

Hello, 

Are you saying that this code above would work already in the current proposal? With just setting properties within creation of new InterceptingCall, and without having to use the InterceptorBuilder and CallerBuilder? Sorry, that's not how I understood it but I may have missed something. To me this code above is a lot more readable than having to use the builders. If the above code would work with the existing proposal I'm +1 :) 

My initial feedback was based on initial impressions when looking at the proposal and reading through the examples given. I did not find the code as readable and as intuitive as I think it could be, most of it being due to the heavy builder approach. 

I understand fluent interfaces (pattern of construction via with* / chained methods and builders as suggested) may make more sense for strongly typed languages; IMO in Javascript they only make sense when dealing with for DSL-y things where they help readability (like DB query builders for example) or where they may actually have execution effects (ie stream processing, and action chaining). So in this case personally I felt the whole OOP and builder approach was perhaps a bit overkill and not very intuitive. 

Having said that I did look into the Java documentation (I have 0 experience with gRPC in Java) and it seems that this builder approach is used extensively.It's also present in grpc-go within DialOption type. This proposal PR seems very similar to the Java API. I suppose there could be an argument made to keep consistency in the API between the platforms. If this is a goal, I am fine with that, and more platform-appropriate abstractions can be made on top if needed. I think there may already be some examples of this within Go grpc eco system.

Personally I do find the approach of keeping things simple and just setting object properties more intuitive and in line with most existing Javascript API's. I suppose in some ways it comes down to on project goals. Do we want to keep API consistency between platforms, or tune the API to follow more platform-specific approaches that may add to readability and ease of entry (but go against consistency)?

Anyway this is just my opinion. I am certainly not an authority on grpc and if other more knowledgeable people have provided feedback and agreed to the proposal that is cool with me. I don't want to hold anything back or enter indecision deadlock and prevent this from moving through the proposal process if everyone else is good with it.

Thanks for reading,

Bojan

[dd...@netflix.com]

I've updated the gRPC PR and the proposal to promote the `interceptCall` function to represent the full interceptor, and also to clarify that use of the builders is optional.

[Michael Lumish]

Can you show in the proposal what the trivial implementation of all of the interception methods would look like without the builders?

To view this discussion on the web visit https://groups.google.com/d/msgid/grpc-io/504cc473-66c5-4f1b-92f1-34f3414ccf15%40googlegroups.com.

[dd...@netflix.com]

Ok, updated the proposal to provide a full example without builders.

[dbo...@gmail.com]

This looks a lot clearer. Thanks for the clarification. Sorry I misunderstood the original proposal as documented.

[dd...@netflix.com]

@Michael Lumish

Are there any unaddressed concerns with the proposal?