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

[axum-extra] metrics integration? #2988

Open
1 task done
oxalica opened this issue Oct 14, 2024 · 3 comments
Open
1 task done

[axum-extra] metrics integration? #2988

oxalica opened this issue Oct 14, 2024 · 3 comments

Comments

@oxalica
Copy link
Contributor

oxalica commented Oct 14, 2024

  • I have looked for existing issues (including closed) about this

Feature Request

Motivation

In #671, we added an example to manually instrument a single endpoint to support metrics. But the boilerplate is quite large for a middleware that wraps all requests track about request or response body, eg. body size, time-to-first-byte, time of round-trip and etc.

Proposal

Add a feature-gated module metrics in axum-extra to provide a MetricLayer middleware that instrument all requests and responses with metrics, maybe in a configurable way.

Since the middleware only does instrumentation, it only depends on metrics crate and is generic over export methods. It could be done in a few hundreds of lines, and users are free to customize export methods on their own: push metrics elsewhere, expose prometheus scrape endpoint as a route, or listen on another port for metrics.

This solution contrasts to axum-prometheus which is strongly coupled with prometheus exporter and does many hard-coded non-trivial things (e.g. always listen on localhost:9000).

Alternatives

  1. Do this in a separated crate like axum-metrics. This may be better depending on how many users want this feature.
  2. Do this in tower-http::metrics. This would not be easy to use since we almost always want a MatchedPath label. We could split the instrumentation with a generic trait ExtractLabels and impl ExtractLabels in axum-extra. But I doubt whether the increased complexity worth it, and this requires more boilerplate for axum users.
@yanns
Copy link
Collaborator

yanns commented Oct 14, 2024

Can you check again https://github.com/Ptrskay3/axum-prometheus?

I'm using since years, and it's using metrics, and does not force using localhost:9000.
You put the route("/metrics", get(|| async move { metric_handle.render() })) where you want.

@oxalica
Copy link
Contributor Author

oxalica commented Oct 14, 2024

Can you check again https://github.com/Ptrskay3/axum-prometheus?

I'm using since years, and it's using metrics, and does not force using localhost:9000. You put the route("/metrics", get(|| async move { metric_handle.render() })) where you want.

🤔 Ah, seems I'm encountering Ptrskay3/axum-prometheus#66 because axum-prometheus enables http-listener feature of metrics-exporter-prometheus unconditionally. I'm not sure which crate is to blame.

But that does not change the point though, axum-prometheus is more specialized than what I'm proposed here. This issue is more like: to upstream generic instrumentation code into axum-extra.

@Ptrskay3
Copy link
Contributor

Ptrskay3 commented Oct 14, 2024

axum-prometheus — despite its name — is not tightly coupled with Prometheus, it's rather centered around metrics.rs. You can just disable the default features, and bring your own exporter.

And if you plan to use Prometheus, then the http-listener feature is explicitly enabled, because .. well, you have to use something, and I assume that's the first thing people reach for. Let's note that metrics-exporter-prometheus also has that feature enabled by default (and also push-gateway).

Ptrskay3/axum-prometheus#66 actually comes from an unfortunate API in metrics-exporter-prometheus itself.. you may read more about it here: metrics-rs/metrics#501

However, you are not stuck with it, it's still possible to drop down lower level APIs of axum-prometheus, and avoid the issue yourself, using metrics_exporter_prometheus::PrometheusBuilder

Based on the discussion at this issue, I have a feeling that you're proposing here is probably 80-85% the same thing that axum-prometheus already provides.

Also tangentially related: Ptrskay3/axum-prometheus#66 (comment)

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

No branches or pull requests

3 participants