From 16f21cec12a0bed2a0a40da9db61423329c4e7b1 Mon Sep 17 00:00:00 2001
From: Gabriele Modena <gmodena@wikimedia.org>
Date: Mon, 4 Mar 2024 13:03:07 +0100
Subject: [PATCH 1/6] promethus: collect default metrics.

Add a config option to collect default metrics
(including GC). This should be enable only when
running with no workers.
---
 lib/metrics/prometheus.js                | 5 +++++
 test/utils/simple_config_no_workers.yaml | 1 +
 2 files changed, 6 insertions(+)

diff --git a/lib/metrics/prometheus.js b/lib/metrics/prometheus.js
index e6ede33..920c164 100644
--- a/lib/metrics/prometheus.js
+++ b/lib/metrics/prometheus.js
@@ -8,6 +8,11 @@ class PrometheusMetric {
 	constructor( options, client ) {
 		this.client = client;
 		this.options = cloneDeep( options );
+
+		// TODO: update eslint rules to allow optional chaining operator `?`
+		if ( this.options.prometheus && this.options.prometheus.collect_default === true ) {
+			this.client.collectDefaultMetrics();
+		}
 		if ( this.options.labels === undefined ) {
 			this.options.labels = { names: [] };
 		}
diff --git a/test/utils/simple_config_no_workers.yaml b/test/utils/simple_config_no_workers.yaml
index 1ec6a44..7140362 100644
--- a/test/utils/simple_config_no_workers.yaml
+++ b/test/utils/simple_config_no_workers.yaml
@@ -4,6 +4,7 @@ logging:
 metrics:
   - type: prometheus
     port: 9000
+    collect_default: true
   - type: statsd
     host: localhost
     port: 8125

From 6f1dc03a75fd40363a12f23b1394a840949db616 Mon Sep 17 00:00:00 2001
From: Gabriele Modena <gmodena@wikimedia.org>
Date: Mon, 4 Mar 2024 13:24:09 +0100
Subject: [PATCH 2/6] prometheus: collect_default validation in cluster

Throw an excpetion if metrics.prometheus.collect_default
is enabled in cluster mode.
---
 lib/metrics/servers/prometheus.js | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/lib/metrics/servers/prometheus.js b/lib/metrics/servers/prometheus.js
index 67a2f51..1d67dc3 100644
--- a/lib/metrics/servers/prometheus.js
+++ b/lib/metrics/servers/prometheus.js
@@ -11,6 +11,12 @@ class PrometheusServer {
 			if ( num_workers === 0 ) {
 				res.end( Prometheus.register.metrics() );
 			} else {
+				// config should be the first `metrics` block in service-runner config,
+				// that matched `type === 'prometheus'`.
+				if ( config.prometheus.collect_default !== null &&
+						config.prometheus.collect_default === true ) {
+					throw new Error( 'metrics.prometheus.collect_default cannot be enabled in cluster mode' );
+				}
 				AggregatorRegistry.clusterMetrics( ( err, metrics ) => {
 					if ( err ) {
 						this._logger.log( 'error/service-runner', err );

From b4fabf5dcedf5df817a767085fe8c4943e7341ed Mon Sep 17 00:00:00 2001
From: Gabriele Modena <gmodena@wikimedia.org>
Date: Mon, 4 Mar 2024 15:17:07 +0100
Subject: [PATCH 3/6] prometheus: fix attribute access

---
 lib/metrics/servers/prometheus.js | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/metrics/servers/prometheus.js b/lib/metrics/servers/prometheus.js
index 1d67dc3..cfe39e5 100644
--- a/lib/metrics/servers/prometheus.js
+++ b/lib/metrics/servers/prometheus.js
@@ -11,10 +11,9 @@ class PrometheusServer {
 			if ( num_workers === 0 ) {
 				res.end( Prometheus.register.metrics() );
 			} else {
-				// config should be the first `metrics` block in service-runner config,
+				// config should contain the first `metrics` block in service-runner config,
 				// that matched `type === 'prometheus'`.
-				if ( config.prometheus.collect_default !== null &&
-						config.prometheus.collect_default === true ) {
+				if ( config !== null && config.collect_default === true ) {
 					throw new Error( 'metrics.prometheus.collect_default cannot be enabled in cluster mode' );
 				}
 				AggregatorRegistry.clusterMetrics( ( err, metrics ) => {

From 7036c5004c1eeca9f6301e7f618d595fa71dda19 Mon Sep 17 00:00:00 2001
From: Gabriele Modena <gmodena@wikimedia.org>
Date: Mon, 4 Mar 2024 20:42:23 +0100
Subject: [PATCH 4/6] metric: add noop metrics.

Add a noop type of metric not registered by service-runner Metric interface.
This is used to delegate collection of default prometheus
metrics to prom-client.collectDefaultMetrics.
---
 lib/heapwatch.js                              | 21 +++++++++++++
 lib/metrics/index.js                          | 15 ++++++----
 lib/metrics/metric.js                         |  1 +
 lib/metrics/prometheus.js                     | 30 +++++++++++--------
 test/features/tests.js                        | 22 ++++++++++++++
 test/utils/simple_config_no_workers.yaml      |  1 -
 ...ple_config_no_workers_collect_default.yaml | 15 ++++++++++
 7 files changed, 87 insertions(+), 18 deletions(-)
 create mode 100644 test/utils/simple_config_no_workers_collect_default.yaml

diff --git a/lib/heapwatch.js b/lib/heapwatch.js
index 88cdad3..e01aed5 100644
--- a/lib/heapwatch.js
+++ b/lib/heapwatch.js
@@ -4,6 +4,16 @@ const cluster = require( 'cluster' );
 
 class HeapWatch {
 	constructor( conf, logger, metrics ) {
+		if ( metrics !== null ) {
+			const prometheusOptions = metrics.options.find( ( option ) =>
+				option.type === 'prometheus' && option.collect_default === true
+			);
+			// When true, a noop dummy metric will be instantiated
+			// and passed down to a PrometheusClient instance.
+			// This metric won't be registered directly by service-runner,
+			// but will be used to trigger a call to prom-client.collectDefaultMetrics().
+			this.collect_default = !!prometheusOptions;
+		}
 		this.conf = conf = conf || {};
 		this.limit = ( conf.limitMB || 1500 ) * 1024 * 1024;
 		this.logger = logger;
@@ -15,6 +25,17 @@ class HeapWatch {
 
 	watch() {
 		const usage = process.memoryUsage();
+		if ( this.collect_default ) {
+			// collect some default metrics recommended by Prometheus itself.
+			// https://prometheus.io/docs/instrumenting/writing_clientlibs/#standard-and-runtime-collectors
+			this.metrics.makeMetric( {
+				type: 'noop',
+				name: 'prometheus.default',
+				prometheus: {
+					collect_default: true
+				}
+			} );
+		}
 		this.metrics.makeMetric( {
 			type: 'Gauge',
 			name: 'heap.rss',
diff --git a/lib/metrics/index.js b/lib/metrics/index.js
index c77df63..454c2b8 100644
--- a/lib/metrics/index.js
+++ b/lib/metrics/index.js
@@ -86,12 +86,17 @@ class Metrics {
 	//     }
 	// }
 	makeMetric( options ) {
-		let metric = this.cache.get( options.name );
-		if ( metric === undefined ) {
-			metric = new Metric( this.clients, this.logger, options );
-			this.cache.set( options.name, metric );
+		// noop is a kind of metric not registered by service-runner Metric interface.
+		// This is used to delegate collection of default prometheus
+		// metrics to prom-client.collectDefaultMetrics.
+		if ( options.collect_default === undefined || options.type !== 'noop' ) {
+			let metric = this.cache.get( options.name );
+			if ( metric === undefined ) {
+				metric = new Metric( this.clients, this.logger, options );
+				this.cache.set( options.name, metric );
+			}
+			return metric;
 		}
-		return metric;
 	}
 
 	close() {
diff --git a/lib/metrics/metric.js b/lib/metrics/metric.js
index 3dd5496..83e168b 100644
--- a/lib/metrics/metric.js
+++ b/lib/metrics/metric.js
@@ -81,6 +81,7 @@ class Metric {
 			this.logger.log( 'error/metrics', `endTiming() unsupported for metric type ${ this.type }` );
 		}
 	}
+
 }
 
 module.exports = Metric;
diff --git a/lib/metrics/prometheus.js b/lib/metrics/prometheus.js
index 920c164..fb85abb 100644
--- a/lib/metrics/prometheus.js
+++ b/lib/metrics/prometheus.js
@@ -8,11 +8,6 @@ class PrometheusMetric {
 	constructor( options, client ) {
 		this.client = client;
 		this.options = cloneDeep( options );
-
-		// TODO: update eslint rules to allow optional chaining operator `?`
-		if ( this.options.prometheus && this.options.prometheus.collect_default === true ) {
-			this.client.collectDefaultMetrics();
-		}
 		if ( this.options.labels === undefined ) {
 			this.options.labels = { names: [] };
 		}
@@ -26,13 +21,24 @@ class PrometheusMetric {
 		this.options.labels.names = this.options.labels.names.map( this._normalize );
 
 		this.options.prometheus.name = this._normalize( this.options.prometheus.name );
-		this.metric = new this.client[ this.options.type ]( {
-			name: this.options.prometheus.name,
-			help: this.options.prometheus.help,
-			labelNames: this.options.labels.names,
-			buckets: this.options.prometheus.buckets,
-			percentiles: this.options.prometheus.percentiles
-		} );
+		if ( this.options.type !== 'noop' ) {
+			this.metric = new this.client[ this.options.type ]( {
+				name: this.options.prometheus.name,
+				help: this.options.prometheus.help,
+				labelNames: this.options.labels.names,
+				buckets: this.options.prometheus.buckets,
+				percentiles: this.options.prometheus.percentiles
+			} );
+		} else if ( this.options.prometheus && this.options.prometheus.collect_default === true ) {
+			// TODO: update eslint rules to allow optional chaining operator `?`
+			// A no op metric.
+			// Invoke collectDefaultMetrics() but don't register any new metric
+			// via the service-runner Metric interface.
+			// https://prometheus.io/docs/instrumenting/writing_clientlibs/#standard-and-runtime-collectors
+			// TODO: collectDefaultMetrics() does not support providing labels.
+			//  Default labels should be set for the whole registry.
+			this.client.collectDefaultMetrics();
+		}
 	}
 
 	/**
diff --git a/test/features/tests.js b/test/features/tests.js
index d78ab4d..21102ab 100644
--- a/test/features/tests.js
+++ b/test/features/tests.js
@@ -101,6 +101,28 @@ describe( 'service-runner tests', () => {
 			.finally( () => process.removeListener( 'warning', warningListener ) );
 	} );
 
+	it( 'Must produce prometheus default metrics when hit ', () => {
+		const server = new TestServer( `${__dirname}/../utils/simple_config_no_workers_collect_default.yaml` );
+		const response = { status: null, body: null };
+		return server.start()
+			.then( () => {
+				preq.get( { uri: 'http://127.0.0.1:9000' } )
+					.then( ( res ) => {
+						response.status = res.status;
+						response.body = res.body;
+					} );
+			} )
+			.delay( 1000 )
+			.then( () => {
+				// nodejs_version_info is reported by calls to prom-client.collectDefaultMetrics()
+				assert.ok(
+					response.body.includes( 'nodejs_version_info ' ),
+					'Must collect default metrics prometheus output.'
+				);
+			} )
+			.finally( () => server.stop() );
+	} );
+
 	// preq prevents the AssertionErrors from surfacing and failing the test
 	// performing the test this way presents them correctly
 	it( 'Must increment hitcount metrics when hit, no workers', () => {
diff --git a/test/utils/simple_config_no_workers.yaml b/test/utils/simple_config_no_workers.yaml
index 7140362..1ec6a44 100644
--- a/test/utils/simple_config_no_workers.yaml
+++ b/test/utils/simple_config_no_workers.yaml
@@ -4,7 +4,6 @@ logging:
 metrics:
   - type: prometheus
     port: 9000
-    collect_default: true
   - type: statsd
     host: localhost
     port: 8125
diff --git a/test/utils/simple_config_no_workers_collect_default.yaml b/test/utils/simple_config_no_workers_collect_default.yaml
new file mode 100644
index 0000000..ff97b0f
--- /dev/null
+++ b/test/utils/simple_config_no_workers_collect_default.yaml
@@ -0,0 +1,15 @@
+num_workers: 0
+logging:
+  level: fatal
+metrics:
+  - type: prometheus
+    port: 9000
+    collect_default:  true
+  - type: statsd
+    host: localhost
+    port: 8125
+services:
+  - name: test
+    module: test/utils/simple_server.js
+    conf:
+      port: 12345

From 7461e2793bc5823b86028e66f23bafcebe159a23 Mon Sep 17 00:00:00 2001
From: "James D. Forrester" <jforrester@wikimedia.org>
Date: Sat, 4 May 2024 18:21:38 +0300
Subject: [PATCH 5/6] build: Apply eslint fixes to the GC stack

---
 config.yaml                                              | 1 -
 lib/worker.js                                            | 1 +
 test/features/tests.js                                   | 2 +-
 test/utils/simple_config_no_workers_collect_default.yaml | 2 +-
 4 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/config.yaml b/config.yaml
index aa87fe3..57e6747 100644
--- a/config.yaml
+++ b/config.yaml
@@ -42,4 +42,3 @@ services:
       port: 12345
       interface: localhost
       # more per-service config settings
-
diff --git a/lib/worker.js b/lib/worker.js
index 9d9a410..00b59a6 100644
--- a/lib/worker.js
+++ b/lib/worker.js
@@ -119,6 +119,7 @@ class Worker extends BaseService {
 		// For 0.10: npm install heapdump
 		process.on( 'SIGUSR2', this._dumpHeapHandler = () => {
 			try {
+				// eslint-disable-next-line n/no-missing-require
 				const heapdump = require( 'heapdump' );
 				const cwd = process.cwd();
 				console.error( 'SIGUSR2 received! Writing snapshot.' );
diff --git a/test/features/tests.js b/test/features/tests.js
index 21102ab..27596e7 100644
--- a/test/features/tests.js
+++ b/test/features/tests.js
@@ -102,7 +102,7 @@ describe( 'service-runner tests', () => {
 	} );
 
 	it( 'Must produce prometheus default metrics when hit ', () => {
-		const server = new TestServer( `${__dirname}/../utils/simple_config_no_workers_collect_default.yaml` );
+		const server = new TestServer( `${ __dirname }/../utils/simple_config_no_workers_collect_default.yaml` );
 		const response = { status: null, body: null };
 		return server.start()
 			.then( () => {
diff --git a/test/utils/simple_config_no_workers_collect_default.yaml b/test/utils/simple_config_no_workers_collect_default.yaml
index ff97b0f..7140362 100644
--- a/test/utils/simple_config_no_workers_collect_default.yaml
+++ b/test/utils/simple_config_no_workers_collect_default.yaml
@@ -4,7 +4,7 @@ logging:
 metrics:
   - type: prometheus
     port: 9000
-    collect_default:  true
+    collect_default: true
   - type: statsd
     host: localhost
     port: 8125

From 2bee7361836cbdefb4bdfb00641483ef7a6ee9c1 Mon Sep 17 00:00:00 2001
From: "James D. Forrester" <jforrester@wikimedia.org>
Date: Thu, 12 Dec 2024 11:17:07 -0500
Subject: [PATCH 6/6] tests: Re-work to use fetch not removed preq, per rebase

---
 lib/heapwatch.js       |  4 ++--
 lib/worker.js          |  1 -
 test/features/tests.js | 12 ++++++------
 3 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/lib/heapwatch.js b/lib/heapwatch.js
index e01aed5..5f93ba3 100644
--- a/lib/heapwatch.js
+++ b/lib/heapwatch.js
@@ -5,8 +5,8 @@ const cluster = require( 'cluster' );
 class HeapWatch {
 	constructor( conf, logger, metrics ) {
 		if ( metrics !== null ) {
-			const prometheusOptions = metrics.options.find( ( option ) =>
-				option.type === 'prometheus' && option.collect_default === true
+			const prometheusOptions = metrics.options.find(
+				( option ) => option.type === 'prometheus' && option.collect_default === true
 			);
 			// When true, a noop dummy metric will be instantiated
 			// and passed down to a PrometheusClient instance.
diff --git a/lib/worker.js b/lib/worker.js
index 00b59a6..9d9a410 100644
--- a/lib/worker.js
+++ b/lib/worker.js
@@ -119,7 +119,6 @@ class Worker extends BaseService {
 		// For 0.10: npm install heapdump
 		process.on( 'SIGUSR2', this._dumpHeapHandler = () => {
 			try {
-				// eslint-disable-next-line n/no-missing-require
 				const heapdump = require( 'heapdump' );
 				const cwd = process.cwd();
 				console.error( 'SIGUSR2 received! Writing snapshot.' );
diff --git a/test/features/tests.js b/test/features/tests.js
index 27596e7..bfd35e1 100644
--- a/test/features/tests.js
+++ b/test/features/tests.js
@@ -105,11 +105,13 @@ describe( 'service-runner tests', () => {
 		const server = new TestServer( `${ __dirname }/../utils/simple_config_no_workers_collect_default.yaml` );
 		const response = { status: null, body: null };
 		return server.start()
-			.then( () => {
-				preq.get( { uri: 'http://127.0.0.1:9000' } )
-					.then( ( res ) => {
+			.then( async () => {
+				// eslint-disable-next-line n/no-unsupported-features/node-builtins
+				await fetch( 'http://127.0.0.1:9000' )
+					.then( async ( res ) => {
 						response.status = res.status;
-						response.body = res.body;
+						// This is a ReadableStream of a Uint8Array, but we just want the string
+						response.body = new TextDecoder().decode( await res.arrayBuffer() );
 					} );
 			} )
 			.delay( 1000 )
@@ -123,8 +125,6 @@ describe( 'service-runner tests', () => {
 			.finally( () => server.stop() );
 	} );
 
-	// preq prevents the AssertionErrors from surfacing and failing the test
-	// performing the test this way presents them correctly
 	it( 'Must increment hitcount metrics when hit, no workers', () => {
 		const server = new TestServer( `${ __dirname }/../utils/simple_config_no_workers.yaml` );
 		const response = { status: null, body: null };