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

Server::trace_fn should receive full request, not just headers #630

Closed
polivbr opened this issue May 6, 2021 · 1 comment · Fixed by #634
Closed

Server::trace_fn should receive full request, not just headers #630

polivbr opened this issue May 6, 2021 · 1 comment · Fixed by #634
Labels

Comments

@polivbr
Copy link

polivbr commented May 6, 2021

Feature Request

Crates

tonic

Motivation

While the Server::trace_fn method allows a span to be created for each request being serviced, there is not enough information to create the span with meaningful data. For example, I would like to name the span after the request path to identify which service method is being invoked. However, trace_fn only has access to the headers, which don't contain this data. If trace_fn was instead passed the entire request, the request path (and a lot more) would be available for span construction.

Proposal

trace_fn should be invoked with the entire request, not just the headers. Unfortunately, this will require an API change.

Alternatives

I haven't attempted it, but it should be possible to use a full-blown interceptor as an alternative, albeit with a lot more work.

@davidpdrsn
Copy link
Member

I wonder if there is a reason the API wasn't designed like that to begin with 🤔 Could just be an oversight in which case I think it would be a good change.

I haven't attempted it, but it should be possible to use a full-blown interceptor as an alternative, albeit with a lot more work.

I don't think its possible to do with interceptors as they can only return new requests. There isn't a way for an interceptor to return a span to tonic for it to manage.

I think it would be possible using tower to make a middleware that creates a span an manages it itself but it would require quite a bit of boilerplate.

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

Successfully merging a pull request may close this issue.

2 participants