Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(microservices): add support for grpc reflection api #12899

Merged
merged 3 commits into from
Mar 18, 2024

Conversation

jtimmons
Copy link
Contributor

@jtimmons jtimmons commented Dec 7, 2023

PR Checklist

Please check if your PR fulfills the following requirements:

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Automatically add support for the gRPC reflection API if the user has the @grpc/reflection package installed

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

I currently maintain a Nest Module package (nestjs-grpc-reflection) which adds this capability to a user's app, but opening this PR as a discussion point for whether this makes sense to add to NestJS directly!

The gRPC server reflection API exposes information about a gRPC package so that clients can introspect it, this is gaining in popularity in developer tooling such as Postman, grpcui, ezy and so on as it removes the need for developers to manually manage proto file definitions and can instead rely on their client to fetch the correct definition automatically

@coveralls
Copy link

coveralls commented Dec 7, 2023

Pull Request Test Coverage Report for Build ac32cf05-85fc-4042-afa2-a4bcb1a35160

Details

  • 0 of 2 (0.0%) changed or added relevant lines in 1 file are covered.
  • 1 unchanged line in 1 file lost coverage.
  • Overall coverage decreased (-0.03%) to 92.135%

Changes Missing Coverage Covered Lines Changed/Added Lines %
packages/microservices/server/server-grpc.ts 0 2 0.0%
Files with Coverage Reduction New Missed Lines %
packages/microservices/server/server-grpc.ts 1 91.39%
Totals Coverage Status
Change from base Build 2bf2e4c8-5da9-4dd1-8f93-3b733029afc6: -0.03%
Covered Lines: 6736
Relevant Lines: 7311

💛 - Coveralls

@jtimmons jtimmons marked this pull request as ready for review January 12, 2024 03:13
@ssilve1989
Copy link
Contributor

👍 looking forward to seeing this functionality released!

@ssilve1989
Copy link
Contributor

@kamilmysliwiec any chance this can be reviewed?

@kamilmysliwiec
Copy link
Member

You can declare a new custom strategy that inherits from the ServerGrpc class and overrides one method (I'm assuming loadProto) to register gRPC reflection API. If we want to make it even simpler, I'd be happy to approve a PR that adds one extra method called, let's say, onPreLoadDefinition where anyone could hook any logic they need (before calling grpcPackage.loadPackageDefinition).

I see no need for lazily loading yet another library when this should be fairly simple to achieve without making any changes to the codebase.

@jtimmons
Copy link
Contributor Author

You can declare a new custom strategy that inherits from the ServerGrpc class and overrides one method (I'm assuming loadProto) to register gRPC reflection API. If we want to make it even simpler, I'd be happy to approve a PR that adds one extra method called, let's say, onPreLoadDefinition where anyone could hook any logic they need (before calling grpcPackage.loadPackageDefinition).

I see no need for lazily loading yet another library when this should be fairly simple to achieve without making any changes to the codebase.

sure, I can take a pass at one of those options and add to the documentation as a recipe? My thinking in integrating it more deeply into the ServerGrpc microservice strategy though is that:

  1. The reflection spec and @grpc/reflection package are part of the gRPC node ecosystem alongside the server and proto-loader that we're using already so it's more a feature of that ecosystem than something standalone
  2. Reflection as a whole is beginning to gain some good traction in gRPC community as it is a big improvement to the developer experience in local testing, for type/client generation of a live schema, etc

imo there's almost no reason to run a gRPC service without reflection these days so given the utility of it and it being central to the grpc-node ecosystem, it might make sense to try and make as frictionless as possible for people to enable?

@kamilmysliwiec
Copy link
Member

But this means adding yet another peer dependency to the @nestjs/microservices package which already has way too many of them.

I agree that @grpc/reflection package has several benefits and using it makes a lot of sense for most gRPC applications. What I'm trying to say is that we should rather look for an alternative API that would let users install this dependency themselves and register it through a dedicated API (instead of doing this internally within the @nestjs/microservices pkg under a flag).

@jtimmons
Copy link
Contributor Author

yep, that makes sense to me! So between the options we have, making a custom strategy feels a bit heavy-handed and isn't very composable with other changes like this in the future so I'm looking more into how we can inject the reflection service into the existing strategy. Feels like there's two paths here:

  1. We can add an event handler like the onPreLoadDefinition that you mentioned into our GrpcOptions. It feels like maybe the wrong place to include this since it's not configuration but the plus is that it'd be easy for people to integrate with their existing services
export const grpcClientOptions: GrpcOptions = {
  transport: Transport.GRPC,
  options: {
    ...
    onPreLoadDefinition: (pkg, server) => {
      new ReflectionService(pkg).addToServer(server);
    }
  },
}
  1. We could do the same on the raw ServerGrpc class and then have people load that as if it were a custom strategy. This is a bit more cumbersome for users but feels maybe a little more correct? The usage could look something like this:
  const grpcServer = ServerGrpc(grpcClientOptions.options);
  grpcServer.onPreLoadDefinition((pkg, server) => {
    const reflection = new ReflectionService(pkg);
    reflection.addToServer(server);
  });

  app.connectMicroservice({ strategy: grpcServer });

any preference between these directions or some other path I missed here @kamilmysliwiec?

@kamilmysliwiec
Copy link
Member

Let's pursue the number 1 option (easy to integrate with existing services and compatible with the regular gRPC microservice creation flow - with no need to explicitly import the server class)

@jtimmons jtimmons force-pushed the feat/grpc-reflection branch 3 times, most recently from 98a2f0c to 6c8fac7 Compare March 1, 2024 03:32
@jtimmons
Copy link
Contributor Author

jtimmons commented Mar 1, 2024

Let's pursue the number 1 option (easy to integrate with existing services and compatible with the regular gRPC microservice creation flow - with no need to explicitly import the server class)

great, just pushed those changes! Added a onLoadPackageDefinition hook to the GrpcOptions class and an example of usage to the grpc sample

@kamilmysliwiec kamilmysliwiec merged commit d5e2879 into nestjs:master Mar 18, 2024
3 checks passed
@kamilmysliwiec
Copy link
Member

LGTM

@jtimmons
Copy link
Contributor Author

@kamilmysliwiec a little late on this but finally put a PR up to document this new field's usage in nestjs/docs.nestjs.com#3027

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants