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

CHANGELOG.md

@@ -12,6 +12,8 @@ project adheres to [Semantic Versioning](http://semver.org/).
 ### Changed
 
 - Organized default metrics
+- fix: 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

@@ -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

@@ -202,6 +202,39 @@ exports.waitFor = async function waitFor(promise, limit = 5_000) {
  * @property labels {object}
  */
 
+/**
+ * Copy the labels the store takes ownership of, coercing non-nullish values
+ * to strings so exposition escapes them (#791).
+ * @param {object} labels
+ * @returns {object} a copy owned by the store
+ */
+function normalizeLabels(labels) {
+	const copy = { ...labels };
+
+	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 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.
  */
@@ -216,6 +249,20 @@ 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.
+	 * @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
@@ -229,7 +276,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 +295,7 @@ class LabelMap {
 		if (entry !== undefined) {
 			entry.value += value;
 		} else {
-			this.#map.set(key, { value, labels });
+			this.#insert(key, { value, labels });
 		}
 
 		return this;
@@ -278,8 +325,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;
@@ -307,10 +353,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 +479,9 @@ class LabelGrouper {
 
 	/**
 	 * Adds the `value` to the `key`'s array of values.
+	 *
+	 * NB: no label normalization here, by design. Store-backed labels were
+	 * already normalized on first insertion.
 	 * @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);