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' } }, ]); }); });