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

Prometheus metrics like http_request_duration_milliseconds should use path variables #720

Closed
checketts opened this issue Feb 11, 2021 · 1 comment

Comments

@checketts
Copy link
Contributor

checketts commented Feb 11, 2021

Prometheus metrics currently aren't showing the path variables:

For example I have many metrics like:

http_request_duration_milliseconds{quantile="0.1",path="/api/admin/features/other-feature/toggle/on",method="POST",status="200"} 11.892953
http_request_duration_milliseconds{quantile="0.5",path="/api/admin/features/other-feature/toggle/on",method="POST",status="200"} 11.892953
http_request_duration_milliseconds{quantile="0.9",path="/api/admin/features/other-feature/toggle/on",method="POST",status="200"} 11.892953
http_request_duration_milliseconds{quantile="0.99",path="/api/admin/features/other-feature/toggle/on",method="POST",status="200"} 11.892953
http_request_duration_milliseconds_sum{path="/api/admin/features/other-feature/toggle/on",method="POST",status="200"} 11.892953
http_request_duration_milliseconds_count{path="/api/admin/features/other-feature/toggle/on",method="POST",status="200"} 1

Note the part of the path 'other-feature' should be :featureName so multiple toggles doesn't explode the number of metrics and so that metrics are aggreggated for the same endpoint.

Perhaps using the express-prometheus-middleware lib might be a good idea. (Or we could mimic their approach for path variables (https://github.com/joao-fontenele/express-prometheus-middleware/blob/master/src/index.js#L53)

@ivarconr
Copy link
Member

I agree that we should migrate towards path variables to avoid explosion in time-series.

I am not i favor of depending directly on express-prometheus-middleware for two reasons:

  1. we might need to support multiple metrics formats in the future
  2. express-prometheus-middleware depends directly on "prom-client" and given it being a global you soon end up in version problems (e.g.: fix: upgrade prom-client from 12.0.0 to 13.1.0 #709)

I think however we mimic they way express-prometheus-middleware resolves paths.

checketts added a commit to checketts/unleash that referenced this issue Feb 12, 2021
Tymek pushed a commit that referenced this issue Aug 26, 2022
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

2 participants