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

feat(#8426): API express internals + endpoint monitoring via Prometheus #8354

Merged
merged 16 commits into from Aug 3, 2023

Conversation

kennsippell
Copy link
Member

@kennsippell kennsippell commented Jun 30, 2023

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:

  • Prom-client default metrics for Node.js
  • Prom-client default metrics for Node.js Garbage collection
  • prometheus-api-metrics metrics for Express endpoints

Works with these dashboards

medic/cht-watchdog#60
#8426

Code review checklist

  • Readable: Concise, well named, follows the style guide, documented if necessary.
  • Documented: Configuration and user documentation on cht-docs
  • Tested: Unit and/or e2e where appropriate
  • Internationalised: All user facing text
  • Backwards compatible: Works with existing data and configuration or includes a migration. Any breaking changes documented in the release notes.

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.

@kennsippell kennsippell changed the title feat(cht-watchdog#60) - Endpoint monitoring via Prometheus feat(cht-watchdog#60) - API express internals + endpoint monitoring via Prometheus Jun 30, 2023
@@ -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',
Copy link
Member Author

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/?

Copy link
Member

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>

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

api/src/routing.js Outdated Show resolved Hide resolved
@garethbowen garethbowen changed the title feat(cht-watchdog#60) - API express internals + endpoint monitoring via Prometheus feat(#8426): API express internals + endpoint monitoring via Prometheus Aug 1, 2023
@kennsippell kennsippell marked this pull request as ready for review August 2, 2023 03:10
api/src/routing.js Outdated Show resolved Hide resolved
Copy link
Member

@garethbowen garethbowen left a 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?

@kennsippell
Copy link
Member Author

kennsippell commented Aug 2, 2023

Has someone actually hooked this up to watchdog yet?

Yep over here:
medic/cht-watchdog#74
medic/cht-watchdog#75

It will be broken by the recent metricPrefix change, but I'll fix that shortly.

Copy link
Member

@dianabarsan dianabarsan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Smooth

@kennsippell
Copy link
Member Author

Tested with local CHT, grafana, prometheus. API-side looks good.

@kennsippell kennsippell closed this Aug 2, 2023
@kennsippell kennsippell reopened this Aug 2, 2023
@kennsippell kennsippell merged commit 2f894cd into master Aug 3, 2023
44 checks passed
@kennsippell kennsippell deleted the dog60-endpoint-monitoring branch August 3, 2023 05:04
@mrjones-plip
Copy link
Contributor

So awesome to see this merged! Nice work all.

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

Successfully merging this pull request may close these issues.

None yet

5 participants