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

Non-string label values bypassed escapeLabelValue() and could render
malformed exposition (#791): a value whose string form contains a quote
or a newline produced invalid output. Escaping them during metrics() was
rejected in #792/#793 because metrics() cost is linear in total
cardinality; the maintainer's counterproposal is to pay the cost at the
storage boundary instead, once per new label combination.

- LabelMap gains a single insertion point (#insert), used by set,
  setDelta, getOrAdd and merge. It replaces the entry's labels with a
  copy the store owns, coercing every value with template interpolation
  - the same ToString the exposition applies.
- The copy is unconditional. The entry keeps those labels for its
  lifetime, so holding a reference the caller can still mutate would let
  a later mutation change what a stored series reports. Only a
  combination's first record reaches #insert, so recording an existing
  combination copies nothing.
- normalizeLabels() walks the source with for...in, which picks up
  inherited enumerable labels; keyFrom() reads labels by name and sees
  those too, so a copy without them could not reproduce the key its
  entry is filed under, and remove(entry.labels) - which Summary's
  pruning uses - would quietly miss.
- __proto__ is defined with Object.defineProperty: it passes the label
  name regexp, and plain assignment would invoke the prototype setter
  instead of defining a property, dropping the label. The spread that
  seeds the copy is for object shape - building it key by key instead
  costs about 24 bytes per stored series.
- Nullish values are copied as-is: keyFrom() treats them as absent, so
  coercing them would make stored labels compute a different key than
  the one they are stored under, and would collapse {a: null} with
  {a: 'null'} after serialization. Their rendered form needs no
  escaping anyway.
- merge() keeps the stored labels on update instead of overwriting them
  with the caller's raw object.
- Summary's stored value no longer carries a second copy of the labels;
  the export helpers take entry.labels, so getOrAdd() is back to calling
  init() with no arguments.
- LabelGrouper deliberately does NOT normalize: aggregation input comes
  from registry.getMetricsAsJSON(), whose store-backed labels were
  already normalized on first insertion, so re-checking every value
  would tax aggregate() for work the stores already did. Labels that
  never pass through the stores (custom collectors, registry default
  labels) flow through aggregation unchanged, as before.

Measured on Node 24.11 (arm64), one implementation per process, median
of 9 samples: recording an existing combination is unchanged (51.5ns ->
51.9ns); a new combination's first insertion costs 8-22% more depending
on label count, and the per-record cost is back within noise by about a
hundred records of that series. The repo's benchmark suite reports no
significant regressions across its 46 cases. Retained heap at 250k
unique series is unchanged (64.6MiB vs 64.7MiB).

Observable changes: label values that pass through the built-in stores
are reported as strings ('3' instead of 3) by getMetricsAsJSON(), metric
get(), worker payloads and aggregation output; and a label-less summary
reports labels: {} rather than labels: undefined, matching the other
metric types. Both are noted in the changelog. Custom collector results,
registry default labels and exemplar labels do not pass through the
stores and are unchanged.

Fixes #791

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

Author: milcho0604

CHANGELOG.md

@@ -46,6 +46,11 @@ This release marks our first release under the Prometheus umbrella.
 - chore: Add copyright license headers and test
 - Make cluster and worker-thread metric aggregation order deterministic
 - Export `MetricObject`, `MetricObjectWithValues`, `MetricValue` and `MetricValueWithName` from the TypeScript definitions
+- fix: Non-string label values (except `null`/`undefined`) are coerced to strings when a
+  combination is first stored, so exposition escapes them and `getMetricsAsJSON()` reports
+  them as strings. The store now keeps its own copy of the labels: 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

@@ -72,9 +72,10 @@ class Summary extends Metric {
 			if (this.pruneAgedBuckets && s.td.size() === 0) {
 				this.store.remove(entry.labels);
 			} else {
-				values.push(...extractSummariesForExport(s, this.percentiles));
-				values.push(getSumForExport(s, this));
-				values.push(getCountForExport(s, this));
+				const labels = entry.labels;
+				values.push(...extractSummariesForExport(s, labels, this.percentiles));
+				values.push(getSumForExport(s, labels, this));
+				values.push(getCountForExport(s, labels, this));
 			}
 		}
 
@@ -126,30 +127,30 @@ class Summary extends Metric {
 	}
 }
 
-function extractSummariesForExport(summaryOfLabels, percentiles) {
+function extractSummariesForExport(summaryOfLabels, labels, percentiles) {
 	summaryOfLabels.td.compress();
 
 	return percentiles.map(percentile => {
 		const percentileValue = summaryOfLabels.td.percentile(percentile);
 		return {
-			labels: Object.assign({ quantile: percentile }, summaryOfLabels.labels),
+			labels: Object.assign({ quantile: percentile }, labels),
 			value: percentileValue ? percentileValue : 0,
 		};
 	});
 }
 
-function getCountForExport(value, summary) {
+function getCountForExport(value, labels, summary) {
 	return {
 		metricName: `${summary.name}_count`,
-		labels: value.labels,
+		labels,
 		value: value.count,
 	};
 }
 
-function getSumForExport(value, summary) {
+function getSumForExport(value, labels, summary) {
 	return {
 		metricName: `${summary.name}_sum`,
-		labels: value.labels,
+		labels,
 		value: value.sum,
 	};
 }
@@ -179,7 +180,6 @@ 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,

lib/util.js

@@ -168,6 +168,72 @@ exports.nowTimestamp = function nowTimestamp() {
  * @property labels {object}
  */
 
+/**
+ * Copy the labels the store is about to take ownership of, coercing
+ * non-nullish values to the string Prometheus expects. Coercion uses
+ * template interpolation — exactly what rendering would have done later —
+ * so the rendered form is unchanged while escaping no longer gets
+ * skipped (#791).
+ *
+ * The copy is unconditional: the entry keeps these labels for its lifetime,
+ * so holding on to an object the caller can still mutate would let a later
+ * mutation change what a stored series reports.
+ *
+ * Nullish values are copied as-is: `keyFrom()` treats them as absent, so
+ * coercing them to `"null"`/`"undefined"` would make the stored labels
+ * compute a different key than the one they are stored under — breaking
+ * `remove(entry.labels)` round-trips (Summary's pruning does exactly that)
+ * and collapsing `{a: null}` with `{a: 'null'}` after serialization. Their
+ * rendered form (`"null"`) contains nothing that needs escaping anyway.
+ *
+ * Two corner cases are deliberately unsupported (each needs another
+ * `for...in`-visible property present — alone, `isEmpty()` keeps original
+ * and copy agreed on `''`):
+ * - `Object.create(null)` labels missing a declared name that collides with
+ *   `Object.prototype` (`constructor`, …): the plain copy reads the
+ *   inherited member where the original read `undefined`.
+ * - Non-enumerable label properties: no enumeration sees them, so the copy
+ *   drops what `keyFrom()` read by property access.
+ * Symbols are not label data: `keyFrom()` iterates declared names,
+ * exposition uses `Object.entries()`; neither reads them.
+ * @param {object} labels
+ * @returns {object} a copy owned by the store
+ */
+function normalizeLabels(labels) {
+	// Spread first, for the packed object shape V8 gives a clone: building the
+	// copy up key by key instead costs about 24 bytes per stored series.
+	const copy = { ...labels };
+
+	// Then walk the source with `for...in`, which picks up inherited enumerable
+	// labels too. `keyFrom()` reads labels by name, so it sees those as well, and
+	// a copy that dropped them could not reproduce the key its entry is filed
+	// under — `remove(entry.labels)`, which Summary's pruning uses, would quietly
+	// miss. Non-enumerable labels stay out of reach of any enumeration; they only
+	// survived before because the store kept the caller's object.
+	for (const name in labels) {
+		const value = labels[name];
+		const stored =
+			typeof value === 'string' || value === null || value === undefined
+				? value
+				: `${value}`;
+
+		if (name === '__proto__') {
+			// A legal label name, but assigning it would invoke the prototype
+			// setter instead of defining a property, dropping 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.
  */
@@ -182,6 +248,22 @@ class LabelMap {
 		this.#labelNames = new Set(labelNames.slice().sort());
 	}
 
+	/**
+	 * The single insertion point — every new label combination enters the map
+	 * here, and takes its own copy of the labels on the way in. Only a
+	 * combination's first record reaches this method, so the recording fast
+	 * path for existing combinations copies nothing.
+	 * @param {string} key precomputed `keyFrom(entry.labels)`
+	 * @param {StatsEntry} entry
+	 * @returns {StatsEntry}
+	 */
+	#insert(key, entry) {
+		entry.labels = normalizeLabels(entry.labels);
+		this.#map.set(key, entry);
+
+		return entry;
+	}
+
 	/**
 	 * @function setValue
 	 * @param {object} labels
@@ -195,7 +277,7 @@ class LabelMap {
 		if (entry !== undefined) {
 			entry.value = value;
 		} else {
-			this.#map.set(key, { value, labels });
+			this.#insert(key, { value, labels });
 		}
 
 		return this;
@@ -214,7 +296,7 @@ class LabelMap {
 		if (entry !== undefined) {
 			entry.value += value;
 		} else {
-			this.#map.set(key, { value, labels });
+			this.#insert(key, { value, labels });
 		}
 
 		return this;
@@ -244,8 +326,7 @@ class LabelMap {
 		let entry = this.#map.get(key);
 
 		if (entry === undefined) {
-			entry = { value: init(), labels };
-			this.#map.set(key, entry);
+			entry = this.#insert(key, { value: init(), labels });
 		}
 
 		return entry.value;
@@ -273,10 +354,11 @@ class LabelMap {
 
 		let entry = this.#map.get(key);
 		if (entry !== undefined) {
-			Object.assign(entry, values, { labels });
+			// Keep the stored labels: they were copied on first insertion and
+			// identify the same combination (same key).
+			Object.assign(entry, values, { labels: entry.labels });
 		} else {
-			entry = { ...values, labels };
-			this.#map.set(key, entry);
+			entry = this.#insert(key, { ...values, labels });
 		}
 
 		return entry;
@@ -400,6 +482,13 @@ class LabelGrouper {
 
 	/**
 	 * Adds the `value` to the `key`'s array of values.
+	 *
+	 * NB: no label normalization here, by design. Aggregation input comes from
+	 * `registry.getMetricsAsJSON()`, whose store-backed labels were already
+	 * normalized by LabelMap on first insertion — re-checking every value on
+	 * this path would tax `aggregate()` for work the stores already did.
+	 * Labels that never pass through the stores (custom collector results,
+	 * registry default labels) arrive here as-is, unchanged from before.
 	 * @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('should output metrics as JSON', () => {
 			it('should output metrics as JSON', async () => {
 				register.registerMetric(getMetric());
@@ -757,7 +798,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);