From de2b36280d36f0b0ea5c8783b4ec98a5374f5eba Mon Sep 17 00:00:00 2001 From: Changhyun Kim Date: Sat, 1 Aug 2026 23:16:25 +0900 Subject: [PATCH] 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 --- CHANGELOG.md | 5 + lib/summary.js | 20 ++-- lib/util.js | 103 ++++++++++++++++++-- test/defaultMetricsTest.js | 3 +- test/metrics/versionTest.js | 7 +- test/registerTest.js | 45 ++++++++- test/summaryTest.js | 22 +++++ test/utilTest.js | 182 +++++++++++++++++++++++++++++++++--- 8 files changed, 353 insertions(+), 34 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4507479f..4ecead0d 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/summary.js b/lib/summary.js index 94a14748..2ca22fa0 100644 --- a/lib/summary.js +++ b/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, diff --git a/lib/util.js b/lib/util.js index 18dca108..e3804dff 100644 --- a/lib/util.js +++ b/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. */ diff --git a/test/defaultMetricsTest.js b/test/defaultMetricsTest.js index ff41e70f..b713554d 100644 --- a/test/defaultMetricsTest.js +++ b/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' }); }); }); diff --git a/test/metrics/versionTest.js b/test/metrics/versionTest.js index 0f2cbc4e..3037df08 100644 --- a/test/metrics/versionTest.js +++ b/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([ diff --git a/test/registerTest.js b/test/registerTest.js index ce4a8757..564cc3ad 100644 --- a/test/registerTest.js +++ b/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', diff --git a/test/summaryTest.js b/test/summaryTest.js index 30cfe1cb..22031c31 100644 --- a/test/summaryTest.js +++ b/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); diff --git a/test/utilTest.js b/test/utilTest.js index c1fe8ebc..ae91b298 100644 --- a/test/utilTest.js +++ b/test/utilTest.js @@ -95,7 +95,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 3, labels: { a: 2 } }, + { value: 3, labels: { a: '2' } }, ]); }); @@ -107,7 +107,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 4, labels: { a: 2 } }, + { value: 4, labels: { a: '2' } }, ]); }); @@ -120,11 +120,11 @@ describe('utils', () => { expect(Array.from(map.values())).toStrictEqual([ { value: 22, - labels: { a: 2 }, + labels: { a: '2' }, }, { value: 3, - labels: { a: 3 }, + labels: { a: '3' }, }, ]); }); @@ -138,7 +138,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 3, labels: { a: 2 } }, + { value: 3, labels: { a: '2' } }, ]); }); @@ -149,7 +149,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 3 + 4, labels: { a: 2 } }, + { value: 3 + 4, labels: { a: '2' } }, ]); }); @@ -161,8 +161,8 @@ describe('utils', () => { expect(map.size).toEqual(2); expect(Array.from(map.values())).toStrictEqual([ - { value: 3, labels: { a: 2 } }, - { value: 3, labels: { a: 3 } }, + { value: 3, labels: { a: '2' } }, + { value: 3, labels: { a: '3' } }, ]); }); }); @@ -197,7 +197,7 @@ describe('utils', () => { expect(map.entry({ b: 22 })).toStrictEqual({ value: 10, - labels: { b: 22 }, + labels: { b: '22' }, }); }); }); @@ -263,8 +263,8 @@ describe('utils', () => { expect(actual).toStrictEqual(4); expect(Array.from(map.values())).toStrictEqual([ - { value: [2, 3], labels: { c: 200 } }, - { value: 4, labels: { c: 401 } }, + { value: [2, 3], labels: { c: '200' } }, + { value: 4, labels: { c: '401' } }, ]); expect(callback).toHaveBeenCalled(); }); @@ -289,7 +289,165 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 4, labels: { a: 3 } }, + { value: 4, labels: { a: '3' } }, + ]); + }); + }); + + describe('label normalization', () => { + it('coerces non-string label values once, at insertion', () => { + const map = new LabelMap(['a', 'b']); + + map.set({ a: 3, b: true }, 1); + + expect(Array.from(map.values())).toStrictEqual([ + { value: 1, labels: { a: '3', b: 'true' } }, + ]); + }); + + it("does not mutate the caller's labels object", () => { + const map = new LabelMap(['a']); + const labels = { a: 3 }; + + map.set(labels, 1); + + expect(labels).toStrictEqual({ a: 3 }); + }); + + it("does not keep a reference to the caller's labels object", () => { + const map = new LabelMap(['a']); + const labels = { a: 'x' }; + + map.set(labels, 1); + labels.a = 'mutated afterwards'; + + expect(map.entry({ a: 'x' }).labels).toStrictEqual({ a: 'x' }); + }); + + it('looks up with either representation', () => { + const map = new LabelMap(['a']); + + map.set({ a: 3 }, 7); + + expect(map.get({ a: 3 })).toEqual(7); + expect(map.get({ a: '3' })).toEqual(7); + }); + + it('leaves nullish values untouched so stored labels round-trip', () => { + const map = new LabelMap(['a', 'b']); + + map.set({ a: null, b: 3 }, 7); + + // keyFrom() treats nullish as absent; coercing null to 'null' would + // make the stored labels compute a different key than the one the + // entry is stored under, breaking remove(entry.labels) round-trips. + expect(map.get({ a: null, b: 3 })).toEqual(7); + const [entry] = Array.from(map.values()); + expect(entry.labels).toStrictEqual({ a: null, b: '3' }); + + map.remove(entry.labels); + expect(map.size).toEqual(0); + }); + + it('leaves undefined values untouched as well', () => { + const map = new LabelMap(['a', 'b']); + + map.set({ a: undefined, b: 3 }, 7); + + const [entry] = Array.from(map.values()); + expect(entry.labels).toStrictEqual({ a: undefined, b: '3' }); + + // keyFrom() treats an explicit undefined as absent, so the two + // spellings have to stay the same series. + expect(map.get({ b: 3 })).toEqual(7); + expect(map.size).toEqual(1); + + map.remove(entry.labels); + expect(map.size).toEqual(0); + }); + + it('keeps a `__proto__` label as an own property', () => { + const map = new LabelMap(['__proto__']); + // A literal would set the prototype instead of defining a property. + const labels = JSON.parse('{"__proto__":3}'); + + map.set(labels, 1); + + const [entry] = Array.from(map.values()); + expect(Object.hasOwn(entry.labels, '__proto__')).toBe(true); + expect(entry.labels.__proto__).toEqual('3'); + expect(map.get(labels)).toEqual(1); + + map.remove(entry.labels); + expect(map.size).toEqual(0); + }); + + it('keeps a `__proto__` label that is only inherited', () => { + const map = new LabelMap(['__proto__']); + const proto = JSON.parse('{"__proto__":"a"}'); + const labels = Object.create(proto); + + map.set(labels, 1); + + const [entry] = Array.from(map.values()); + expect(map.get(entry.labels)).toEqual(1); + + map.remove(entry.labels); + expect(map.size).toEqual(0); + }); + + it('keeps inherited labels, which keyFrom() reads', () => { + const map = new LabelMap(['a']); + // keyFrom() reads labels[name], so it sees the prototype chain; + // an own-properties-only copy could not reproduce its own key. + const labels = Object.create({ a: 'x' }); + + map.set(labels, 1); + + const [entry] = Array.from(map.values()); + expect(map.get(entry.labels)).toEqual(1); + + map.remove(entry.labels); + expect(map.size).toEqual(0); + }); + + it('coerces inherited labels too', () => { + const map = new LabelMap(['a']); + const labels = Object.create({ a: 3 }); + + map.set(labels, 1); + + const [entry] = Array.from(map.values()); + expect(entry.labels).toStrictEqual({ a: '3' }); + }); + + it('keeps null and the string "null" distinct', () => { + const map = new LabelMap(['a']); + + map.set({ a: null }, 1); + map.set({ a: 'null' }, 2); + + expect(map.size).toEqual(2); + }); + + it('normalizes labels for entries created by getOrAdd()', () => { + const map = new LabelMap(['a']); + + map.getOrAdd({ a: 3 }, () => 1); + + expect(Array.from(map.values())).toStrictEqual([ + { value: 1, labels: { a: '3' } }, + ]); + }); + + it('keeps normalized labels across merge() updates', () => { + const map = new LabelMap(['a']); + + map.merge({ a: 3 }, { count: 1 }); + map.merge({ a: 3 }, { count: 2 }); + + expect(Array.from(map.values())).toStrictEqual([ + { count: 2, labels: { a: '3' } }, ]); }); });