Feat/prometheus exporter #172
Conversation
…PI and API request metrics via a metrics endpoint. Also added an express middleware function to track count and duration of requests
| metrics: { | ||
| enabled: false, | ||
| prefix: 'starter', | ||
| collectDefaultMetrics: true |
There was a problem hiding this comment.
@gu - are there any performance / memory considerations for collecting metrics by default? What if a fork doesn't use them?
There was a problem hiding this comment.
@brianghig I haven't done any direct performance comparisons with default metrics disabled. If they are disabled, then the output of the metrics route would essentially be an empty object, which prometheus will be able to handle.
Hoping that the implementation for our team will give us a better idea on performance impact
| */ | ||
| const getMetrics = () => { | ||
| if (!metricsEnabled) { | ||
| return Promise.reject('Metrics not configured'); |
There was a problem hiding this comment.
@gu - should we throw an error here instead of rejecting the promise? It seems like we're moving toward async/await that would favor that style of error handling.
Also, should this read "Metrics not enabled" instead of not configured?
| return Promise.reject('Metrics not configured'); | ||
| } | ||
|
|
||
| return register.metrics(); |
There was a problem hiding this comment.
@gu - how should we handle situations where the start() method is not called prior to a getMetrics() request? Understanding that this is now in the middleware initialization, we may want to provide a more explicit error message.
| // Track start of request duration | ||
| const start = Date.now(); | ||
|
|
||
| res.on('finish', () => { |
There was a problem hiding this comment.
@gu - does the finish event handle failures? For example, if the request throws an error or times out?
There was a problem hiding this comment.
I believe it does but I'll verify that
|
It's always good to have more options available. I will say that the APM route was amazing in that the only required configuration for it was what I outlined in #32. The default configuration without any additional setup captured any request going into express in a very granular way, and metrics delivery was seamless. |
|
@tzfx Yeah I agree that APM was very convenient on another project that I used it on. I think over time I started to lean more towards the exporter/prometheus/grafana side since I think I personally prefer that pattern a bit more. I'm happy to update this to make this change less 'use-this-or-else' if you have any suggestions to make it more modular! I'm also fine with nixing this PR since I haven't been able to keep an eye on it :( |
|
Closing since it’s stale and I haven’t been able to keep it up to date |
Introduces a very basic set of functionality to expose a new API endpoint that returns application metrics for consumption by Prometheus.
The existing issue regarding performance logging has a few notes about Elastic APM usage, however I've personally had a better experience using the Prometheus/Grafana stack than ELK on a separate project. I'm more than happy to list a few pros/cons that I've noticed through using them both! Also I'm open to ideas on how to make this more modular so we aren't necessarily forcing/prescribing Prometheus usage.
By default will return Prometheus metrics for Node including garbage collection metrics, heap metrics, eventloop metrics, and more
When adding express middleware, we can also expose individual route stats. Currently this is limited to request count and a request duration histogram