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

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