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 Request: metrics or instrumentation #1526

Closed
sevagh opened this issue May 16, 2020 · 15 comments · Fixed by #3420
Closed

Feature Request: metrics or instrumentation #1526

sevagh opened this issue May 16, 2020 · 15 comments · Fixed by #3420
Labels
enhancement a feature, ready for implementation

Comments

@sevagh
Copy link

sevagh commented May 16, 2020

It would be nice to have a /metrics endpoint that exposes Prometheus-style metrics.

Note that I might be opinionated for working mostly with the Prometheus ecosystem - perhaps a more general metrics library, with different exposition formats to choose from, could work? I also don't know which existing Haskell metrics libraries or Prometheus libraries exist or are good (but hackage shows there might be some).

I'm not sure what metrics are the best to expose. I bet PostgREST developers know more about essential PostgREST KPIs. But things like:

postgrest_query_execution_time (histogram)
postgrest_http_requests
postgrest_schema_reloads
...
@steve-chavez
Copy link
Member

Needs more discussion, but It could be a good idea!

One thing I'd like to note for now is that we can't use a metrics endpoint because it would conflict with users routes(one example of using a metrics table here). So maybe we can come up with a prefix, like /pgrst/metrics or /internal/metrics.

@sevagh
Copy link
Author

sevagh commented Jul 10, 2020

That sounds good. Given PostgREST + NGINX is the recommended/common deployment, one could easily expose the internal metrics prefix at their desired location.

@steve-chavez
Copy link
Member

steve-chavez commented Sep 3, 2021

On #1933, we were thinking of using a special header for this and avoid creating an extra route.

Seems Prometheus doesn't support adding headers for scraping though 😞 prometheus/prometheus#1724

PostgREST + NGINX is the recommended/common deployment

But since Nginx would be present, then I guess it's not a problem because it can map the header to a url. Another option in the future might be #1909 as well. So a special header would do.

Edit: ref https://github.com/qnikst/prometheus-haskell

@darora
Copy link

darora commented Sep 22, 2021

Hmm at least for liveness/health checks, we'd want to be hitting the postgrest instances directly, rather than going through nginx or a similar rewriting layer.

Even with an nginx setup, I'd imagine we generally have it load balancing between multiple postgrest instances transparently, so even for metrics you'd likely want to be hitting each instance directly, rather than going over nginx.

One alternative would be to use a secondary port to host endpoints for liveness/metrics etc. This would avoid creating a breaking change where we reserve e.g. /internal or a similar base path, and trivially allow users to not expose these endpoints externally.

@rupurt
Copy link

rupurt commented May 5, 2022

Can haskell run multiple web servers on different ports? That's how it's typically done in other languages so that you don't have route path conflicts + you typically don't expose the prometheus webserver port to the outside world.

@steve-chavez
Copy link
Member

@rupurt Yeah, we already have that on latest https://postgrest.org/en/latest/configuration.html#admin-server-port

@rupurt
Copy link

rupurt commented May 5, 2022

@steve-chavez awesome. Would be great to have prometheus metrics in there 😄

@uhbif19
Copy link

uhbif19 commented Aug 9, 2022

Is it okay to use https://hackage.haskell.org/package/prometheus for this task?

@steve-chavez
Copy link
Member

@uhbif19 Yes, that one should do.

For posterity, #2129 was closed but the pool metrics discussed there would still be useful.

@steve-chavez steve-chavez added the enhancement a feature, ready for implementation label Sep 16, 2022
@steve-chavez
Copy link
Member

steve-chavez commented Sep 16, 2022

From #2477

It would be great if we can get
1.. GC
2. Query response times
3. Requests queued, DB connection pool usage count etc
metrics on the Admin Server port at maybe /met

For 2, I was actually referring to time taken for a request for round trip from postgrest to DB.
It's helpful in scenarios when we are observing high latencies from postgrest but DB takes only few milli seconds. Such issues could be due to high load on postgrest pods, cpu throttling, connection pool crunch etc (in k8s world).

@bhupixb
Copy link

bhupixb commented Mar 27, 2023

Has this feature been released in any 10.x version?

@steve-chavez
Copy link
Member

Nope, not yet implemented.

(Issue would be closed if so)

@steve-chavez
Copy link
Member

steve-chavez commented Jun 8, 2023

These days seems we should be using OpenTelemetry instead of Prometheus. Maybe with:

Article about the differences: https://www.timescale.com/blog/prometheus-vs-opentelemetry-metrics-a-complete-guide/

@steve-chavez
Copy link
Member

steve-chavez commented Dec 13, 2023

A metric for the connection pool max acquisition time would be helpful to prevent acquisition timeouts. While it gets higher it will reach the timeout.

Also OpenTelemetry Traces seem to correspond to Server Timing?

@develop7
Copy link
Collaborator

@steve-chavez yep, they seem to be a perfect match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement a feature, ready for implementation
Development

Successfully merging a pull request may close this issue.

7 participants