-
-
Notifications
You must be signed in to change notification settings - Fork 219
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
feat(#8426): API express internals + endpoint monitoring via Prometheus #8354
Conversation
api/src/routing.js
Outdated
@@ -111,6 +112,16 @@ app.postJsonOrCsv = (path, callback) => handleJsonOrCsvRequest('post', path, cal | |||
app.postJson = (path, callback) => handleJsonRequest('post', path, callback); | |||
app.putJson = (path, callback) => handleJsonRequest('put', path, callback); | |||
|
|||
app.use(prometheusMiddleware({ | |||
metricsPath: '/api_prometheus_metrics', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure where to put this. somewhere under /api/
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We've usually followed this format /api/v1/<thing>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose this won't export any PHI. If it's possible to export any URL, I think we're at risk of exposing PHI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have moved to /api/vi/prometheus_metrics
.
Can you elaborate on the risk of exposing PHI? I want to make sure I'm clear about your concerns. The intended beahvior here is to only export the Express routes, not URLs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, you're correct, there no immediate concern for PHI being exposed through this API at this moment. However, we should still be vigilant whenever we update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
One nitpick inline.
Has someone actually hooked this up to watchdog yet?
Yep over here: It will be broken by the recent metricPrefix change, but I'll fix that shortly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Smooth
Tested with local CHT, grafana, prometheus. API-side looks good. |
So awesome to see this merged! Nice work all. |
Description
Adds new endpoint
/api_prometheus_metrics
exposing details of what's happening within CHT API Express Server via Prometheus.Conversation in medic/cht-watchdog#60, but I'm installing from a private branch here to work around PayU/prometheus-api-metrics#112 and PayU/prometheus-api-metrics#114.
This exposes:
Works with these dashboards
medic/cht-watchdog#60
#8426
Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.