diff --git a/src/app.ts b/src/app.ts index a1f42a7..0f43de0 100644 --- a/src/app.ts +++ b/src/app.ts @@ -33,7 +33,7 @@ app.use( makeMetricsApiMiddleware({ port: 9082, isNormalizePathEnabled: true, - discardUnmatched: false, + discardUnmatched: true, }), ); diff --git a/src/middleware/metrics.ts b/src/middleware/metrics.ts index ac6180b..da1f82d 100644 --- a/src/middleware/metrics.ts +++ b/src/middleware/metrics.ts @@ -8,6 +8,14 @@ import UrlPattern from 'url-pattern'; import logger from '../utils/logging'; +const withoutTrailingSlash = (path: string) => { + // Express routes don't include a trailing slash unless it's actually just `/` path, then it stays + if (path !== '/' && path.endsWith('/')) { + return path.slice(0, -1); + } + return path; +}; + type Options = { port?: number, isNormalizePathEnabled?: boolean, @@ -32,69 +40,46 @@ const setupMetricService = ({ port }: Options = { port: 9082 }) => { }); }; -const captureAllRoutes = ({ urlPatternMaker }: Options, app: express.Express) => { - const allRoutes = listEndpoints(app).filter( - (route) => route.path !== '/*' && route.path !== '*', - ); - - return allRoutes.map((route) => { - const path = route.path.endsWith('/') ? route.path.replace(/\/$/, '') : route.path; - - logger.log('debug', `Route found: ${route.path} - ${JSON.stringify(route)}`); - // route.pattern = route.path; // TODO remove? - - // NOTE: urlPatternMaker has to create an UrlPattern compatible object. - const pattern = urlPatternMaker ? urlPatternMaker(route.path) : new UrlPattern(route.path, { - segmentNameCharset: 'a-zA-Z0-9_-', - }).toString(); - - if (!pattern) { - logger.log('debug', 'skipping route due to empty path'); - } - - return { ...route, path: pattern || path }; - }); +type Route = { + methods: string[], + path: string, + pattern: UrlPattern }; - const makeMetricsApiMiddleware = (options: Options = {}) => { const { isNormalizePathEnabled, discardUnmatched, createServer } = options; - const allRoutes = [] as { methods: string[], path: string, pattern?: UrlPattern }[]; + let allRoutes: Route[]; + // This function takes a path request and returns a path that should go into the `path` label of the metric + // The goal here is to not have an infinite number of paths so get rid of path variables and unknown paths const normalizePath: NormalizePathFn = (req, _opts) => { if (!isNormalizePathEnabled) { return req.url; } - const path = (() => { - const parsedPath = url.parse(req.originalUrl || req.url).pathname || req.url; - if (parsedPath.endsWith('/')) { - // Remove trailing slash - return parsedPath.replace(/\/$/, ''); - } - return parsedPath; - })(); - - const pattern = allRoutes.filter((route) => { - if (route.methods.indexOf(req.method) === -1) { + const parsedPath = url.parse(req.originalUrl || req.url).pathname || req.url; + const path = withoutTrailingSlash(parsedPath); + + const matchingRoute = allRoutes.find((route) => { + if (!route.methods.includes(req.method)) { return false; } + // match will be null if it doesn't fit the pattern and will be some object depending on path params if it does + // eg path /abc/123 will match route /abc/:id with object {id: 123} but will return null for route /xyz/:id try { - if (route.path.match(path)) { - return true; - } + const match: null | object = route.pattern.match(path); + return match !== null; } catch (e: unknown) { logger.error('Error: something went wrong.'); return false; } - return false; - })[0]?.pattern; + }); - if (discardUnmatched && !pattern) { - return ''; + if (discardUnmatched && !matchingRoute) { + return 'unmatched-url'; } - return pattern?.stringify() || path || ''; + return matchingRoute?.path ?? 'no-path'; }; const metricsMiddleware = promBundle({ @@ -113,11 +98,18 @@ const makeMetricsApiMiddleware = (options: Options = {}) => { } const handler: RequestHandler = (req, res, next) => { - if (allRoutes.length === 0) { + if (!allRoutes) { + // Scrape all the paths registered with express on the first recording of metrics. These paths will later be used + // to ensure we don't use unknown paths (ie spam calls) in metric labels and don't overwhelm Prometheus. + // Unfortunately, this doesn't include static paths since they are not registered with Express. If desired, + // we could add them by recursively listing /public. try { - captureAllRoutes(options, req.app as express.Express).forEach((route) => { - allRoutes.push(route); - }); + allRoutes = listEndpoints(req.app as express.Express) + .filter((route) => route.path !== '/*' && route.path !== '*') + .map((route) => ({ + ...route, + pattern: new UrlPattern(route.path), + })); } catch (e) { logger.log('error', `unable to capture route for prom-metrics: ${e}`); }