Skip to content

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

Open
milcho0604 wants to merge 1 commit into
prometheus:mainfrom
milcho0604:fix/normalize-labels-in-labelmap
Open

fix: normalize label values once, when a label combination is first stored#798
milcho0604 wants to merge 1 commit into
prometheus:mainfrom
milcho0604:fix/normalize-labels-in-labelmap

Conversation

@milcho0604

Copy link
Copy Markdown
Contributor

Fixes #791, along the direction proposed there: pay the coercion cost once, when a new label combination is first stored, instead of on every metrics() call (#792/#793 were declined for adding cost to rendering, which is linear in total cardinality).

What

  • LabelMap gains a single insertion point (#insert) used by set, setDelta, getOrAdd and merge. It coerces label values with template interpolation — the same ToString the exposition applies — via a copy-on-write normalizeLabels() that returns all-string label sets untouched (no allocation in the common case).
  • Nullish values stay untouched. keyFrom() treats them as absent, so coercing them to "null"/"undefined" would make 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 worker serialization. Their rendered form contains nothing that needs escaping.
  • merge() keeps the stored (normalized) labels on update instead of overwriting them with the caller's raw object — the update path that could previously replace stored labels without going through insertion.
  • getOrAdd() passes the normalized labels to init(), so values that keep their own copy of the labels store the same normalized object. Summary needs this: its exported labels come from the stored value, not the map entry.
  • LabelGrouper deliberately does not normalize, per the discussion in Non-string label values bypass escaping and can produce malformed exposition #791: 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 — aggregate() keeps its current cost, byte-for-byte. I verified the assumption: worker payloads are exactly getMetricsAsJSON() output, where metric values carry stored (normalized) labels. The inputs that reach aggregation without normalization are unchanged from main: custom collector results, registry default labels (merged in raw by getMetricsAsJSON()), and the synthesized le/quantile labels, which are attached numerically at export time — all reserved or user-controlled values that pass through exactly as today (promtool accepts the aggregated output, see below). On Cluster fixes #789: its diff touches lib/cluster.js/lib/worker.js lifecycle only — payloads are still getMetricsAsJSON() output fed to Registry.aggregate(), so the two changes stay orthogonal.

With the stores normalized, the existing render-time escaping (which already handles strings correctly) produces well-formed exposition for the #791 reproduction:

g{x="say \"hi\""} 1

Cost

The recording path for existing combinations is unchanged — lookup only, no new code. First insertion of a combination pays one scan of its labels, plus a copy and coercion when something is non-string (getOrAdd scans once more inside #insert; normalization is idempotent). Interleaved 5-round medians (Node 25, arm64, lower is better; ranges in parentheses):

path main this PR
hot: inc() on existing combination × 5M 135.2 ms (135.0–137.9) 132.4 ms (131.9–137.4) — within noise
metrics() with 10k series × 50 199.8 ms (195.7–210.6) 199.1 ms (184.2–202.0) — within noise
300k first insertions 48.3 ms (47.7–49.2) 51.0 ms (50.2–53.1) — +5.6%, ~9 ns per new combination

aggregate() is untouched — no code change on that path.

Benchmark script (run against two checkouts, interleaved)
'use strict';
// usage: node bench.js <prom-client dir> <hot|insert|render>
const path = process.argv[2];
const which = process.argv[3];
const client = require(require('node:path').resolve(path, 'index.js'));

function hrms(fn) {
	const t0 = process.hrtime.bigint();
	fn();
	return Number(process.hrtime.bigint() - t0) / 1e6;
}

if (which === 'hot') {
	const r = new client.Registry();
	const c = new client.Counter({ name: 'c_total', help: 'h', labelNames: ['x'], registers: [r] });
	c.inc({ x: 'warm' }, 1);
	const N = 5_000_000;
	console.log(hrms(() => {
		for (let i = 0; i < N; i++) c.inc({ x: 'warm' }, 1);
	}).toFixed(1));
} else if (which === 'insert') {
	const { LabelMap } = require(require('node:path').resolve(path, 'lib/util.js'));
	const N = 300_000;
	const maps = [];
	for (let i = 0; i < 10; i++) maps.push(new LabelMap(['a', 'b']));
	console.log(hrms(() => {
		for (const m of maps) {
			for (let i = 0; i < N / 10; i++) m.set({ a: `v${i}`, b: 'k' }, i);
		}
	}).toFixed(1));
} else if (which === 'render') {
	const r = new client.Registry();
	const c = new client.Counter({ name: 'c_total', help: 'h', labelNames: ['x'], registers: [r] });
	for (let i = 0; i < 10_000; i++) c.inc({ x: `v${i}` }, 1);
	(async () => {
		await r.metrics();
		const t0 = process.hrtime.bigint();
		for (let i = 0; i < 50; i++) await r.metrics();
		console.log((Number(process.hrtime.bigint() - t0) / 1e6).toFixed(1));
	})();
}

Observable changes

  • Label values that pass through the built-in stores are reported as strings (3 becomes "3") wherever stored labels surface: getMetricsAsJSON(), each metric's public get(), worker payloads, and — transitively — aggregation output. Registry.aggregate() itself passes labels through untouched. Noted in the changelog.
  • No index.d.ts change: label output types are already string | number, and le/quantile/default/custom-collector labels can still be numeric.
  • keyFrom() already coerces ordinary non-nullish values while building keys, so get()/set() lookups accept either representation — pinned by new tests, including null vs 'null' staying distinct and remove(entry.labels) round-tripping.

Scope

Covered: everything that goes through the built-in metric stores (Counter, Gauge, Histogram, Summary); cluster aggregation benefits transitively because worker payloads carry stored labels. Not covered (unchanged, documented): custom collector results rendered directly from get(), registry default labels, and exemplar labels — none of these pass through the stores. Relocating the quote/backslash/newline escaping itself into the stores is deliberately left out: rendering currently escapes backslashes, so it would have to move atomically across every label source or double-escape; I can scope and benchmark that separately.

Test

  • New label normalization unit block for LabelMap: coercion at insertion, caller's object never mutated, all-string sets stored without copying, lookups by either representation, nullish round-trips (remove(entry.labels) works; null vs 'null' stay distinct), getOrAdd init receiving normalized labels, merge keeping normalized labels across updates. LabelGrouper tests pin the pass-through behaviour (raw labels preserved).
  • End-to-end regressions through real metrics (both content types): Gauge.set with array/number labels renders escaped; Summary.observe covers the stored-value labels path.
  • Existing expectations that pinned numeric stored labels updated to the normalized form (versionTest, defaultMetricsTest, utilTest); aggregation expectations stay raw, pinning the pass-through.
  • Applying the full test patch to main without the lib changes yields 23 failures; the full suite passes with them: 556/556, lint + prettier + tsc clean.
  • promtool check metrics accepts the rendered output (rc=0) for a registry mixing quote/newline/backslash/boolean/number labels across all four metric types, and for a simulated cluster round-trip (two workers' getMetricsAsJSON() through JSON serialization into AggregatorRegistry.aggregate()). The same direct-render check on main fails with the error from Non-string label values bypass escaping and can produce malformed exposition #791 (unexpected end of label value, rc=1).

Comment thread lib/util.js
Comment on lines +459 to +464
* 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.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@jdmarshall jdmarshall left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is pretty close to ready to go, except that the nested double loop is giving me some pause.

I can't land this yet though because we are working on a dot release to get a huge pile of existing changes out. But this should be able to make the 1.0 cutoff for sure.

Comment thread lib/util.js
…tored

Non-string label values bypassed escapeLabelValue() and could render
malformed exposition (prometheus#791): a value whose string form contains a quote
or a newline produced invalid output. Escaping them during metrics() was
rejected in prometheus#792/prometheus#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 prometheus#791

Signed-off-by: Changhyun Kim <milcho0604@gmail.com>
@milcho0604
milcho0604 force-pushed the fix/normalize-labels-in-labelmap branch from 1510af3 to de2b362 Compare August 4, 2026 03:06
This was referenced Aug 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Non-string label values bypass escaping and can produce malformed exposition

2 participants