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 1 commit 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
1510af3 to
de2b362
Compare
| * 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?
58fcc1e to
8d2c8d2
Compare
| values.push(...extractSummariesForExport(s, this.percentiles)); | ||
| values.push(getSumForExport(s, this)); | ||
| values.push(getCountForExport(s, this)); | ||
| const labels = entry.labels; |
There was a problem hiding this comment.
So are we not encoding the label values in the individual store entries? I would have figured we did not need to pass these down from above.
1aed31c to
17aa3a3
Compare
…tored Non-string label values bypassed escapeLabelValue() and could render malformed exposition (prometheus#791). Escaping during metrics() costs linear in total cardinality (prometheus#792/prometheus#793 were declined for that), so values are coerced at the storage boundary instead, once per new combination. - LabelMap gains one insertion point, #insert, used by set, setDelta, getOrAdd and merge. The entry gets a copy the store owns, coerced with the same ToString the exposition applies, so a caller mutating its object afterwards cannot change what a stored series reports. - normalizeLabels() walks with for...in, because keyFrom() reads inherited enumerable labels too. Labels that no enumeration reaches, or whose prototype chain intercepts writes, are unsupported. The copy keeps the source prototype, so keyFrom() answers absent declared names the way the source did. Nullish values are copied as-is, and __proto__ needs Object.defineProperty rather than assignment. - merge() keeps the stored labels instead of overwriting them with the caller's object. getOrAdd() hands the stored labels to init(), so Summary's value holds that same object rather than the caller's, and its export helpers are unchanged from main. LabelGrouper does not normalize; the store-backed labels it receives are normalized already. Benchmarks and the observable output changes are in the PR description. Fixes prometheus#791 Signed-off-by: Changhyun Kim <milcho0604@gmail.com>
17aa3a3 to
ea8872d
Compare
|
You were right, the extra parameters were not needed. |
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 — vianormalizeLabels(), which always returns a copy the store owns.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).