diff --git a/CHANGELOG.md b/CHANGELOG.md index de63ceba..ac5a8d3d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,8 @@ project adheres to [Semantic Versioning](http://semver.org/). - perf: Remove truthy conditionals from default metric collectors - fix: Preserve zero-valued Counter exemplars - perf: Remove object and array fallback truthiness from core metric paths +- 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 (Fixes [#838](https://github.com/prometheus/client_js/issues/838)) ### Added diff --git a/lib/summary.js b/lib/summary.js index 3e0754ab..87d18003 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 69f12d52..cec6e1e7 100644 --- a/lib/util.js +++ b/lib/util.js @@ -202,6 +202,46 @@ exports.waitFor = async function waitFor(promise, limit = 5_000) { * @property labels {object} */ +/** + * @callback InitEntry + * @param {object} storedLabels the labels the store now owns + * @returns {*} the value to store + */ + +/** + * 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 +256,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 +285,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 +304,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 +326,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 {InitEntry} [init] receives the stored labels, returns an empty record * @returns {*} the existing value or the result of init() */ getOrAdd(labels, init) { @@ -278,8 +334,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 +362,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 +488,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' } }, ]); }); });