fix: normalize label values once, when a label combination is first stored - #798
fix: normalize label values once, when a label combination is first stored#798milcho0604 wants to merge 3 commits into
Conversation
| * 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. |
jdmarshall
left a comment
There was a problem hiding this comment.
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.
a8d4a88 to
1510af3
Compare
…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>
1510af3 to
de2b362
Compare
Signed-off-by: Jason Marshall <jdmarshall@users.noreply.github.com>
Signed-off-by: Changhyun Kim <milcho0604@gmail.com>
| // 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 |
There was a problem hiding this comment.
This is too much exposition. And restates the jsdoc unless I'm misreading.
| * 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. |
There was a problem hiding this comment.
This restates paragraph 1.
| * Symbols are not label data: `keyFrom()` iterates declared names, | ||
| * exposition uses `Object.entries()`; neither reads them. |
There was a problem hiding this comment.
Some of this would do better as the commit message, where I can still find it when I'm trying to figure out how to add additional functionality.
In fact maybe this whole section should be in the commit message.
| - Export `MetricObject`, `MetricObjectWithValues`, `MetricValue` and `MetricValueWithName` from the TypeScript definitions | ||
| - chore: Old label processing code marked as deprecated | ||
| - perf: Stop rebuilding the label array for every rendered series, and skip shared label handling entirely for values that do not set it; 9-22% faster `metrics()` in the registry benchmarks | ||
| - fix: Non-string label values (except `null`/`undefined`) are coerced to strings when a |
There was a problem hiding this comment.
Shorter, and you'll need to rebase and move these two the new section.
| return { | ||
| metricName: `${summary.name}_count`, | ||
| labels: value.labels, | ||
| labels, |
There was a problem hiding this comment.
If we are storing the normalized keys in the store in #insert() then these extra parameters should not be necessary, right?
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
LabelMapgains a single insertion point (#insert) used byset,setDelta,getOrAddandmerge. It coerces label values with template interpolation — the sameToStringthe exposition applies — via a copy-on-writenormalizeLabels()that returns all-string label sets untouched (no allocation in the common case).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 — breakingremove(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 toinit(), so values that keep their own copy of the labels store the same normalized object.Summaryneeds this: its exported labels come from the stored value, not the map entry.LabelGrouperdeliberately does not normalize, per the discussion in Non-string label values bypass escaping and can produce malformed exposition #791: aggregation input comes fromregistry.getMetricsAsJSON(), whose store-backed labels were already normalized on first insertion, so re-checking every value would taxaggregate()for work the stores already did —aggregate()keeps its current cost, byte-for-byte. I verified the assumption: worker payloads are exactlygetMetricsAsJSON()output, where metric values carry stored (normalized) labels. The inputs that reach aggregation without normalization are unchanged frommain: custom collector results, registry default labels (merged in raw bygetMetricsAsJSON()), and the synthesizedle/quantilelabels, which are attached numerically at export time — all reserved or user-controlled values that pass through exactly as today (promtoolaccepts the aggregated output, see below). On Cluster fixes #789: its diff toucheslib/cluster.js/lib/worker.jslifecycle only — payloads are stillgetMetricsAsJSON()output fed toRegistry.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:
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 (
getOrAddscans once more inside#insert; normalization is idempotent). Interleaved 5-round medians (Node 25, arm64, lower is better; ranges in parentheses):inc()on existing combination × 5Mmetrics()with 10k series × 50aggregate()is untouched — no code change on that path.Benchmark script (run against two checkouts, interleaved)
Observable changes
3becomes"3") wherever stored labels surface:getMetricsAsJSON(), each metric's publicget(), worker payloads, and — transitively — aggregation output.Registry.aggregate()itself passes labels through untouched. Noted in the changelog.index.d.tschange: label output types are alreadystring | number, andle/quantile/default/custom-collector labels can still be numeric.keyFrom()already coerces ordinary non-nullish values while building keys, soget()/set()lookups accept either representation — pinned by new tests, includingnullvs'null'staying distinct andremove(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 fromget(), 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
label normalizationunit block forLabelMap: 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;nullvs'null'stay distinct),getOrAddinit receiving normalized labels,mergekeeping normalized labels across updates.LabelGroupertests pin the pass-through behaviour (raw labels preserved).Gauge.setwith array/number labels renders escaped;Summary.observecovers the stored-value labels path.versionTest,defaultMetricsTest,utilTest); aggregation expectations stay raw, pinning the pass-through.mainwithout the lib changes yields 23 failures; the full suite passes with them: 556/556, lint + prettier + tsc clean.promtool check metricsaccepts 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 intoAggregatorRegistry.aggregate()). The same direct-render check onmainfails with the error from Non-string label values bypass escaping and can produce malformed exposition #791 (unexpected end of label value, rc=1).