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

Non-string label values bypassed escapeLabelValue() and could render
malformed exposition (#791). Escaping during metrics() costs linear in
total cardinality (#792/#793 were declined for that), so values are
coerced at the storage boundary instead, once per new combination.

- LabelMap gains one insertion point, #insert, used by set, setDelta,
  getOrAdd and merge. The entry gets a copy the store owns, coerced
  with the same ToString the exposition applies, so a caller mutating
  its object afterwards cannot change what a stored series reports.
- normalizeLabels() walks with for...in, because keyFrom() reads
  inherited enumerable labels too. Labels that no enumeration reaches,
  or whose prototype chain intercepts writes, are unsupported. The copy
  keeps the source prototype, so keyFrom() answers absent declared names
  the way the source did.
  Nullish values are copied as-is, and __proto__ needs
  Object.defineProperty rather than assignment.
- merge() keeps the stored labels instead of overwriting them with the
  caller's object. getOrAdd() hands the stored labels to init(), so
  Summary's value holds that same object rather than the caller's, and
  its export helpers are unchanged from main. LabelGrouper does not
  normalize; the store-backed labels it receives are normalized already.

Benchmarks and the observable output changes are in the PR description.

Fixes #791

Signed-off-by: Changhyun Kim <milcho0604@gmail.com>

Author: milcho0604

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);