From ea8872db9520bbebf756d0c49c6346f55dfb03ad 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). 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 --- CHANGELOG.md | 2 + lib/summary.js | 29 ++-- lib/util.js | 66 ++++++++-- test/defaultMetricsTest.js | 3 +- test/metrics/versionTest.js | 7 +- test/registerTest.js | 45 ++++++- test/summaryTest.js | 22 ++++ test/utilTest.js | 254 ++++++++++++++++++++++++++++++++++-- 8 files changed, 390 insertions(+), 38 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index beb6faa1..4af58a4a 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/lib/summary.js b/lib/summary.js index 94a14748..627e0246 100644 --- a/lib/summary.js +++ b/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++; diff --git a/lib/util.js b/lib/util.js index 96dc0eec..7db64073 100644 --- a/lib/util.js +++ b/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,7 +320,7 @@ 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) { @@ -278,8 +328,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, { 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. */ diff --git a/test/defaultMetricsTest.js b/test/defaultMetricsTest.js index 00ad8c25..165b6895 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 9e0810bb..b9cfcdd6 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('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', diff --git a/test/summaryTest.js b/test/summaryTest.js index 30cfe1cb..8f4d17f3 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 11f5ceef..54ce7d1f 100644 --- a/test/utilTest.js +++ b/test/utilTest.js @@ -114,7 +114,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 3, labels: { a: 2 } }, + { value: 3, labels: { a: '2' } }, ]); }); @@ -126,7 +126,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 4, labels: { a: 2 } }, + { value: 4, labels: { a: '2' } }, ]); }); @@ -139,11 +139,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' }, }, ]); }); @@ -157,7 +157,7 @@ describe('utils', () => { expect(map.size).toEqual(1); expect(Array.from(map.values())).toStrictEqual([ - { value: 3, labels: { a: 2 } }, + { value: 3, labels: { a: '2' } }, ]); }); @@ -168,7 +168,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' } }, ]); }); @@ -180,8 +180,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' } }, ]); }); }); @@ -216,7 +216,7 @@ describe('utils', () => { expect(map.entry({ b: 22 })).toStrictEqual({ value: 10, - labels: { b: 22 }, + labels: { b: '22' }, }); }); }); @@ -282,11 +282,83 @@ 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(); }); + + it('hands the stored labels to init', () => { + const map = new LabelMap(['c']); + let seen; + + map.getOrAdd({ c: 200 }, labels => { + seen = labels; + return 4; + }); + + expect(seen).toStrictEqual({ c: '200' }); + expect(seen).toBe(map.entry({ c: 200 }).labels); + }); + + it.each([ + ['null prototype', () => Object.create(null)], + ['a chain ending in null', () => Object.create(Object.create(null))], + ])('keeps the source prototype for %s', (_name, make) => { + // keyFrom() reads declared names by property access. An ordinary copy + // would answer 'constructor' with Object.prototype's member and compute + // a different key than the entry is filed under, so remove() would miss. + const map = new LabelMap(['region', 'constructor']); + const labels = make(); + labels.region = 'eu'; + + map.set(labels, 1); + const stored = map.entry(labels).labels; + + expect(Object.getPrototypeOf(stored)).toBe( + Object.getPrototypeOf(labels), + ); + expect(stored.constructor).toBeUndefined(); + expect(map.keyFrom(stored)).toEqual(map.keyFrom(labels)); + + map.remove(stored); + expect(Array.from(map.values())).toStrictEqual([]); + }); + + it('does not let a __proto__ label replace the copy prototype', () => { + // Object.assign() would write through the __proto__ setter here. + class Labels {} + const map = new LabelMap(['__proto__', 'region']); + const labels = Object.create(Labels.prototype); + Object.defineProperty(labels, '__proto__', { + value: { poisoned: 'yes' }, + enumerable: true, + writable: true, + configurable: true, + }); + labels.region = 'eu'; + + map.set(labels, 1); + const stored = map.entry(labels).labels; + + expect(Object.getPrototypeOf(stored)).toBe(Labels.prototype); + expect(stored.poisoned).toBeUndefined(); + // Plain assignment would reach the setter, which drops a string. + expect(Object.hasOwn(stored, '__proto__')).toBe(true); + expect(stored.__proto__).toEqual('[object Object]'); + }); + + it('leaves the map untouched when init throws', () => { + const map = new LabelMap(['c']); + + expect(() => + map.getOrAdd({ c: 200 }, () => { + throw new Error('nope'); + }), + ).toThrow('nope'); + expect(Array.from(map.values())).toStrictEqual([]); + expect(map.getOrAdd({ c: 200 }, () => 4)).toStrictEqual(4); + }); }); describe('clear()', () => { @@ -308,7 +380,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' } }, ]); }); });