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

Feature: Expose a request path label in the http_request_* metric by default #491

Closed
zephinzer opened this issue Nov 5, 2018 · 7 comments
Assignees

Comments

@zephinzer
Copy link

zephinzer commented Nov 5, 2018

Currently, metrics are exposed as:

http_request_duration_microseconds{handler="users",quantile="0.5"} 401.237
...
http_request_duration_microseconds{handler="users",quantile="0.5"} 652.793
...
http_request_duration_microseconds{handler="users",quantile="0.5"} 652.793

I would like to add on to this by also allowing for a path label to be added so that each route does not need to be manually instrumented with a handler name. I'm proposing that the default collection should result in:

http_request_duration_microseconds{handler="users",path="/users/logout",quantile="0.5"} 401.237
...
http_request_duration_microseconds{handler="users",path="/users/login",quantile="0.5"} 652.793
...
http_request_duration_microseconds{handler="users",path="/users/status",quantile="0.5"} 652.793
...
# other examples I think would be useful
http_requests_total{code="200",handler="users",path="/users/status",method="get"} 3
promhttp_metric_handler_requests_total{path="/some/other/endpoint",code="200"} 2
promhttp_metric_handler_requests_total{path="/another/endpoint",code="500"} 0
promhttp_metric_handler_requests_total{path="/yet/another/endpoint",code="503"} 0

This could be done by adding r.URL.Path to the labels creation as a path label.

I'm new to Golang but would be happy to attempt this with some guidance!

Any opinions?

@beorn7
Copy link
Member

beorn7 commented Nov 5, 2018

Yeah, this would be pretty straight-forward to add.

IIRC, when implementing the middlewares, we were concerned that it's very easy to create cardinality explosions. @stuartnelson3 do you remember any of such concerns?

@beorn7 beorn7 self-assigned this Nov 5, 2018
@stuartnelson3
Copy link
Contributor

That was our concern. Some users could be putting user IDs or some other non-bounded set of data in the path, so we decided to avoid potentially destroying our users' prometheus servers.

@beorn7
Copy link
Member

beorn7 commented Nov 5, 2018

@stuartnelson3 and i discussed this over lunch. Most importantly, my memory served my right: Back then, we deliberately did not include this feature. We indeed had the concerns mentioned above.

In particular, the requested feature only makes sense with handlers that handle multiple paths. (If a handler only handles one path, you can curry the metric vector with a path label before using it with the instrumentation middleware.) But that's exactly the case where you run into the risk of unintentionally creating many different label values. For example, a malicious (or unintentionally broken) client could bombard your services with random URLs and thus create a new time series with each request. To protect against this, one might try to exclude 404s from the metric. But then we are getting fairly specific, which I would see out of scope of the instrumentation middlewares. Those middlewares are meant to be convenience function to handle standard situation in an easy way. If things get specific, it might just be best to instrument in the conventional way, as explained here. This way is not that complicated, so that I would be wary of adding to much power features to the convenience middlewares in promhttp.

Or in yet other words: The convenience middlewares in promhttp are provided so that it is easy to instrument an http.Handler in simple scenarios, even if you don't know a lot about instrumentation. Adding an automatic path label is however an easy way shoot yourself into the foot, especially for naive users. It would be useful for more involved users, but for those, it would also be easy to use the direct instrumentation as described in the linked talk.

Thus, I'm on the fence if that feature would be a net gain.

@zephinzer
Copy link
Author

Agree with the potential behaviour of having URLs - URL fuzzing by some common pentesting tools might cause this too. However, (correct me if i'm wrong, but) I believe this can be mitigated during the metrics extraction from Prometheus to a dashboard (Grafana in our case) via promql?

For example in our setup, we are monitoring the duration and status codes by endpoints which we've identified as critical for the business front-end. The exact endpoints tend to change based on business direction and it's useful for us to have a path label so that the monitoring and code is de-coupled.

But to summarise, I think you might be recommending that this be a third party instrumentation package instead of being in the core code? I noticed that such functionality is also not included in the core of the NodeJS but as a promclient (https://github.com/siimon/prom-client) package, so it could be in line with your fundamentals.

@beorn7
Copy link
Member

beorn7 commented Nov 6, 2018

Prometheus itself would suffer. The TSDB of Prometheus has a very low price per sample (usually between one and two bytes) but a much much higher price per serios (kilobytes). Thus, your server would explode.

I'm not suggesting to use a 3rd party package. All the tools you need are included in client_golang. You should just not use the convenience-middlewares but the usual instrumentation primitives as described in the linked video.

@zephinzer
Copy link
Author

cool, thanks for the advice on the potential effects on tsdb @beorn7! I gave it a watch and have managed to get up my own middleware to do something like what I was requesting for, am closing this-

but if anyone else is looking for such a convenience middleware despite the above warnings, I've published it at https://github.com/zephinzer/ezpromhttp!

pipecd-bot pushed a commit to pipe-cd/pipecd that referenced this issue Jul 16, 2021
**What this PR does / why we need it**:
This PR aims to expose two metrics about HTTP handler:
- `pipecd_http_requests_total`
- `pipecd_http_request_duration_milliseconds`

To do that, it mainly does these two:
- add an HTTP middleware to track metrics
- make a new package `httpapi` to put all HTTP handlers on the same package so that they can be wrapped by the middleware easily

Notes:
- As mentioned in the file, `pkg/app/api/httpapi/metricsmiddleware/delegator.go` is entirely borrowed from `prometheus/client_golang`.
- The most core part in this PR is `pkg/app/api/httpapi/httpapi.go`
- I've carefully checked the HTTP handlers work well as before and all metrics we expect got exposed.

(Supplement)
`client_golang/promhttp` doesn't add a label for HTTP path to prevent Prometheus's TSDB from high-cardinarity. See prometheus/client_golang#491 for more details.

**Which issue(s) this PR fixes**:

Fixes #

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
-->
```release-note
NONE
```

This PR was merged by Kapetanios.
@pavelpatrin
Copy link

I have a case, when to provide request path as a label is enough useful: gRPC Gateway server which is registered by RegisterXxxHandlerServer() call. It is impossible to produce metrics with gRPC middlewares, they are not called.

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

No branches or pull requests

4 participants