feat(prometheus): add getMetricsRequestHandler-method to Prometheus
#1879
Conversation
… exporter This commit introduces a new method on the `PrometheusExporter`-class that allows users of server frameworks with an existing service instance to call the `getMetricsRequestHandler`-method as part of a route to return the collected metrics in Prometheus format.
Codecov Report
@@ Coverage Diff @@
## main #1879 +/- ##
==========================================
- Coverage 92.74% 92.73% -0.02%
==========================================
Files 173 173
Lines 5971 5973 +2
Branches 1269 1269
==========================================
+ Hits 5538 5539 +1
- Misses 433 434 +1
|
|
@weyert You need to sign the CLA (you can follow the bot message to approve it) and also the lint CI fails because of an unrelated problem so you can ignore it |
|
I have updated the tests so it uses sinon to do the mocking for my unit test. |
Yes, I am going through the easycla process :) |
weyert
requested review from
dyladan,
Flarna,
johnbley,
legendecas,
markwolff,
mayurkale22,
mwear,
naseemkullah,
obecny,
OlivierAlbertini and
vmarchaud
as code owners
|
@vmarchaud I have promoted the pull request. It's not a draft anymore |
| * @param request Incoming HTTP request of server instance | ||
| * @param response HTTP response objet used to response to request | ||
| */ | ||
| public getMetricsRequestHandler( |
There was a problem hiding this comment.
This function names implies you are getting a handler, which is not the what is actually happening. I think it would be smarter to return a middleware that is compatible with express and other web frameworks:
public getMetricsRequestHandler() {
const self = this;
return function metricsHandler(req, res, next) {
// req validation?
// suppress instrumenting this method? optionally?
// error handling?
self.exportMetrics(res);
};
}There was a problem hiding this comment.
The idea I was having to return a raw handler which has the IncomingMessage, and ServerResponse arguments as we can't really be sure what the user's framework is. I was thinking the user could do something like this for Express:
app.get('/metrics', (req, res) => {
return exporter.getMetricsRequestHandler(req, res)
})
while for Hapi you could do something like:
function HapiWrapper(exporter) {
const handler = exporter.getMetricsRequestHandler()
return async ( { raw }, h) {
await handler(raw.req, raw.res)
return h.close
}
}
while for Koa something like:
const router = app.router()
router.get('/metrics', async (ctx) => {
await handle(ctx.req, ctx.res)
ctx.respond = false
})
Probably would need to allow the handler to be async but the above the idea to offer most flexibility. Maybe the name needs to better reflect this?
There was a problem hiding this comment.
But you have a point maybe it should be returned with .bind(this)
There was a problem hiding this comment.
I agree with @weyert here, its better to use a simpler model that doesn't necessarely match express api so it can be used with every framework (at the cost of having a different usage)
There was a problem hiding this comment.
Thank you @vmarchaud :)
|
Does anyone understand why it complains about |
Because you have not regenerated the package-lock.json files. If you delete the file then bootstrap again it will be fixed. |
Yes, I did regenerate it but the lock file is being ignored in |
|
Oh it changed recently :) |
Hmm, yeah, I had issues with it earlier. I will have a look at it tomorrow :) |
|
@obecny I have made your requested change. I did goof up a bit while doing it (deleted wrong file) but should be sorted now |
|
@weyert Thanks for the contribution ! |
… exporter
This commit introduces a new method on the
PrometheusExporter-class thatallows users of server frameworks with an existing service instance to
call the
getMetricsRequestHandler-method as part of a route to returnthe collected metrics in Prometheus format.
Fixes #1872
Which problem is this PR solving?
Short description of the changes
getMetricsRequestHandler-method to thePrometheusExporter-class which calls the internal_exportMetrics-method