From 9364b8b67635b9a1fb1254678f4be9736bfd773e Mon Sep 17 00:00:00 2001 From: Nick Oliver Date: Tue, 28 Apr 2020 14:49:55 -0700 Subject: [PATCH 1/3] test(integration): record metric names and ensure descriptions --- .../__snapshots__/one-app.spec.js.snap | 35 +++++++++++++++++++ __tests__/integration/helpers/testRunner.js | 5 +-- __tests__/integration/one-app.spec.js | 30 +++++++++++++++- package-lock.json | 15 ++++++++ package.json | 1 + prod-sample/docker-compose.yml | 1 + 6 files changed, 84 insertions(+), 3 deletions(-) diff --git a/__tests__/integration/__snapshots__/one-app.spec.js.snap b/__tests__/integration/__snapshots__/one-app.spec.js.snap index 6eba3d91..9b7f340a 100644 --- a/__tests__/integration/__snapshots__/one-app.spec.js.snap +++ b/__tests__/integration/__snapshots__/one-app.spec.js.snap @@ -42,6 +42,41 @@ Object { } `; +exports[`Tests that require Docker setup one-app successfully started metrics has all metrics 1`] = ` +Array [ + "circuit", + "circuit_perf", + "nodejs_active_handles", + "nodejs_active_handles_total", + "nodejs_active_requests", + "nodejs_active_requests_total", + "nodejs_eventloop_lag_seconds", + "nodejs_external_memory_bytes", + "nodejs_gc_pause_seconds_total", + "nodejs_gc_reclaimed_bytes_total", + "nodejs_gc_runs_total", + "nodejs_heap_size_total_bytes", + "nodejs_heap_size_used_bytes", + "nodejs_heap_space_size_available_bytes", + "nodejs_heap_space_size_total_bytes", + "nodejs_heap_space_size_used_bytes", + "nodejs_version_info", + "oneapp_holocron_module_map_poll_consecutive_errors_total", + "oneapp_holocron_module_map_poll_total", + "oneapp_holocron_module_map_poll_wait_seconds", + "oneapp_version_info", + "process_cpu_seconds_total", + "process_cpu_system_seconds_total", + "process_cpu_user_seconds_total", + "process_heap_bytes", + "process_max_fds", + "process_open_fds", + "process_resident_memory_bytes", + "process_start_time_seconds", + "process_virtual_memory_bytes", +] +`; + exports[`Tests that require Docker setup one-app successfully started one-app server provides reporting routes client reported errors logs errors when reported to /_/report/errors 1`] = `"reported client error"`; exports[`Tests that require Docker setup one-app successfully started one-app server provides reporting routes csp-violations reported to server logs violations reported to /_/report/errors 1`] = `"CSP Violation: {\\\\n \\\\\\"csp-report\\\\\\": {\\\\n \\\\\\"document-uri\\\\\\": \\\\\\"bad.example.com"`; diff --git a/__tests__/integration/helpers/testRunner.js b/__tests__/integration/helpers/testRunner.js index 2b74e213..49fa8559 100644 --- a/__tests__/integration/helpers/testRunner.js +++ b/__tests__/integration/helpers/testRunner.js @@ -28,7 +28,7 @@ const deepMergeObjects = require('../../../src/server/utils/deepMergeObjects'); const prodSampleDir = path.resolve('./prod-sample/'); const pathToDockerComposeTestFile = path.resolve(prodSampleDir, 'docker-compose.test.yml'); -const setUpTestRunner = async ({ oneAppLocalPortToUse } = {}) => { +const setUpTestRunner = async ({ oneAppLocalPortToUse, oneAppMetricsLocalPortToUse } = {}) => { const pathToBaseDockerComposeFile = path.resolve(prodSampleDir, 'docker-compose.yml'); const seleniumServerPort = getRandomPortNumber(); // create docker compose file from base with changes needed for tests @@ -46,7 +46,8 @@ const setUpTestRunner = async ({ oneAppLocalPortToUse } = {}) => { 'one-app': { ports: [ `${oneAppLocalPortToUse}:8443`, - ], + oneAppMetricsLocalPortToUse ? `${oneAppMetricsLocalPortToUse}:3005` : undefined, + ].filter(Boolean), }, }, }, diff --git a/__tests__/integration/one-app.spec.js b/__tests__/integration/one-app.spec.js index 187e8037..c5ff6ec1 100644 --- a/__tests__/integration/one-app.spec.js +++ b/__tests__/integration/one-app.spec.js @@ -18,6 +18,7 @@ /* eslint-disable no-underscore-dangle */ import fetch from 'cross-fetch'; import yargs, { argv } from 'yargs'; +import parsePrometheusTextFormat from 'parse-prometheus-text-format'; import { setUpTestRunner, tearDownTestRunner } from './helpers/testRunner'; import { waitFor } from './helpers/wait'; @@ -49,8 +50,10 @@ describe('Tests that require Docker setup', () => { const defaultFetchOptions = createFetchOptions(); let originalModuleMap; const oneAppLocalPortToUse = getRandomPortNumber(); + const oneAppMetricsLocalPortToUse = getRandomPortNumber(); const appAtTestUrls = { fetchUrl: `https://localhost:${oneAppLocalPortToUse}`, + fetchMetricsUrl: `http://localhost:${oneAppMetricsLocalPortToUse}/metrics`, browserUrl: 'https://one-app:8443', }; @@ -59,7 +62,7 @@ describe('Tests that require Docker setup', () => { beforeAll(async () => { removeModuleFromModuleMap('late-frank'); originalModuleMap = readModuleMap(); - ({ browser } = await setUpTestRunner({ oneAppLocalPortToUse })); + ({ browser } = await setUpTestRunner({ oneAppLocalPortToUse, oneAppMetricsLocalPortToUse })); }); afterAll(async () => { @@ -85,6 +88,31 @@ describe('Tests that require Docker setup', () => { expect(rawHeaders).not.toHaveProperty('access-control-allow-credentials'); }); + describe('metrics', () => { + it('connects', async () => { + expect.assertions(1); + const response = await fetch(appAtTestUrls.fetchMetricsUrl); + expect(response).toHaveProperty('status', 200); + }); + + it('has all metrics', async () => { + expect.assertions(1); + const response = await fetch(appAtTestUrls.fetchMetricsUrl); + const parsedMetrics = parsePrometheusTextFormat(await response.text()); + expect(parsedMetrics.map((metric) => metric.name).sort()).toMatchSnapshot(); + }); + + it('has help information on each metric', async () => { + expect.assertions(1); + const response = await fetch(appAtTestUrls.fetchMetricsUrl); + const parsedMetrics = parsePrometheusTextFormat(await response.text()); + const errors = parsedMetrics + .filter((metric) => (!metric.help) || metric.help.length === 0) + .map((metric) => metric.name); + expect(errors).toEqual([]); + }); + }); + test('app rejects CORS OPTIONS pre-flight requests for POST', async () => { const response = await fetch( `${appAtTestUrls.fetchUrl}/success`, diff --git a/package-lock.json b/package-lock.json index 742f441b..053ac294 100644 --- a/package-lock.json +++ b/package-lock.json @@ -15759,6 +15759,15 @@ "integrity": "sha1-bVuTSkVpk7I9N/QKOC1vFmao5cY=", "dev": true }, + "parse-prometheus-text-format": { + "version": "1.1.1", + "resolved": "https://registry.npmjs.org/parse-prometheus-text-format/-/parse-prometheus-text-format-1.1.1.tgz", + "integrity": "sha512-dBlhYVACjRdSqLMFe4/Q1l/Gd3UmXm8ruvsTi7J6ul3ih45AkzkVpI5XHV4aZ37juGZW5+3dGU5lwk+QLM9XJA==", + "dev": true, + "requires": { + "shallow-equal": "^1.2.0" + } + }, "parse5": { "version": "3.0.3", "resolved": "https://registry.npmjs.org/parse5/-/parse5-3.0.3.tgz", @@ -17718,6 +17727,12 @@ "kind-of": "^6.0.2" } }, + "shallow-equal": { + "version": "1.2.1", + "resolved": "https://registry.npmjs.org/shallow-equal/-/shallow-equal-1.2.1.tgz", + "integrity": "sha512-S4vJDjHHMBaiZuT9NPb616CSmLf618jawtv3sufLl6ivK8WocjAo58cXwbRV1cgqxH0Qbv+iUt6m05eqEa2IRA==", + "dev": true + }, "shallowequal": { "version": "1.1.0", "resolved": "https://registry.npmjs.org/shallowequal/-/shallowequal-1.1.0.tgz", diff --git a/package.json b/package.json index 81d881f6..60687497 100644 --- a/package.json +++ b/package.json @@ -161,6 +161,7 @@ "node-mocks-http": "1.8.0", "nodemon": "^1.19.4", "ora": "^4.0.3", + "parse-prometheus-text-format": "^1.1.1", "react-test-renderer": "^16.13.1", "rimraf": "^3.0.0", "rollup": "^2.2.0", diff --git a/prod-sample/docker-compose.yml b/prod-sample/docker-compose.yml index a654f063..844384ec 100644 --- a/prod-sample/docker-compose.yml +++ b/prod-sample/docker-compose.yml @@ -14,6 +14,7 @@ services: image: one-app:at-test expose: - "8443" + - "3005" volumes: - ./one-app/one-app-cert.pem:/opt/cert.pem - ./one-app/one-app-privkey.pem:/opt/key.pem From d1332598feac5477b12142aaf3542996c1332327 Mon Sep 17 00:00:00 2001 From: Nick Oliver Date: Tue, 28 Apr 2020 15:08:08 -0700 Subject: [PATCH 2/3] feat(metrics): update prom-client for more dimensions also removes the need for gc-stats, using the Node.JS API instead --- .../__snapshots__/one-app.spec.js.snap | 11 +++++-- .../__snapshots__/metricsServer.spec.js.snap | 7 ----- __tests__/server/metricsServer.spec.js | 31 +------------------ package-lock.json | 6 ++-- package.json | 4 +-- src/server/metricsServer.js | 15 +-------- 6 files changed, 14 insertions(+), 60 deletions(-) delete mode 100644 __tests__/server/__snapshots__/metricsServer.spec.js.snap diff --git a/__tests__/integration/__snapshots__/one-app.spec.js.snap b/__tests__/integration/__snapshots__/one-app.spec.js.snap index 9b7f340a..822bfe4a 100644 --- a/__tests__/integration/__snapshots__/one-app.spec.js.snap +++ b/__tests__/integration/__snapshots__/one-app.spec.js.snap @@ -50,11 +50,16 @@ Array [ "nodejs_active_handles_total", "nodejs_active_requests", "nodejs_active_requests_total", + "nodejs_eventloop_lag_max_seconds", + "nodejs_eventloop_lag_mean_seconds", + "nodejs_eventloop_lag_min_seconds", + "nodejs_eventloop_lag_p50_seconds", + "nodejs_eventloop_lag_p90_seconds", + "nodejs_eventloop_lag_p99_seconds", "nodejs_eventloop_lag_seconds", + "nodejs_eventloop_lag_stddev_seconds", "nodejs_external_memory_bytes", - "nodejs_gc_pause_seconds_total", - "nodejs_gc_reclaimed_bytes_total", - "nodejs_gc_runs_total", + "nodejs_gc_duration_seconds", "nodejs_heap_size_total_bytes", "nodejs_heap_size_used_bytes", "nodejs_heap_space_size_available_bytes", diff --git a/__tests__/server/__snapshots__/metricsServer.spec.js.snap b/__tests__/server/__snapshots__/metricsServer.spec.js.snap deleted file mode 100644 index 9be93587..00000000 --- a/__tests__/server/__snapshots__/metricsServer.spec.js.snap +++ /dev/null @@ -1,7 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`metricsServer metrics setup warns if unable to collect garbage collection metrics 1`] = ` -Array [ - "Unable to set up garbage collection monitor. This is not an issue for local development.", -] -`; diff --git a/__tests__/server/metricsServer.spec.js b/__tests__/server/metricsServer.spec.js index 2ea59850..2bc6ffa0 100644 --- a/__tests__/server/metricsServer.spec.js +++ b/__tests__/server/metricsServer.spec.js @@ -21,27 +21,17 @@ describe('metricsServer', () => { jest.spyOn(console, 'warn').mockImplementation(() => {}); let client; - let promGcStats; let healthCheck; let logging; - function load({ gcStatsError = false } = {}) { + function load() { jest.resetModules(); jest.mock('prom-client'); - if (gcStatsError) { - jest.mock('gc-stats', () => { - throw new Error('unable to resolve gc-stats'); - }, { virtual: true }); - } else { - jest.mock('gc-stats', () => jest.fn(), { virtual: true }); - } - jest.mock('prometheus-gc-stats', () => jest.fn(() => () => {})); jest.mock('../../src/server/utils/logging/serverMiddleware', () => jest.fn((req, res, next) => next())); jest.mock('../../src/server/middleware/healthCheck'); client = require('prom-client'); - promGcStats = require('prometheus-gc-stats'); logging = require('../../src/server/utils/logging/serverMiddleware'); healthCheck = require('../../src/server/middleware/healthCheck').default; @@ -53,25 +43,6 @@ describe('metricsServer', () => { load(); expect(client.collectDefaultMetrics).toHaveBeenCalledTimes(1); }); - - it('collects default metrics every ten seconds', () => { - load(); - expect(client.collectDefaultMetrics).toHaveBeenCalledTimes(1); - expect(client.collectDefaultMetrics.mock.calls[0][0]).toHaveProperty('timeout', 10 * 1e3); - }); - - it('collects garbage collection metrics', () => { - load(); - expect(promGcStats).toHaveBeenCalledTimes(1); - expect(promGcStats).toHaveBeenCalledWith(client.register); - }); - - it('warns if unable to collect garbage collection metrics', () => { - console.warn.mockClear(); - load({ gcStatsError: true }); - expect(console.warn).toHaveBeenCalledTimes(1); - expect(console.warn.mock.calls[0]).toMatchSnapshot(); - }); }); describe('unknown routes', () => { diff --git a/package-lock.json b/package-lock.json index 053ac294..ab821c63 100644 --- a/package-lock.json +++ b/package-lock.json @@ -16354,9 +16354,9 @@ "dev": true }, "prom-client": { - "version": "11.5.3", - "resolved": "https://registry.npmjs.org/prom-client/-/prom-client-11.5.3.tgz", - "integrity": "sha512-iz22FmTbtkyL2vt0MdDFY+kWof+S9UB/NACxSn2aJcewtw+EERsen0urSkZ2WrHseNdydsvcxCTAnPcSMZZv4Q==", + "version": "12.0.0", + "resolved": "https://registry.npmjs.org/prom-client/-/prom-client-12.0.0.tgz", + "integrity": "sha512-JbzzHnw0VDwCvoqf8y1WDtq4wSBAbthMB1pcVI/0lzdqHGJI3KBJDXle70XK+c7Iv93Gihqo0a5LlOn+g8+DrQ==", "requires": { "tdigest": "^0.1.1" } diff --git a/package.json b/package.json index 60687497..0770eb1c 100644 --- a/package.json +++ b/package.json @@ -101,8 +101,7 @@ "opossum": "^5.0.0", "opossum-prometheus": "^0.1.0", "pidusage": "^2.0.17", - "prom-client": "^11.5.3", - "prometheus-gc-stats": "^0.6.2", + "prom-client": "^12.0.0", "prop-types": "^15.7.2", "react": "^16.13.1", "react-dom": "^16.13.1", @@ -181,7 +180,6 @@ } }, "optionalDependencies": { - "gc-stats": "^1.4.0", "heapdump": "^0.3.15" }, "standard-version": { diff --git a/src/server/metricsServer.js b/src/server/metricsServer.js index 418e52be..e6300c53 100644 --- a/src/server/metricsServer.js +++ b/src/server/metricsServer.js @@ -17,24 +17,11 @@ import express from 'express'; import helmet from 'helmet'; import { register as metricsRegister, collectDefaultMetrics } from 'prom-client'; -import gcStats from 'prometheus-gc-stats'; import logging from './utils/logging/serverMiddleware'; import healthCheck from './middleware/healthCheck'; -// Probe every 10th second. -collectDefaultMetrics({ timeout: 10e3 }); -gcStats(metricsRegister)(); - -// prometheus-gc-stats uses gc-stats but swallows the error if not importable -// try importing ourselves so we can log a warning -try { - /* eslint import/no-extraneous-dependencies: ["error", {"optionalDependencies": true}] */ - // eslint-disable-next-line global-require, import/no-unresolved - require('gc-stats'); -} catch (err) { - console.warn('Unable to set up garbage collection monitor. This is not an issue for local development.'); -} +collectDefaultMetrics(); export function createMetricsServer() { const app = express(); From c6d49a1f273f944c75772e0f6bc41d71ed7f513e Mon Sep 17 00:00:00 2001 From: Nick Oliver Date: Wed, 13 May 2020 16:01:40 -0700 Subject: [PATCH 3/3] test(integration): use a positive scenario for metric help --- __tests__/integration/one-app.spec.js | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/__tests__/integration/one-app.spec.js b/__tests__/integration/one-app.spec.js index c5ff6ec1..47f18fd4 100644 --- a/__tests__/integration/one-app.spec.js +++ b/__tests__/integration/one-app.spec.js @@ -106,10 +106,14 @@ describe('Tests that require Docker setup', () => { expect.assertions(1); const response = await fetch(appAtTestUrls.fetchMetricsUrl); const parsedMetrics = parsePrometheusTextFormat(await response.text()); - const errors = parsedMetrics - .filter((metric) => (!metric.help) || metric.help.length === 0) + const allMetricNames = parsedMetrics .map((metric) => metric.name); - expect(errors).toEqual([]); + + const metricsNamesWithHelpInfo = parsedMetrics + .filter((metric) => metric.help && metric.help.length > 0) + .map((metric) => metric.name); + + expect(metricsNamesWithHelpInfo).toEqual(allMetricNames); }); });