Skip to content

Add LifeCycleHooks for adding callbacks to requests #96

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 15 commits into from
Closed

Add LifeCycleHooks for adding callbacks to requests #96

wants to merge 15 commits into from

Conversation

@davidpdrsn
Copy link
Member

@davidpdrsn davidpdrsn commented May 26, 2021 •
edited
Loading

Edit: Things have changed a bit since this description was written. See #96 (comment)

This adds metrics::Traffic which is a middleware for measuring requests per second/minute and error rates. Doesn't actually aggregate anything. Just facilitates calling some callbacks that do the actual work such as sending events to prometheus.

Example usage:

#[tokio::test]
async fn unary_request() {
    let mut svc = ServiceBuilder::new()
        .layer(TrafficLayer::new(
            ServerErrorsAsFailures::make_classifier(),
            MyCallbacks,
        ))
        .service_fn(echo);

    svc.ready()
        .await
        .unwrap()
        .call(Request::new(Body::from("foobar")))
        .await
        .unwrap();
}

async fn echo(req: Request<Body>) -> Result<Response<Body>, hyper::Error> {
    Ok(Response::new(req.into_body()))
}

#[derive(Clone)]
struct MyCallbacks;

struct Data {
    uri: Uri,
    method: Method,
    version: Version,
    request_received_at: Instant,
}

impl Callbacks<ServerErrorsFailureClass> for MyCallbacks {
    type Data = Data;

    fn prepare<B>(&mut self, request: &Request<B>) -> Self::Data {
        Data {
            uri: request.uri().clone(),
            method: request.method().clone(),
            version: request.version(),
            request_received_at: Instant::now(),
        }
    }

    fn on_response<B>(
        &mut self,
        response: &Response<B>,
        classification: ClassifiedResponse<ServerErrorsFailureClass, ()>,
        data: &mut Data,
    ) {
        // ...
    }

    fn on_eos(
        self,
        trailers: Option<&HeaderMap>,
        classification: Result<(), ServerErrorsFailureClass>,
        data: Data,
    ) {
        // ...
    }

    fn on_failure(
        self,
        failed_at: FailedAt,
        failure_classification: ServerErrorsFailureClass,
        data: Data,
    ) {
        // ...
    }
}

There are details about exactly when everything is called in the docs.

Part of #57

@davidpdrsn davidpdrsn added the A-new-middleware Area: new middleware proposals label May 26, 2021
davidpdrsn
davidpdrsn commented Jun 4, 2021
View reviewed changes
@davidpdrsn davidpdrsn marked this pull request as ready for review June 4, 2021 19:12
@davidpdrsn davidpdrsn requested a review from tobz June 4, 2021 19:12
@davidpdrsn
Copy link
Member Author

davidpdrsn commented Jun 4, 2021

@tobz would you maybe be up for reviewing this?

tobz
tobz reviewed Jun 16, 2021
View reviewed changes
tower-http/CHANGELOG.md
Comment on lines +10 to +11
- Add `metrics::Traffic` for measuring how many responses or errors a service is
producing.
Copy link
Member

@tobz tobz Jun 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is potentially a more philosophical thought but Traffic feels like it should almost do all of this, plus track response latency, plus track the in-flight count. The last major thing to track, IMO, is response latency... which would bring the middleware count up to three, based on us having InFlightRequests and Traffic right now.

Copy link
Member Author

@davidpdrsn davidpdrsn Jun 16, 2021

Choose a reason for hiding this comment

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

Yeah I've had similar thoughts. I've considered calling it something like RequestLifecycleCallbacks 😜 but better.

The intended use case is to use this together with some metrics backend chosen by the user. At Embark we use the metrics crate and prometheus meaning all the aggregation is taken care of.

As mentioned I also do want to provide another crate that provides ready to use integration between tower-http and the metrics crate. However I don't think baking the metrics crate directly in to tower-http is appropriate.

Copy link
Member Author

@davidpdrsn davidpdrsn Jun 18, 2021

Choose a reason for hiding this comment

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

@tobz Do you think think RequestResponseCallbacks would be a better name?

Copy link
Member

@tobz tobz Jun 21, 2021

Choose a reason for hiding this comment

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

Hmmm, something with lifecycle in the name definitely sounds better. Just spitballing, but maybe like... lifecycle hooks? Something like that.

Copy link
Member Author

@davidpdrsn davidpdrsn Jul 2, 2021

Choose a reason for hiding this comment

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

I've renamed and moved some things around. This middleware is now called LifeCycleHooks and lives in tower_http::life_cycle, so doesn't live in metrics anymore. I agree I think this makes more sense.

@tobz what do you think?

tobz
tobz requested changes Jun 16, 2021
View reviewed changes
Copy link
Member

@tobz tobz left a comment

Choose a reason for hiding this comment

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

Solid foundation so far.

@davidpdrsn davidpdrsn requested a review from tobz June 16, 2021 16:44
@davidpdrsn davidpdrsn changed the title Add Traffic middleware for measuring traffic and error rates Add LifeCycleHooks for adding callbacks to requests Jul 2, 2021
@davidpdrsn
Copy link
Member Author

davidpdrsn commented Jul 5, 2021

I think I'm going to close this for now. I'm not confident in the design. Specifically I think it would be cool to have something that was powerful enough to implement the Trace middleware on top of. That isn't possible with the current design.

@davidpdrsn davidpdrsn closed this Jul 5, 2021
@davidpdrsn davidpdrsn mentioned this pull request Nov 30, 2021
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tobz tobz Awaiting requested review from tobz

Assignees

No one assigned

Labels

A-new-middleware Area: new middleware proposals

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@davidpdrsn @tobz