fix: normalize label values once, when a label combination is first stored - #798
Open
milcho0604 wants to merge 1 commit into
Open
fix: normalize label values once, when a label combination is first stored#798milcho0604 wants to merge 1 commit into
milcho0604 wants to merge 1 commit into
Conversation
jdmarshall
reviewed
Aug 2, 2026
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. |
jdmarshall
requested changes
Aug 2, 2026
jdmarshall
left a comment
Contributor
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.
milcho0604
force-pushed
the
fix/normalize-labels-in-labelmap
branch
from
August 2, 2026 09:50
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>
milcho0604
force-pushed
the
fix/normalize-labels-in-labelmap
branch
from
August 4, 2026 03:06
1510af3 to
de2b362
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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).