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

Cannot use TraceLayer with GrpcService #973

Closed
conradludgate opened this issue Apr 14, 2022 · 6 comments
Closed

Cannot use TraceLayer with GrpcService #973

conradludgate opened this issue Apr 14, 2022 · 6 comments

Comments

@conradludgate
Copy link
Contributor

conradludgate commented Apr 14, 2022

Bug Report

Version

tonic: 0.7.1
prost: 0.10
tonic-build: 0.7.0
tower: 0.4
tower-http: 0.2.5

Platform

N/A

Crates

tonic/tower-http

Description

tower-http's trace::TraceLayer is incompatible with a tonic client GrpcService. It's Response does not implement Default, but since this change it's now required.

Demo

We use this layer to automatically propagate our opentelemetry contexts

@LucioFranco
Copy link
Member

cc @davidpdrsn

@davidpdrsn
Copy link
Member

If you box the response body like this it works:

let mut tester_client = TesterClient::new(
    ServiceBuilder::new()
        .layer(tower_http::map_response_body::MapResponseBodyLayer::new(http_body::Body::boxed_unsync))
        .layer(tower_http::trace::TraceLayer::new_for_grpc())
        .service(conn),
);

Requires adding the map-response-body feature to tower-http.

@davidpdrsn
Copy link
Member

Also, looking at https://github.com/NAlexPear/tonic/blob/2a61ea0e3e7e746f21a1f82f7d6e34bff58640c0/tonic-build/src/client.rs#L61 I think the T::ResponseBody: Default default bound should only be on with_interceptor and not on new. That'd be worth looking into I think. I believe default bodies are only used for interceptors errors.

@conradludgate
Copy link
Contributor Author

I think you are correct. I've forked locally and can confirm that new does not seem to need it, shall I make a PR?

@davidpdrsn
Copy link
Member

Sure!

@davidpdrsn
Copy link
Member

Fixed by #974

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 a pull request may close this issue.

3 participants