jochen-schweizer / express-prom-bundle

express middleware with standard prometheus metrics in one bundle
MIT License
310 stars 68 forks source link

Missing `promClient` type in the `express_prom_bundle` interface #98

Closed hjpotter92 closed 2 years ago

hjpotter92 commented 2 years ago

The type definition for express_prom_bundle is as follows:

interface express_prom_bundle {
  normalizePath: express_prom_bundle.NormalizePathFn;
  normalizeStatusCode: express_prom_bundle.NormalizeStatusCodeFn;
}

whereas, in the module, a promClient instance is also exposed:

// this is kept only for compatibility with the code relying on older version
main.promClient = promClient;

main.normalizePath = normalizePath;
main.normalizeStatusCode = normalizeStatusCode;
main.clusterMetrics = clusterMetrics;
module.exports = main;

The same is true for the Middleware interface. I am looking to inject custom metrics alongside the bundle, so that a single metric middleware would provide prometheus server with the metrics. If there is another way to achieve the same, please provide some example for the same.

Thanks

disjunction commented 2 years ago

just use that exposed promClient and add your metrics. The middleware will output all of them regardless if they come from bundle or from your custom code. Or do you have a specific problem, where this approach doesn't work?

hjpotter92 commented 2 years ago

just use that exposed promClient and add your metrics. The middleware will output all of them regardless if they come from bundle or from your custom code. Or do you have a specific problem, where this approach doesn't work?

@disjunction sorry if my point was not clear. I only meant that the lsp server reports an error when using the bundle in typescript:

import bundle from "express-prom-bundle";
const client = bundle.promClient;
const metricsMiddleware = bundle({});
const { promClient } = metricsMiddleware;

reports the following error:

image

disjunction commented 2 years ago

the prom-client is a peer dependency, so express-prom-bundle doesn't know exactly the type definition. Also i have no experience with lsp, but just as a workaround, did you try:

const promClient = (metricsMiddleware as any).promClient;

let me know if the workaround works, and I'll need to think of a proper way to solve it. I guess now with TS used everywhere the library does need a clear way how to do that.

disjunction commented 2 years ago

... actually we can add it to interface Middleware as dumb any in the index.d.ts. Feel free to make a PR if that works for you.

disjunction commented 2 years ago

sorry for spamming :) on the third thought... because prom-client is a peer dependency, you don't need to use the re-exported instance of it, instead do the usual:

import * as client from 'prom-client'

It is re-exported in the middleware as a backwards compatibility feature from the times when it was an actual dependency.

So it's not a bug, but a feature. And the readme doesn't mention you even can do this stuff. I guess i removed it exactly because it is there for the older code only.

hjpotter92 commented 2 years ago

sorry for spamming :) on the third thought... because prom-client is a peer dependency, you don't need to use the re-exported instance of it, instead do the usual:

import * as client from 'prom-client'

It is re-exported in the middleware as a backwards compatibility feature from the times when it was an actual dependency.

So it's not a bug, but a feature. And the readme doesn't mention you even can do this stuff. I guess i removed it exactly because it is there for the older code only.

ah, that does help. perhaps add a small section to readme about publishing custom metrics, as it is not readily apparent that the bundle aggregates those results as well?

Thanks for help, and for the library :D