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

Support {:all, endpoint} in :services to auto-discover #49

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,14 +47,15 @@ This is written and tested using [grpcurl](https://github.com/fullstorydev/grpcu
end
```
or both as desired. `version` is the grpc reflection spec, which can be `v1` or `v1alpha`. `services` is the services that will be exposed by that server by reflection. You can expose a service through both services if desired.
1. Add the reflection supervisor to your supervision tree to host the cached reflection state
Instead of using an explicit list of services, you may alternatively use `{:all, MyApp.GrpcEndpoint}`, to include all services from an endpoint. This is a more succint alternative if you want to expose reflection for all services in your endpoint. On the other hand, you lose the fine-grained control of the ehaustive list.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this list is starting to get a bit dense with the config options inlined in these paragraphs. Now that services has multiple forms what do you think about simplifying this instruction list and moving the config descriptions into a separate block? I think the two forms for the content of the services entry should be together.

3. Add the reflection supervisor to your supervision tree to host the cached reflection state
```elixir
children = [
...other children,
GrpcReflection
]
```
1. Add your servers to your grpc endpoint
4. Add your servers to your grpc endpoint

## interacting with your reflection server

Expand Down Expand Up @@ -82,7 +83,7 @@ message HelloReply {
$ grpcurl -plaintext -format text -d 'name: "faker"' localhost:50051 helloworld.Greeter.SayHello
message: "Hello faker"
today: <
seconds:1708412184 nanos:671267628
seconds:1708412184 nanos:671267628
>
```

Expand Down
4 changes: 2 additions & 2 deletions lib/grpc_reflection.ex
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ defmodule GrpcReflection do
defmodule Helloworld.Reflection.Server do
use GrpcReflection.Server,
version: :v1,
services: [Helloworld.Greeter.Service]
services: [Helloworld.Greeter.Service] # alternatively, `{:all, HelloWorld.Endpoint}` can be used to include all services from the endpoint.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't describe version: :v1 vs version: v1alpha here. If you want to include this breakout here I would suggest putting in a stand-in for services and version that can be described in text outside of the code block.

Maybe something like:

 defmodule Helloworld.Reflection.Server do
     use GrpcReflection.Server,
         version: <version>,
         services: <services>
 end

Where <version> is either :v1 or :v1alpha, and <services> is either a list of GRPC services to reflect, or the tuple {:all, Endpoint} to reflect all services from that endpoint

end
```

2. Add the reflection supervisor to your supervision tree
```elixir
children = [
GrpcReflection,
{GRPC.Server.Supervisor, endpoint: Helloworld.Endpoint, port: 50051, start_server: true},
GrpcReflection
Comment on lines +21 to -22
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not related to the PR, but I believe this is the correct order, as the server has dependency on infra started by the GrpcReflection supervisor.

]
```

Expand Down
12 changes: 12 additions & 0 deletions lib/grpc_reflection/service/agent.ex
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,12 @@ defmodule GrpcReflection.Service.Agent do
name = Keyword.get(opts, :name)
services = Keyword.get(opts, :services)

services =
case services do
{:all, endpoint} -> endpoint_services(endpoint)
list -> list
end

case Builder.build_reflection_tree(services) do
{:ok, state} ->
Agent.start_link(fn -> state end, name: name)
Expand All @@ -24,6 +30,12 @@ defmodule GrpcReflection.Service.Agent do
end
end

defp endpoint_services(endpoint) do
:servers
|> endpoint.__meta__()
|> Enum.map(fn server -> server.__meta__(:service) end)
end

@spec list_services(cfg_t()) :: list(binary)
def list_services(cfg) do
name = start_agent_on_first_call(cfg)
Expand Down
15 changes: 15 additions & 0 deletions test/grpc_reflection_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
use GrpcCase

defmodule Service do
use GrpcReflection.Server, version: :v1

Check warning on line 7 in test/grpc_reflection_test.exs

View workflow job for this annotation

GitHub Actions / Check elixir+otp versions (1.17.x, 25.3.x)

using map.field notation (without parentheses) to invoke function Grpc.Reflection.V1.ServerReflection.Service.__rpc_calls__() is deprecated, you must add parentheses instead: remote.function()
end

test "adding a service changes responses" do
Expand Down Expand Up @@ -68,4 +68,19 @@
Service.get_by_filename("helloworld.HelloRequest.proto")
end
end

test "server reading services from endpoint" do
defmodule AutoServiceDiscoveryServer do
use GrpcReflection.Server,

Check warning on line 74 in test/grpc_reflection_test.exs

View workflow job for this annotation

GitHub Actions / Check elixir+otp versions (1.17.x, 25.3.x)

using map.field notation (without parentheses) to invoke function Grpc.Reflection.V1.ServerReflection.Service.__rpc_calls__() is deprecated, you must add parentheses instead: remote.function()
version: :v1,
services: {:all, GrpcReflection.TestEndpoint.Endpoint}
end

assert AutoServiceDiscoveryServer.list_services() ==
[
"grpc.reflection.v1alpha.ServerReflection",
"grpc.reflection.v1.ServerReflection",
"helloworld.Greeter"
]
end
end
Loading