From 3b396c7e1965ef1e6be3acab5456ba1e097c9e06 Mon Sep 17 00:00:00 2001 From: Ed Clement Date: Thu, 18 Nov 2021 09:19:33 -0500 Subject: [PATCH] fix(metrics): Only collect metrics for download of tarball files. Metrics were previously collected for any GET request that looked like it was for a package.json or tarball install but this was less reliable as there were was no way to guarantee the request coming in was for an actual package.json (e.g. browser requests for favicon.ico). Also, Verdaccio would only hand off requests that generate a 401/403 to the middelware for non-tarball requests so metrics could be misleading. --- .eslintrc | 2 +- .npmignore | 7 +- README.md | 6 +- package-lock.json | 50 ++++++------ src/index.ts | 4 +- src/metrics.ts | 37 ++------- src/utils.ts | 4 +- tests/metrics.negative.spec.ts | 136 --------------------------------- tests/metrics.spec.ts | 55 ++++++++++--- tests/utils.spec.ts | 3 +- 10 files changed, 89 insertions(+), 215 deletions(-) delete mode 100644 tests/metrics.negative.spec.ts diff --git a/.eslintrc b/.eslintrc index fe440ed..1256c29 100644 --- a/.eslintrc +++ b/.eslintrc @@ -1,7 +1,7 @@ { "extends": ["@verdaccio"], "rules": { - "no-case-declarations": "off", + "no-case-declarations": 0, "@typescript-eslint/ban-ts-ignore": 0, "@typescript-eslint/ban-ts-comment": 0 } diff --git a/.npmignore b/.npmignore index 2e6be1f..c1a8665 100644 --- a/.npmignore +++ b/.npmignore @@ -1,5 +1,8 @@ src/ -test/ +tests/ +coverage/ +.circleci/ +.husky/ .babelrc .editorconfig @@ -9,5 +12,5 @@ test/ .npmignore metrics.ts jest.config.js -README.md +sonar-project.properties tsconfig.json diff --git a/README.md b/README.md index ef80e7a..399febc 100644 --- a/README.md +++ b/README.md @@ -1,11 +1,11 @@ # verdaccio-metrics-middleware -Metrics middleware plugin for Verdaccio. Collects metrics specifically for package install/download requests and +Metrics middleware plugin for Verdaccio. Collects metrics specifically for package tarball install/download requests and exposes them at a configurable metrics endpoint (defaults to `/-/metrics`). The metrics are produced in the standard [prometheus metrics text format](https://prometheus.io/docs/instrumenting/exposition_formats/#text-format-example). A [counter](https://prometheus.io/docs/concepts/metric_types/#counter) metric is used to track the number of package -installs/downloads. The following [labels](https://prometheus.io/docs/practices/naming/#labels) are applied to _every_ -request: +tarball installs/downloads. The following [labels](https://prometheus.io/docs/practices/naming/#labels) are applied to +_every_ request: - `username` - The Verdaccio username of the user attempting to install/download a package. If the request is unauthenticated then the value `UNKNOWN` is used. - `userAgentName` - The name of the user agent the client used to make the request. It is derived from the `user-agent` diff --git a/package-lock.json b/package-lock.json index 1217794..e31d626 100644 --- a/package-lock.json +++ b/package-lock.json @@ -7,7 +7,7 @@ "": { "name": "@xlts.dev/verdaccio-metrics-middleware", "version": "0.1.0", - "license": "MIT", + "license": "UNLICENSED", "dependencies": { "express": "4.17.1", "prom-client": "14.0.1" @@ -4342,9 +4342,9 @@ } }, "node_modules/acorn": { - "version": "8.5.0", - "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.5.0.tgz", - "integrity": "sha512-yXbYeFy+jUuYd3/CDcg2NkIYE991XYX/bje7LmjJigUciaeO1JR4XxXgCIV1/Zc/dRuFEyw1L0pbA+qynJkW5Q==", + "version": "8.6.0", + "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.6.0.tgz", + "integrity": "sha512-U1riIR+lBSNi3IbxtaHOIKdH8sLFv3NYfNv8sg7ZsNhcfl4HF2++BfqqrNAxoCLQW1iiylOj76ecnaUxz+z9yw==", "dev": true, "bin": { "acorn": "bin/acorn" @@ -5606,9 +5606,9 @@ "dev": true }, "node_modules/csstype": { - "version": "2.6.18", - "resolved": "https://registry.npmjs.org/csstype/-/csstype-2.6.18.tgz", - "integrity": "sha512-RSU6Hyeg14am3Ah4VZEmeX8H7kLwEEirXe6aU2IPfKNvhXwTflK5HQRDNI0ypQXoqmm+QPyG2IaPuQE5zMwSIQ==", + "version": "2.6.19", + "resolved": "https://registry.npmjs.org/csstype/-/csstype-2.6.19.tgz", + "integrity": "sha512-ZVxXaNy28/k3kJg0Fou5MiYpp88j7H9hLZp8PDC3jV0WFjfH5E9xHb56L0W59cPbKbcHXeP4qyT8PrHp8t6LcQ==", "dev": true }, "node_modules/data-urls": { @@ -5793,9 +5793,9 @@ "integrity": "sha1-WQxhFWsK4vTwJVcyoViyZrxWsh0=" }, "node_modules/electron-to-chromium": { - "version": "1.3.900", - "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.3.900.tgz", - "integrity": "sha512-SuXbQD8D4EjsaBaJJxySHbC+zq8JrFfxtb4GIr4E9n1BcROyMcRrJCYQNpJ9N+Wjf5mFp7Wp0OHykd14JNEzzQ==", + "version": "1.3.902", + "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.3.902.tgz", + "integrity": "sha512-zFv5jbtyIr+V9FuT9o439isXbkXQ27mJqZfLXpBKzXugWE8+3RotHbXJlli0/r+Rvdlkut0OOMzeOWLAjH0jCw==", "dev": true }, "node_modules/emittery": { @@ -14224,9 +14224,9 @@ } }, "node_modules/signal-exit": { - "version": "3.0.5", - "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.5.tgz", - "integrity": "sha512-KWcOiKeQj6ZyXx7zq4YxSMgHRlod4czeBQZrPb8OKcohcqAXShm7E20kEMle9WBt26hFcAf0qLOcp5zmY7kOqQ==", + "version": "3.0.6", + "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.6.tgz", + "integrity": "sha512-sDl4qMFpijcGw22U5w63KmD3cZJfBuFlVNbVMKje2keoKML7X2UzWbc4XrmEbDwg0NXJc3yv4/ox7b+JWb57kQ==", "dev": true }, "node_modules/sisteransi": { @@ -18762,9 +18762,9 @@ } }, "acorn": { - "version": "8.5.0", - "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.5.0.tgz", - "integrity": "sha512-yXbYeFy+jUuYd3/CDcg2NkIYE991XYX/bje7LmjJigUciaeO1JR4XxXgCIV1/Zc/dRuFEyw1L0pbA+qynJkW5Q==", + "version": "8.6.0", + "resolved": "https://registry.npmjs.org/acorn/-/acorn-8.6.0.tgz", + "integrity": "sha512-U1riIR+lBSNi3IbxtaHOIKdH8sLFv3NYfNv8sg7ZsNhcfl4HF2++BfqqrNAxoCLQW1iiylOj76ecnaUxz+z9yw==", "dev": true }, "acorn-globals": { @@ -19763,9 +19763,9 @@ } }, "csstype": { - "version": "2.6.18", - "resolved": "https://registry.npmjs.org/csstype/-/csstype-2.6.18.tgz", - "integrity": "sha512-RSU6Hyeg14am3Ah4VZEmeX8H7kLwEEirXe6aU2IPfKNvhXwTflK5HQRDNI0ypQXoqmm+QPyG2IaPuQE5zMwSIQ==", + "version": "2.6.19", + "resolved": "https://registry.npmjs.org/csstype/-/csstype-2.6.19.tgz", + "integrity": "sha512-ZVxXaNy28/k3kJg0Fou5MiYpp88j7H9hLZp8PDC3jV0WFjfH5E9xHb56L0W59cPbKbcHXeP4qyT8PrHp8t6LcQ==", "dev": true }, "data-urls": { @@ -19905,9 +19905,9 @@ "integrity": "sha1-WQxhFWsK4vTwJVcyoViyZrxWsh0=" }, "electron-to-chromium": { - "version": "1.3.900", - "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.3.900.tgz", - "integrity": "sha512-SuXbQD8D4EjsaBaJJxySHbC+zq8JrFfxtb4GIr4E9n1BcROyMcRrJCYQNpJ9N+Wjf5mFp7Wp0OHykd14JNEzzQ==", + "version": "1.3.902", + "resolved": "https://registry.npmjs.org/electron-to-chromium/-/electron-to-chromium-1.3.902.tgz", + "integrity": "sha512-zFv5jbtyIr+V9FuT9o439isXbkXQ27mJqZfLXpBKzXugWE8+3RotHbXJlli0/r+Rvdlkut0OOMzeOWLAjH0jCw==", "dev": true }, "emittery": { @@ -26339,9 +26339,9 @@ } }, "signal-exit": { - "version": "3.0.5", - "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.5.tgz", - "integrity": "sha512-KWcOiKeQj6ZyXx7zq4YxSMgHRlod4czeBQZrPb8OKcohcqAXShm7E20kEMle9WBt26hFcAf0qLOcp5zmY7kOqQ==", + "version": "3.0.6", + "resolved": "https://registry.npmjs.org/signal-exit/-/signal-exit-3.0.6.tgz", + "integrity": "sha512-sDl4qMFpijcGw22U5w63KmD3cZJfBuFlVNbVMKje2keoKML7X2UzWbc4XrmEbDwg0NXJc3yv4/ox7b+JWb57kQ==", "dev": true }, "sisteransi": { diff --git a/src/index.ts b/src/index.ts index 74971c7..7780adf 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1,4 +1,4 @@ -import MetricsPlugin, { API_PATH_PREFIX, REQUEST_COUNTER_OPTIONS } from './metrics'; +import MetricsPlugin, { REQUEST_COUNTER_OPTIONS } from './metrics'; export default MetricsPlugin; -export { API_PATH_PREFIX, REQUEST_COUNTER_OPTIONS }; +export { REQUEST_COUNTER_OPTIONS }; diff --git a/src/metrics.ts b/src/metrics.ts index 682441f..434a3c7 100644 --- a/src/metrics.ts +++ b/src/metrics.ts @@ -6,19 +6,18 @@ import { MetricsConfig, MetricsLabels } from '../types'; import { getUsername, getUserAgentData } from './utils'; -export const API_PATH_PREFIX = '/-/'; export const REQUEST_COUNTER_OPTIONS = { name: 'registry_requests', help: 'HTTP requests made to the registry', // Remember that every unique combination of key-value label pairs represents a new time series, which can // dramatically increase the amount of data stored. Refer to: https://prometheus.io/docs/practices/naming/#labels - labelNames: ['username', 'userAgentName', 'packageGroup', 'statusCode'], + labelNames: ['username', 'userAgentName', 'statusCode', 'packageGroup'], }; /** * A Verdaccio middleware plugin implementation. If enabled the following functionality is added: * 1. A single new metrics endpoint is exposed at a configurable path. - * 2. Metrics are collected related only to install/download of packages. + * 2. Metrics are collected related only to install/download of package tarballs. * Refer to: https://verdaccio.org/docs/plugin-middleware/ */ export default class VerdaccioMiddlewarePlugin implements IPluginMiddleware { @@ -36,7 +35,7 @@ export default class VerdaccioMiddlewarePlugin implements IPluginMiddleware} auth - The Verdaccio authentication service. * @param {IStorageManager} storage -The Verdaccio storage manager service. @@ -48,7 +47,7 @@ export default class VerdaccioMiddlewarePlugin implements IPluginMiddleware} - A promise that resolves to undefined since the function is async. */ public collectMetrics(req: Request, res: Response, next: NextFunction): void { - const { method, path } = req; - - switch (true) { - case !this.metricsEnabled: - return next(); - case method !== 'GET': - this.logger.debug( - { path, method }, - `metrics: [collectMetrics] request is not a 'GET' request: ${method} '${path}'` - ); - return next(); - case path === this.metricsPath: - case path.startsWith(API_PATH_PREFIX): - this.logger.debug({ path }, `metrics: [collectMetrics] request is for an excluded API path: '${path}'`); - return next(); - default: - this.logger.debug(`metrics: [collectMetrics] collecting metrics for request: ${method} '${path}'`); - } + const { path } = req; const authorization = req.header('authorization'); const userAgentString = req.header('user-agent'); @@ -103,11 +85,6 @@ export default class VerdaccioMiddlewarePlugin implements IPluginMiddleware new RegExp(regex).test(decodedPath)) || []; - this.logger.debug( - { authType, username, userAgentName, userAgentVersion, packageGroup }, - 'metrics: [collectMetrics] initial request metrics collected' - ); - // We won't know the final status code until the response is sent to the client. Because of this we don't collect // the metrics for this request until the response 'finish' event is emitted. res.once('finish', () => { @@ -117,8 +94,8 @@ export default class VerdaccioMiddlewarePlugin implements IPluginMiddleware { - describe('should register middleware (metrics disabled)', () => { - const logger = getLogger(); - const app = { use: jest.fn(), get: jest.fn() } as unknown as Application; - - beforeAll(() => { - register.clear(); - const metricsPlugin = new MetricsPlugin({ enabled: false } as MetricsConfig, { logger }); - metricsPlugin.register_middlewares(app, {} as IBasicAuth, {} as IStorageManager); - }); - - test('should not invoke any express API', () => { - expect(app.use).toHaveBeenCalledTimes(0); - expect(app.get).toHaveBeenCalledTimes(0); - }); - - test('should log warn that metrics are disabled', () => { - expect(logger.warn).toHaveBeenCalledWith('metrics: [register_middlewares] metrics are disabled'); - }); - }); - - describe('should not collect metrics when disabled', () => { - const logger = getLogger(); - const { req, res, next } = getExpressMocks(); - - beforeAll(() => { - register.clear(); - new MetricsPlugin({ enabled: false } as MetricsConfig, { logger }).collectMetrics(req, res, next); - }); - - test('should pass the request on to the next middleware function', () => { - expect(logger.debug).toHaveBeenCalledTimes(0); - expect(res.once).toHaveBeenCalledTimes(0); - expect(next).toHaveBeenCalledTimes(1); - }); - - test('should not collect any metrics', async () => { - expect(await register.getMetricsAsJSON()).toEqual(EMPTY_METRICS_JSON); - }); - }); - - describe('should not collect metrics for http methods other then GET', () => { - const logger = getLogger(); - const httpMethod = 'POST'; - const requestOptions = getRequestOptions({ httpMethod }); - const { req, res, next } = getExpressMocks(requestOptions); - - beforeAll(() => { - register.clear(); - new MetricsPlugin({ enabled: true } as MetricsConfig, { logger }).collectMetrics(req, res, next); - }); - - test('should pass the request on to the next middleware function', () => { - expect(logger.debug).toHaveBeenCalledWith( - { path: requestOptions.path, method: httpMethod }, - `metrics: [collectMetrics] request is not a 'GET' request: ${httpMethod} '${requestOptions.path}'` - ); - expect(res.once).toHaveBeenCalledTimes(0); - expect(next).toHaveBeenCalledTimes(1); - }); - - test('should not collect any metrics', async () => { - expect(await register.getMetricsAsJSON()).toEqual(EMPTY_METRICS_JSON); - }); - }); - - describe('should not collect metrics for the metrics path', () => { - const logger = getLogger(); - const metricsPath = `/${chance.word()}`; - const { req, res, next } = getExpressMocks(getRequestOptions({ path: metricsPath })); - - beforeAll(() => { - register.clear(); - new MetricsPlugin({ enabled: true, metricsPath } as MetricsConfig, { logger }).collectMetrics(req, res, next); - }); - - test('should pass the request on to the next middleware function', () => { - expect(logger.debug).toHaveBeenCalledWith( - { path: metricsPath }, - `metrics: [collectMetrics] request is for an excluded API path: '${metricsPath}'` - ); - expect(res.once).toHaveBeenCalledTimes(0); - expect(next).toHaveBeenCalledTimes(1); - }); - - test('should not collect any metrics', async () => { - expect(await register.getMetricsAsJSON()).toEqual(EMPTY_METRICS_JSON); - }); - }); - - describe(`should not collect metrics for restricted '${API_PATH_PREFIX}' API paths`, () => { - const logger = getLogger(); - const metricsPath = `${API_PATH_PREFIX}${chance.word()}`; - const { req, res, next } = getExpressMocks(getRequestOptions({ path: metricsPath })); - - beforeAll(() => { - register.clear(); - new MetricsPlugin({ enabled: true, metricsPath } as MetricsConfig, { logger }).collectMetrics(req, res, next); - }); - - test('should pass the request on to the next middleware function', () => { - expect(logger.debug).toHaveBeenCalledWith( - { path: metricsPath }, - `metrics: [collectMetrics] request is for an excluded API path: '${metricsPath}'` - ); - expect(res.once).toHaveBeenCalledTimes(0); - expect(next).toHaveBeenCalledTimes(1); - }); - - test('should not collect any metrics', async () => { - expect(await register.getMetricsAsJSON()).toEqual(EMPTY_METRICS_JSON); - }); - }); -}); diff --git a/tests/metrics.spec.ts b/tests/metrics.spec.ts index 88c6b30..66bae9f 100644 --- a/tests/metrics.spec.ts +++ b/tests/metrics.spec.ts @@ -1,7 +1,7 @@ import chanceJs from 'chance'; import { register } from 'prom-client'; import { Application } from 'express'; -import { IBasicAuth, IStorageManager } from '@verdaccio/types'; +import { IBasicAuth, IStorageManager, PluginOptions } from '@verdaccio/types'; import MetricsPlugin, { REQUEST_COUNTER_OPTIONS } from '../src'; import { MetricsConfig } from '../types'; @@ -24,17 +24,41 @@ const getMetricsJson = (values) => [ describe('Metrics Plugin', () => { describe('should register middleware (metrics enabled)', () => { const logger = getLogger(); - const app = { use: jest.fn(), get: jest.fn() } as unknown as Application; + const app = { get: jest.fn() } as unknown as Application; beforeAll(() => { register.clear(); - const metricsPlugin = new MetricsPlugin({ enabled: true } as MetricsConfig, { logger }); + const metricsPlugin = new MetricsPlugin( + { enabled: true } as MetricsConfig, + { logger } as PluginOptions + ); metricsPlugin.register_middlewares(app, {} as IBasicAuth, {} as IStorageManager); }); test('should invoke the correct express API calls', () => { - expect(app.use).toHaveBeenCalledTimes(1); - expect(app.get).toHaveBeenCalledTimes(1); + expect(app.get).toHaveBeenCalledTimes(2); + }); + }); + + describe('should register middleware (metrics disabled)', () => { + const logger = getLogger(); + const app = { get: jest.fn() } as unknown as Application; + + beforeAll(() => { + register.clear(); + const metricsPlugin = new MetricsPlugin( + { enabled: false } as MetricsConfig, + { logger } as PluginOptions + ); + metricsPlugin.register_middlewares(app, {} as IBasicAuth, {} as IStorageManager); + }); + + test('should not invoke any express API', () => { + expect(app.get).toHaveBeenCalledTimes(0); + }); + + test('should log warn that metrics are disabled', () => { + expect(logger.warn).toHaveBeenCalledWith('metrics: [register_middlewares] metrics are disabled'); }); }); @@ -43,7 +67,10 @@ describe('Metrics Plugin', () => { beforeAll(() => { register.clear(); - const metricsPlugin = new MetricsPlugin({ enabled: true } as MetricsConfig, { logger: getLogger() }); + const metricsPlugin = new MetricsPlugin( + { enabled: true } as MetricsConfig, + { logger: getLogger() } as PluginOptions + ); metricsPlugin.collectMetrics(req, res, next); metricsPlugin.getMetrics(req, res); }); @@ -82,13 +109,15 @@ describe('Metrics Plugin', () => { beforeAll(() => { register.clear(); - metricsPlugin = new MetricsPlugin({ enabled: true } as MetricsConfig, { logger: getLogger() }); + metricsPlugin = new MetricsPlugin( + { enabled: true } as MetricsConfig, + { logger: getLogger() } as PluginOptions + ); expressMocks.forEach(({ req, res, next }) => metricsPlugin.collectMetrics(req, res, next)); }); test('should invoke the correct express API calls', () => { - expressMocks.forEach(({ res, next }) => { - expect(res.once).toHaveBeenCalledTimes(1); + expressMocks.forEach(({ next }) => { expect(next).toHaveBeenCalledTimes(1); }); }); @@ -132,13 +161,15 @@ describe('Metrics Plugin', () => { beforeAll(() => { register.clear(); - metricsPlugin = new MetricsPlugin({ enabled: true, packageGroups } as MetricsConfig, { logger: getLogger() }); + metricsPlugin = new MetricsPlugin( + { enabled: true, packageGroups } as MetricsConfig, + { logger: getLogger() } as PluginOptions + ); expressMocks.forEach(({ req, res, next }) => metricsPlugin.collectMetrics(req, res, next)); }); test('should invoke the correct express API calls', () => { - expressMocks.forEach(({ res, next }) => { - expect(res.once).toHaveBeenCalledTimes(1); + expressMocks.forEach(({ next }) => { expect(next).toHaveBeenCalledTimes(1); }); }); diff --git a/tests/utils.spec.ts b/tests/utils.spec.ts index 40b081b..9fdc219 100644 --- a/tests/utils.spec.ts +++ b/tests/utils.spec.ts @@ -27,8 +27,7 @@ const userAgentNameTestCases = [ { userAgentName: 'curl', userAgentVersion: '7.68.0', userAgentString: 'curl/7.68.0' }, // Parsing of web browser user agent strings will almost always result in: // `{ userAgentName: 'Mozilla', userAgentVersion: '5.0' }` - // A library could be used to better determine the actual browser but has been deemed unnecessary since a customer - // should never being making requests for packages using a browser. + // A library could be used to better determine the actual browser but has not been deemed necessary. { userAgentName: 'Mozilla', userAgentVersion: '5.0',