Split out handler

[jcgruenhage]

From the source code:

// We short circuit the response status and body to serve the endpoint
// automagically. This way the user does not need to set the middleware *AND*
// an endpoint to serve middleware results. The user is only required to set
// the middleware and tell us what the endpoint should be.
if inner.matches(&path, &method) {
    head.status = StatusCode::OK;
    body = ResponseBody::Other(Body::from_message(inner.metrics()));
}

I'm not convinced that this is a good idea. new() could return a tuple of (middleware, handler), which would make the code a lot cleaner IMO.

One issue that came to mind quickly is that I want the metrics to include the processing times of all the middlewares, so it's registered as the outmost middleware, but that causes the logging middleware to log a 404 for /metrics, because it logs this before the prm middleware changes the status code and body.

Would you be okay with a PR that changes this behaviour?

[nlopes]

Would be indeed. I wonder if we could get two entry points for both kinds of behaviour.

[jcgruenhage]

Is there any reason for the current kind of behaviour except saving 2 lines of code for the user? I don't think saving 2 lines to add some obscure magic is worth it in any case.

[nlopes]

There's no heavy handed reason honestly. I start with the easiest for the user and complicate when needed. In this case it ends up being a breaking change, hence me asking.

I'm not fussed if this provides added value to the user (it seems like it does) and doesn't complicate the library ergonomics (doesn't sound like it).

[jcgruenhage]

Makes sense. So, would you be okay with a PR that changes this behaviour? It's a breaking change, yes, but this library is at an explicitly unstable version, so that is to be expected IMO

[nlopes]

Absolutely. Thank you so much for the work you're putting in, much appreciated.

[jcgruenhage]

I've started to implement this but ran into an error. actix-web requires the handler to be a static function, which it can't be because there needs to be a reference to the registry in there. We could circumvent this by using lazy_static, but that would mean that we can't support custom registries any more. I think the issues I'm facing in #6 are also related to the handling of references to the registry, so maybe pulling the registry into a lazy_static block wouldn't be so bad, and custom registries aren't that important if you can easily get a reference to the registry and reuse the one created by the middleware, right?

Any thoughts on this?

[nlopes]

This isn't forgotten, it's just that I'm on mobile (yearly vacation time). I should be able to give it a look this weekend.

[jcgruenhage]

@nlopes any update on this?

[nlopes]

Ha! Apologies, this time around I did indeed forget it. Let me take a look this week.

[ernestas-poskus]

Current coupled implementation forwards /metrics if .default_service is used which is not expected behaviour, would be good if one could mount /metrics handler as suggested here.

I'm not convinced that this is a good idea. new() could return a tuple of (middleware, handler),

to reproduce https://github.com/actix/examples/blob/master/http-proxy/src/main.rs#L97

[nlopes]

This is a good point. I'm yet to get some solid blocked time to go through this properly myself so bear with me please.

[nlopes]

@jcgruenhage Alright, I've thought about this now and I think this may be a better idea. If anyone can put a PR for this, I'll gladly take a look.

[jcgruenhage]

I'll try to get to it soon.