loopbackio / loopback-next

LoopBack makes it easy to build modern API applications that require complex integrations.
https://loopback.io
Other
4.95k stars 1.07k forks source link

Spike: gRPC support for controllers #521

Closed bajtos closed 7 years ago

bajtos commented 7 years ago

What's needed to make the current REST interface a plugable component and make it easy to add gRPC support as another component.

See https://github.com/strongloop/loopback-next/issues/440

bajtos commented 7 years ago

What have I learned so far:

bajtos commented 7 years ago

Messages definitions (schemas) are based on Protocol Buffers - see https://developers.google.com/protocol-buffers/docs/overview. (Compare to Swagger/OpenAPI Spec which is based on JSON Schema.)

bajtos commented 7 years ago

@raymondfeng @kjdelisle I have the first iteration of my gRPC spike ready for a high-level review, see https://github.com/strongloop/loopback-next/pull/571. Please focus more on the developer experience of people using loopback next, the actual implementation is intentionally dirty.

My next step is to bring in Sequence concept to allow applications to customize the gRPC request handling flow and then try to extend our authentication component to support gRPC too.


I have a major concern about the official grpc implementation for Node.js (https://www.npmjs.com/package/grpc): it is a thin wrapper around C/C++ based implementation, which has several downsides:

BTW, both the server and the client are callback-based only and do not support Promise-style invocation. We will need wrappers to support our async/await style, as can be already seen in my spike pull request. Not a big deal deal, but it adds a bit of extra complexity.


I started my spike with the easiest path, which is to describe the RPC API in a proto file. I think this is similar to API-first approach for building REST APIs, where we start with OpenAPI Spec document.

const helloService = grpc.load(path.join(__dirname, 'hello.proto')).hello;

@service(helloService.Greeter.service)
class MyController implements Greeter {
  hello({name}: HelloRequest): HelloResponse {
    return {message: `hello ${name}`};
  }
}

Do we want to support code-first approach too, where the service/payload specs are built dynamically from the metadata provided by decorators? I am not very familiar with protocol buffers and the implementation used by gRPC in particular, so it's hard to tell for me how easy/difficult this will be to implement.

The .proto format provides quite a lot of type information about the services, request and response payloads. I think it will be very valuable to generate TypeScript interfaces representing the entities defined in .proto files. On the second thought, I think something similar can be done for people starting with an OpenAPI Spec document too. Obviously, this is out of scope of this initial spike and possibly even Core-GA release, but I'd like to know your thoughts - is this something to explore (i.e. create follow-up GitHub issues)?

On a related note: gRPC supports static and dynamic loading of proto specs. My initial spike is using dynamic loading, I think we should check static loading too to understand the differences. It may affect the way how we want to work with .proto and .d.ts files.

ritch commented 7 years ago

I would be -1 for generating a HelloRequest type def from proto, since the whole point is that the proto has the type def already.

Does TypeScript allow you to define types dynamically (eg. plug into the type loading system and add some types)? Seems like a pretty great thing to have for our use cases... eg. OpenAPI schemas => types, protos => types, etc.

Having redundant definitions of types seems like a bad idea (hello.proto + HelloRequest.ts), but it would be better than no types at all. All of the above applies to OpenAPI definitions/schemas.

kjdelisle commented 7 years ago

@ritch I don't think anything you generate at compile time in that way would be available in JavaScript; it might be possible to do that with classes, but for arbitrary types in general, it's all lost unless you capture it with Reflector.defineMetadata(...), and then we'd have to scaffold out the retrieval of that metadata at runtime.

kjdelisle commented 7 years ago

We are introducing a native addon. Users will need a correctly configured build environment on their dev/test/production machines, this used to be a major pain point in the past.

+1, we'll have to mitigate this with explicit documentation that shouts very loudly at prospective users to ensure they're aware of the pitfalls of compile-local, deploy-remote that this brings into the mix

The grpc server implementation is inside the C/C++ code, it creates a parallel networking (TCP & HTTP2) stack to the one included in Node.js. My main concern is that we are loosing visibility and inspectability. The usual tools for monitoring Node.js app performance (CPU usage analysis, event-loop latency) will not work for the gRPC part. Similarly for error handling and post-mortem debugging.

This is a big part of why my current refactoring effort is bringing the idea of Server back. As @virkt25 and I were working on this yesterday, we came across so many items that are implementation-specific, like how we bind controllers to the application, how components themselves are affected by this, etc. If logging, debugging and the like need changes as well, then it makes sense that they would come as part of a complete grpc package that's more of a turn-key item you bolt onto your Application at app start:

// first param is "namespace" for the binding
app.server("grpcServer", new GrpcServer( { ... } );
// server would have parent context that it can use to integrate logging output, etc.

I'm mostly done the first draft of the work, just need to refactor the tests accordingly and it should be up soon.

raymondfeng commented 7 years ago

@bajtos Great stuff. See my comments at https://github.com/strongloop/loopback-next/pull/571.

Ideally, the grpc support should be packaged as a component.

bajtos commented 7 years ago

Thank you for your comments!

@ritch

I would be -1 for generating a HelloRequest type def from proto, since the whole point is that the proto has the type def already. Having redundant definitions of types seems like a bad idea (hello.proto + HelloRequest.ts), but it would be better than no types at all.

I find your comment little bit confusing, maybe we misunderstood each other? What I am proposing is that developers should not need to maintain HelloRequest definition in two places (.proto, .ts/.d.ts), there should be a single source of truth. The first solution that comes to my mind is to write a code generator that can load .proto files and emit .ts files in a pre-compile step.

Does TypeScript allow you to define types dynamically (eg. plug into the type loading system and add some types)?

I think adding (global) types is not enough, because your code needs to import those types from somewhere (a module, a source code). I think we may be able to customize module resolution (see docs) to allow something like the following:

import {HelloRequest} from './hello.proto';

IIUC, this will require app developers to invoke TypeScript compiler programmatically via the API (or via our compiler helper), which may be a problem for people that want to use their own build system (based e.g. on babel, grunt or gulp).

@kjdelisle

I don't think anything you generate at compile time in that way would be available in JavaScript

The HelloRequest and HelloResponse interfaces are used by TypeScript type checker at compile time only, they don't exist at run time. I think that's one of the reasons why dynamically-loaded proto files are so easy to use in JavaScript, because it's so easy to create anonymous (type-less) objects.

This is a big part of why my current refactoring effort is bringing the idea of Server back. As @virkt25 and I were working on this yesterday, we came across so many items that are implementation-specific, like how we bind controllers to the application, how components themselves are affected by this, etc.

In my spike, I used a shared app.controller API for registering controllers, and so far it seems to work pretty well for me. As far as I am concerned, I don't see any pressing needs for bringing the server back. Having said that, I am sure you two have different perspectives, so I am looking forward to see the outcome of your refactoring!

bajtos commented 7 years ago

I pushed a new commit to the spike pull request where I am adding very basic support for gRPC sequences - see https://github.com/strongloop/loopback-next/pull/571/commits/f2ea3a7c9295efec17dfe7e4359388f260635ae7.

The good news: our foundation is solid enough to allow extensions to contribute additional transports.

Other takeaways:

@ritch @kjdelisle @raymondfeng thoughts?

raymondfeng commented 7 years ago

@bajtos Great progress!

raymondfeng commented 7 years ago

I wonder if the sequence can be further divided to reflect the relation between transports and protocols as follows:

raymondfeng commented 7 years ago

BTW, should we create a RFC on the wiki page?

bajtos commented 7 years ago

@raymondfeng

I wonder if the sequence can be further divided to reflect the relation between transports and protocols as follows

Right now, the sequence contract is an interface modeled for the needs of the consumers (REST transport, gRPC transport). The similarities caused by the same underlying protocol (HTTP/1.1, HTTP2) are just an implementation detail. I suppose we can extract some shared methods into a sequence base classes, something like DefaultHttpHandler, but then one of the golden rules for object-orientated design says "prefer composition over inheritance", so I don't think this is a good idea, at least at this point in time.

BTW, should we create a RFC on the wiki page?

I don't know, what would be the content of such RFC page? In this spike, I focused on verifying whether the current design of loopback core is flexible enough to support gRPC transport, I didn't flesh out all details of the future gRPC implementation - more work would be needed for that.

@raymondfeng Could you perhaps write a short outline showing what kind of things you would like the RFC to cover? Then I can open a new GH issue to keep track of that work.

bajtos commented 7 years ago

@raymondfeng I created a story for writing RFC/design proposal, please review and let me know if there is anything else to add to the acceptance criteria: https://github.com/strongloop/loopback-next/issues/582

I am closing this as done.