fix: normalize label values once, when a label combination is first stored

CHANGELOG.md

@@ -16,6 +16,8 @@ project adheres to [Semantic Versioning](http://semver.org/).
 - perf: Histogram rendering builds its export list straight from the store iterator instead of an intermediate array. Faster at high series counts on Node 24 and 26, can be slightly slower on Node 22
 - fix: Correct content type exported for cluster and worker mode.
 - perf: Remove truthy conditionals from default metric collectors
+- fix: Non-nullish, non-string label values are coerced to strings when a combination is first stored, so exposition escapes them; the store also keeps its own copy, so mutating the caller's object after recording no longer changes the stored series
+- fix: Label-less summaries report `labels: {}` in `getMetricsAsJSON()`, like other metrics
 
 ### Added
 

lib/summary.js

@@ -39,14 +39,14 @@ class Summary extends Metric {
 		this.store = new LabelMap(this.labelNames);
 
 		if (this.labelNames.length === 0) {
-			this.store.set(
-				{},
-				{
+			this.store.getOrAdd({}, storedLabels => {
+				return {
+					labels: storedLabels,
 					td: new timeWindowQuantiles(this.maxAgeSeconds, this.ageBuckets),
 					count: 0,
 					sum: 0,
-				},
-			);
+				};
+			});
 		}
 	}
 
@@ -177,14 +177,17 @@ function observe(labels) {
 			);
 		}
 
-		const summaryOfLabel = this.store.getOrAdd(labelValuePair.labels, () => {
-			return {
-				labels: labelValuePair.labels,
-				td: new timeWindowQuantiles(this.maxAgeSeconds, this.ageBuckets),
-				count: 0,
-				sum: 0,
-			};
-		});
+		const summaryOfLabel = this.store.getOrAdd(
+			labelValuePair.labels,
+			storedLabels => {
+				return {
+					labels: storedLabels,
+					td: new timeWindowQuantiles(this.maxAgeSeconds, this.ageBuckets),
+					count: 0,
+					sum: 0,
+				};
+			},
+		);
 
 		summaryOfLabel.td.push(labelValuePair.value);
 		summaryOfLabel.count++;

lib/util.js

@@ -202,6 +202,40 @@ exports.waitFor = async function waitFor(promise, limit = 5_000) {
  * @property labels {object}
  */
 
+/**
+ * Copy the labels the store owns, coercing values for exposition (#791).
+ * @param {object} labels
+ * @returns {object} a copy owned by the store
+ */
+function normalizeLabels(labels) {
+	// Keep the source prototype: keyFrom() reads absent declared names off it.
+	const proto = Object.getPrototypeOf(labels);
+	const copy =
+		proto === Object.prototype ? { ...labels } : Object.create(proto);
+
+	for (const name in labels) {
+		const value = labels[name];
+		const stored =
+			typeof value === 'string' || value === null || value === undefined
+				? value
+				: `${value}`;
+
+		if (name === '__proto__') {
+			// Assigning would hit the prototype setter and drop the label.
+			Object.defineProperty(copy, name, {
+				value: stored,
+				writable: true,
+				enumerable: true,
+				configurable: true,
+			});
+		} else {
+			copy[name] = stored;
+		}
+	}
+
+	return copy;
+}
+
 /**
  * Lookup table for stats by labels.
  */
@@ -216,6 +250,22 @@ class LabelMap {
 		this.#labelNames = new Set(labelNames.slice().sort());
 	}
 
+	/**
+	 * The single insertion point for new label combinations.
+	 * @param {string} key precomputed `keyFrom(entry.labels)`
+	 * @param {StatsEntry} entry
+	 * @param {[Function]} init optional factory, receives the stored labels
+	 * @returns {StatsEntry}
+	 */
+	#insert(key, entry, init) {
+		entry.labels = normalizeLabels(entry.labels);
+		// init() runs before the entry lands, so a throw leaves the map untouched.
+		if (init) entry.value = init(entry.labels);
+		this.#map.set(key, entry);
+
+		return entry;
+	}
+
 	/**
 	 * @function setValue
 	 * @param {object} labels
@@ -229,7 +279,7 @@ class LabelMap {
 		if (entry !== undefined) {
 			entry.value = value;
 		} else {
-			this.#map.set(key, { value, labels });
+			this.#insert(key, { value, labels });
 		}
 
 		return this;
@@ -248,7 +298,7 @@ class LabelMap {
 		if (entry !== undefined) {
 			entry.value += value;
 		} else {
-			this.#map.set(key, { value, labels });
+			this.#insert(key, { value, labels });
 		}
 
 		return this;
@@ -270,16 +320,15 @@ class LabelMap {
 	 * called to create an object to put there. This allows for nested structures.
 	 *
 	 * @param {object} labels labels for the new entry
-	 * @param {[Function]} init function to generate an empty record
+	 * @param {[Function]} init receives the stored labels, returns an empty record
 	 * @returns {*} the existing value or the result of init()
 	 */
 	getOrAdd(labels, init) {
 		const key = this.keyFrom(labels);
 		let entry = this.#map.get(key);
 
 		if (entry === undefined) {
-			entry = { value: init(), labels };
-			this.#map.set(key, entry);
+			entry = this.#insert(key, { labels }, init);
 		}
 
 		return entry.value;
@@ -307,10 +356,9 @@ class LabelMap {
 
 		let entry = this.#map.get(key);
 		if (entry !== undefined) {
-			Object.assign(entry, values, { labels });
+			Object.assign(entry, values, { labels: entry.labels });
 		} else {
-			entry = { ...values, labels };
-			this.#map.set(key, entry);
+			entry = this.#insert(key, { ...values, labels });
 		}
 
 		return entry;
@@ -434,6 +482,8 @@ class LabelGrouper {
 
 	/**
 	 * Adds the `value` to the `key`'s array of values.
+	 *
+	 * NB: no normalization here. Store-backed labels arrive normalized.
 	 * @param {StatsEntry} value Value to add to `key`'s array.
 	 * @returns {LabelGrouper} undefined.
 	 */

test/defaultMetricsTest.js

@@ -101,7 +101,8 @@ describe.each([
 		expect(allMetricValues.length).toBeGreaterThan(0);
 
 		allMetricValues.forEach(metricValue => {
-			expect(metricValue.labels).toMatchObject(labels);
+			// Label values are normalized to strings at the storage boundary.
+			expect(metricValue.labels).toMatchObject({ NODE_APP_INSTANCE: '0' });
 		});
 	});
 

test/metrics/versionTest.js

@@ -25,9 +25,10 @@ function expectVersionMetrics(metrics) {
 	expect(metrics[0].type).toEqual('gauge');
 	expect(metrics[0].name).toEqual('nodejs_version_info');
 	expect(metrics[0].values[0].labels.version).toEqual(nodeVersion);
-	expect(metrics[0].values[0].labels.major).toEqual(versionSegments[0]);
-	expect(metrics[0].values[0].labels.minor).toEqual(versionSegments[1]);
-	expect(metrics[0].values[0].labels.patch).toEqual(versionSegments[2]);
+	// Label values are normalized to strings at the storage boundary.
+	expect(metrics[0].values[0].labels.major).toEqual(`${versionSegments[0]}`);
+	expect(metrics[0].values[0].labels.minor).toEqual(`${versionSegments[1]}`);
+	expect(metrics[0].values[0].labels.patch).toEqual(`${versionSegments[2]}`);
 }
 
 describe.each([

test/registerTest.js

@@ -340,6 +340,47 @@ describe('Register', () => {
 			expect(escapedResult).toMatch(/\\"/);
 		});
 
+		it('should escape non-string label values recorded through a metric', async () => {
+			const gauge = new Gauge({
+				name: 'test_metric',
+				help: 'A test metric',
+				labelNames: ['label', 'code', 'count'],
+			});
+			gauge.set({ label: ['say "hi"'], code: ['a\nb'], count: 3 }, 12);
+
+			const escapedResult = await register.metrics();
+			expect(escapedResult).toMatch(/label="say \\"hi\\""/);
+			expect(escapedResult).toMatch(/code="a\\nb"/);
+			expect(escapedResult).toMatch(/count="3"/);
+		});
+
+		it('should escape summary labels stored inside the summary value', async () => {
+			const summary = new Summary({
+				name: 'test_summary',
+				help: 'A test summary',
+				labelNames: ['x'],
+				percentiles: [0.5],
+			});
+			summary.observe({ x: ['say "hi"'] }, 1);
+
+			const escapedResult = await register.metrics();
+			expect(escapedResult).toMatch(/x="say \\"hi\\""/);
+		});
+
+		it('should render inherited enumerable labels recorded through a metric', async () => {
+			const gauge = new Gauge({
+				name: 'test_metric',
+				help: 'A test metric',
+				labelNames: ['region', 'method'],
+			});
+			const labels = Object.create({ region: 'eu' });
+			labels.method = 'GET';
+			gauge.set(labels, 1);
+
+			const result = await register.metrics();
+			expect(result).toContain('test_metric{method="GET",region="eu"} 1');
+		});
+
 		describe('getMetricsAsArray()', () => {
 			it('should return metrics', async () => {
 				register.registerMetric(getMetric());
@@ -831,7 +872,9 @@ describe('Register', () => {
 		});
 
 		describe('AggregatorRegistry.aggregate()', () => {
-			// These mimic the output of `getMetricsAsJSON`.
+			// Direct aggregate inputs exercising label pass-through. aggregate()
+			// does not normalize, so raw numeric labels here stay raw. (Store-backed
+			// labels in real `getMetricsAsJSON` output arrive already normalized.)
 			const metrics1 = [
 				{
 					name: 'test_histogram',

test/summaryTest.js

@@ -58,6 +58,16 @@ describe.each([
 				expect((await instance.get()).values[8].value).toEqual(1);
 			});
 
+			it('should report empty labels for sum and count', async () => {
+				instance.observe(100);
+				// Through the registry, because that is the documented shape.
+				const [{ values }] = await globalRegistry.getMetricsAsJSON();
+				expect(values[7].metricName).toEqual('summary_test_sum');
+				expect(values[7].labels).toEqual({});
+				expect(values[8].metricName).toEqual('summary_test_count');
+				expect(values[8].labels).toEqual({});
+			});
+
 			it('should validate labels when observing', async () => {
 				const summary = new Summary({
 					name: 'foobar',
@@ -184,6 +194,18 @@ describe.each([
 					});
 				});
 
+				it("should report the stored labels, not the caller's object", async () => {
+					const labels = { method: 3, endpoint: '/test' };
+					instance.observe(labels, 50);
+					labels.method = 'mutated afterwards';
+
+					const { values } = await instance.get();
+					expect(values).toHaveLength(3);
+					for (const value of values) {
+						expect(value.labels.method).toEqual('3');
+					}
+				});
+
 				it('should record and calculate the correct values per label', async () => {
 					instance.labels('GET', '/test').observe(50);
 					instance.labels('POST', '/test').observe(100);