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

chore: fix gRPC multiplex example #2825

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

abhiaagarwal
Copy link

@abhiaagarwal abhiaagarwal commented Jul 9, 2024

Motivation

Updates the gRPC example with the new release of tonic that uses axum 0.7 and hyper 1. Closes #2736.

Solution

Owing to tower::steer, I was able to remove the entire multiplex service in multiplex_service.rs file. Additionally, tonic thankfully gives us a method to covert its server into axum routes, so we avoid all type inference issues (since they are the same types!)

It should be possible to merge the two routers together directly, but I haven't investigated how to make routing possible. I'm not also sure what will happen to the middleware associated with each router. I chose to keep them separate.

@jplatte jplatte mentioned this pull request Jul 10, 2024
@spencerbart
Copy link
Contributor

spencerbart commented Jul 11, 2024

Using TraceLayer::new_for_grpc() doesn't work and the traces don't print when multiplexing this way but works when just using tonic without multiplexing.
let layer = tower::ServiceBuilder::new()
    .layer(TraceLayer::new_for_grpc())
    .into_inner();

let grpc = TonicServer::builder()
    .layer(layer)
    .add_service(deck_svc)
    .into_router();

Here's an example

https://github.com/spencerbart/cards

Scratch all of that. I just needed to put .layer(TraceLayer::new_for_grpc()) after I call .into_router() on tonic's Server::builder().

@maxnachlinger
Copy link

maxnachlinger commented Jul 11, 2024

Scratch all of that. I just needed to put .layer(TraceLayer::new_for_grpc()) after I call .into_router() on tonic's Server::builder().

@spencerbart trivial point, but you can add your TraceLayer before calling into_router() too :)

    Server::builder()
        .layer(TraceLayer::new_for_grpc()) // <--
        .add_service(your_awesome_service)
        .into_router()

@spencerbart
Copy link
Contributor

Scratch all of that. I just needed to put .layer(TraceLayer::new_for_grpc()) after I call .into_router() on tonic's Server::builder().

@spencerbart trivial point, but you can add your TraceLayer before calling into_router() too :)

    Server::builder()
        .layer(TraceLayer::new_for_grpc()) // <--
        .add_service(your_awesome_service)
        .into_router()

@maxnachlinger That's what I had before and it didn't work.

For some reason calling .layer(TraceLayer::new_for_grpc()) before .into_router() doesn't log the traces. You have to create that layer after you call .into_router().

@maxnachlinger
Copy link

@spencerbart hah, sorry for the churn man :)

@abhiaagarwal
Copy link
Author

Yep, I agree that this is very much a minimal POC, and the actual interaction between the grpc and rest server and its associated middleware isn't really that great...

In an ideal world, we should be able to merge the routers together and route based on Content-type, which would needs its own middleware. I think it's possible but I haven't investigated it.

@brocaar
Copy link

brocaar commented Jul 25, 2024

I have been playing with this, but I have not (yet) solved how to work with layers using this example. As you already commented, when using Server::builder().layer(...).into_router(), it looks like the layer is not in the returned routes. On the other hand when using Server::builder().into_router().layer(...), I get type errors (e.g. when using the GrpcWebLayer, https://docs.rs/tonic-web/latest/tonic_web/).

@daheige
Copy link

daheige commented Jul 27, 2024

Yep, I agree that this is very much a minimal POC, and the actual interaction between the grpc and rest server and its associated middleware isn't really that great...

In an ideal world, we should be able to merge the routers together and route based on Content-type, which would needs its own middleware. I think it's possible but I haven't investigated it.

You're absolutely right. Thanks for the pull that allowed me to have the grpc service and http service run on the same port in tonic 0.12.1 release. Here is the crates package I used:

tonic = "0.12.1"
prost = "0.13.1"
tokio = {version = "1.38.0",features = ["full"]}
tower = { version = "0.4.13", features = ["steer"] }
# note: Must be the same as the tonic version
tonic-reflection = "0.12.1"
# note: Must be the same as the tonic version
tonic-build = "0.12.1"

https://github.com/daheige/rs-rpc/blob/main/Cargo.toml

Copy link
Member

@jplatte jplatte left a comment

Choose a reason for hiding this comment

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

Hey, sorry for letting this linger for so long!

I know next to nothing about GRPC, so it's hard for me to review this. @mladedav how about you?

Comment on lines +8 to +9
# needs to match tonic
axum = { version = "0.7.5" }
Copy link
Member

@jplatte jplatte Sep 28, 2024

Choose a reason for hiding this comment

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

I wouldn't want examples within this repo to pull in a crates.io release of axum. I'm considering moving examples to a separate repo anyways though, see #2942 for discussion.

@daheige
Copy link

daheige commented Sep 29, 2024

Hey, sorry for letting this linger for so long!

I know next to nothing about GRPC, so it's hard for me to review this. @mladedav how about you?

I used the solution mentioned above to merge multiple services into one service. This solution is indeed feasible. See the project link below for details.
https://github.com/daheige/rs-rpc/blob/main/src/multiplex_service.rs#L112

Copy link
Collaborator

@mladedav mladedav left a comment

Choose a reason for hiding this comment

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

I did use tonic before, but not like this so I still want to take a closer look to some of the parts used here, but I'd say this looks good.

I want to also double check if we can't make it work with a different versions of Axum between us and tonic. If we can't I'd vote to grant an exception here to depend on crates.io especially if we want to move it soon afterwards anyway.

Comment on lines +79 to +81
.map(|content_type| content_type.as_bytes())
.filter(|content_type| content_type.starts_with(b"application/grpc"))
.is_some()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit, you could do just .and_then(|content_type| content_type.as_bytes().starts_with(b"...").then_some(1)).unwrap_or(0) without an explicit condition instead.

(Although now I see you have just copied this from the deleted file)

@abhiaagarwal
Copy link
Author

I did use tonic before, but not like this so I still want to take a closer look to some of the parts used here, but I'd say this looks good.

I want to also double check if we can't make it work with a different versions of Axum between us and tonic. If we can't I'd vote to grant an exception here to depend on crates.io especially if we want to move it soon afterwards anyway.

I could probably do a cargo patch to resolve this. Lemme give it a shot

@mladedav
Copy link
Collaborator

mladedav commented Sep 30, 2024

If you mean doing [patch.crates-io] inside Cargo.toml, then I don't think we want to do that since once we make breaking changes, tonic in the example will fail to compile.

I see two options to reconcile different axum versions in the exmaple

  • You can map the response type of tonic's router to our response type.
    let grpc = Router::new().nest_service(
        "/",
        grpc.into_service()
            .map_response(|response| response.map(Body::new)),
    );

Then you can even just merge the two routers with let router = rest.merge(grpc); and you don't need Steer and Sharded at all. This works only if the two services don't use the same routes though. I think that's actually better than having completely different behavior based on whether content-type starts with application/grpc, but otherwise this seems like another instance of "I want to route based on something other than path/HTTP method". This compiles but does not run because both routers define a route for /.

  • You can create a layer between Steer and the two different routers so that it is happy with the types.
#[derive(Clone)]
enum MultiplexedService {
    Axum(axum::Router),
    Tonic(tonic::service::AxumRouter),
}

impl Service<Request<Body>> for MultiplexedService {
...
    fn call(&mut self, req: Request<Body>) -> Self::Future {
        match self {
            MultiplexedService::Axum(router) => Box::pin(router.call(req)),
            MultiplexedService::Tonic(router) => {
                let future = router.call(req);
                Box::pin(async {
                    let response = future.await;
                    response.map(|response| response.map(Body::new))
                })
            }
        }
    }
}

Also, it seems some of the methods from tonic you're using are already deprecated, could you check that please?

Comment on lines +69 to +72
let grpc = tonic::transport::Server::builder()
.add_service(reflection_service)
.add_service(GreeterServer::new(GrpcServiceImpl::default()))
.into_router();
Copy link

Choose a reason for hiding this comment

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

If I'm reading the docs correctly, I think you want something like:

Suggested change
let grpc = tonic::transport::Server::builder()
.add_service(reflection_service)
.add_service(GreeterServer::new(GrpcServiceImpl::default()))
.into_router();
let grpc = Routes::default()
.add_service(reflection_service)
.add_service(GreeterServer::new(GrpcServiceImpl::default()))
.into_axum_router();

which would avoid the deprecated into_router function. It was deprecated in 0.12.2 so it's pretty recent!

Thank you for this pr! I found it very helpful ❤️

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.

Update gRPC multixplex example for axum 0.7 and hyper 1
8 participants