chore: fix gRPC multiplex example

[abhiaagarwal]

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.

[spencerbart]

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]

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]

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]

@spencerbart hah, sorry for the churn man :)

[abhiaagarwal]

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]

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]

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

[jplatte]

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?

[jplatte]

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]

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

[mladedav]

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.

[mladedav]

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]

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]

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?