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

util: Add ServiceExt::map_future #542

Merged
merged 6 commits into from
Jan 27, 2021
Merged

util: Add ServiceExt::map_future #542

merged 6 commits into from
Jan 27, 2021

Conversation

davidpdrsn
Copy link
Member

I ran into a thing today where I wanted to write a middleware that wraps
all futures produced by a service in a tracing::Span. So something
like self.inner.call(req).instrument(span).

Afaik all the combinators we have today receive the value produced by
the future and not the future itself. At that point its too late to call
.instrument. So I thought this made sense to add a combinator for.

If you agree I'll go write some docs and clean things up.

@davidpdrsn davidpdrsn requested a review from hawkw January 20, 2021 23:03
@hawkw
Copy link
Member

hawkw commented Jan 20, 2021

I ran into a thing today where I wanted to write a middleware that wraps
all futures produced by a service in a tracing::Span

(fwiw, for this use case in particular, i really should work on upstreaming the tracing/tower integration code I've written into tower itself)

I think this is probably worth having for the general case, though it's kind of hard to explain why you would want it until you already understand why you would want it...not totally sold on the naming but I guess it's probably the best option...

@davidpdrsn davidpdrsn marked this pull request as ready for review January 21, 2021 09:51
@davidpdrsn
Copy link
Member Author

(fwiw, for this use case in particular, i really should work on upstreaming the tracing/tower integration code I've written into tower itself)

Perhaps 😅

I think this is probably worth having for the general case, though it's kind of hard to explain why you would want it until you already understand why you would want it...not totally sold on the naming but I guess it's probably the best option...

Yes agreed. Writing the docs was also a bit tricky without involving tracing::Instrument.

I'm not sold on the name either. I also considered map_call but IMO thats worse.

Regardless, I've cleaned things up so ready for review 👀

Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

this looks good to me! i had a few thoughts...

Comment on lines +49 to +57
impl<R, S, F, T, E, Fut> Service<R> for MapFuture<S, F>
where
S: Service<R>,
F: FnMut(S::Future) -> Fut,
E: From<S::Error>,
Fut: Future<Output = Result<T, E>>,
{
type Response = T;
type Error = E;
Copy link
Member

Choose a reason for hiding this comment

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

TIOLI: we could reduce the number of type parameters here using TryFuture:

Suggested change
impl<R, S, F, T, E, Fut> Service<R> for MapFuture<S, F>
where
S: Service<R>,
F: FnMut(S::Future) -> Fut,
E: From<S::Error>,
Fut: Future<Output = Result<T, E>>,
{
type Response = T;
type Error = E;
impl<R, S, F, Fut> Service<R> for MapFuture<S, F>
where
S: Service<R>,
F: FnMut(S::Future) -> Fut,
Fut: TryFuture,
Fut::Error: From<S::Error>,
{
type Response = Fut::Ok;
type Error = Fut::Error;

not a big deal though...

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting! How does this work in terms of not exposing things from the futures crate?

tower/src/util/map_future.rs Outdated Show resolved Hide resolved
tower/src/util/map_future.rs Outdated Show resolved Hide resolved
I ran into a thing today where I wanted to write a middleware that wraps
all futures produced by a service in a `tracing::Span`. So something
like `self.inner.call(req).instrument(span)`.

Afaik all the combinators we have today receive the value produced by
the future and not the future itself. At that point its too late to call
`.instrument`. So I thought this made sense to add a combinator for.
@davidpdrsn davidpdrsn merged commit d80044f into master Jan 27, 2021
@davidpdrsn davidpdrsn deleted the david/map-future branch January 27, 2021 08:39
davidpdrsn added a commit that referenced this pull request Feb 13, 2021
I forgot at add this one in #542. Think its nice to have for
consistency.
davidpdrsn added a commit that referenced this pull request Feb 13, 2021
I forgot at add this one in #542. Think its nice to have for
consistency.
davidpdrsn added a commit that referenced this pull request Feb 18, 2021
* builder: Add `ServiceBuilder::map_future`

I forgot at add this one in #542. Think its nice to have for
consistency.

* Update tower/src/builder/mod.rs

Co-authored-by: Eliza Weisman <[email protected]>

Co-authored-by: Eliza Weisman <[email protected]>
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.

2 participants