-
Notifications
You must be signed in to change notification settings - Fork 44
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
Adding JSON metrics route option #52
base: master
Are you sure you want to change the base?
Conversation
Hi @Sunny-fr, Thanks for your PR! Can you please share your use-case? |
Yes of course.
So in my code it looks like this : const apiMetrics = require('prometheus-api-metrics')
const middleWare = function (app, router, config) {
app.use(
apiMetrics({
metricsPath: config.publicPath + '/metrics',
metricsJsonSuffix: '/json',
})
)
} it's not perfect, i think the better way is to provide a full path to the json route. if (this.setupOptions.metricsJsonRoute !== '' ? this.setupOptions.metricsJsonRoute : routeUrl === `${this.setupOptions.metricsRoute}.json`) {
debug('Request to /metrics endpoint');
return res.json(Prometheus.register.getMetricsAsJSON());
} But I tried to keep it simple and understandable :) |
i actually prefer your second alternative. IMO, it's less confusing and more flexible. |
"metricsJsonPath" defaults to ``${setupOptions.metricsRoute}.json`
Ok thanks ! setupOptions.metricsRoute = metricsPath || '/metrics';
setupOptions.metricsJsonRoute = metricsJsonPath || `${setupOptions.metricsRoute}.json`; |
Cool @Sunny-fr , we just need to mention it in the readme |
Ok, i'll make a proposition :) |
"metricsJsonPath" defaults to ``${setupOptions.metricsRoute}.json`
@kobik here it is, sorry for the delay. |
bump ! 👍 |
debug(`Init metrics middleware with options: ${JSON.stringify(options)}`); | ||
setupOptions.metricsRoute = metricsPath || '/metrics'; | ||
setupOptions.metricsJsonRoute = metricsJsonPath || `${setupOptions.metricsRoute}.json`; |
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.
thinking of that again, IMO we need to sanitize metricsJsonRoute
so it would always contain starting /
@@ -47,7 +47,7 @@ API and process monitoring with [Prometheus](https://prometheus.io) for Node.js | |||
- Process Metrics as recommended by Prometheus [itself](https://prometheus.io/docs/instrumenting/writing_clientlibs/#standard-and-runtime-collectors) | |||
- Endpoint to retrieve the metrics - used for Prometheus scraping | |||
- Prometheus format | |||
- JSON format (`${path}.json`) | |||
- JSON format (default to `${path}.json`, can be changed in options) |
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.
default is /${metricsPath}.json
, right?
@@ -61,6 +61,7 @@ app.use(apiMetrics()) | |||
### Options | |||
|
|||
- metricsPath - Path to access the metrics. `default: /metrics` | |||
- metricsJsonPath - Path to access the json formatted metrics. `default: {metricsPath}.json` |
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.
default is /${metricsPath}.json, right?
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.
see my comments @Sunny-fr
Hi @Sunny-fr, did you have the chance to see my comments? |
Hello There, Yes thank you, Im running out of time these days, but I'm willing to do it asap :) Thanks again ! |
Hello there,
The suffix for the metrics path on express is hard coded, it would be nice to have it "customizable" :
prometheus-api-metrics/src/express-middleware.js
Line 101 in 441cb48
Thank you