Fix escaping for non-string label values - #792
Conversation
| return str; | ||
| } | ||
| return escapeString(str).replace(/"/g, '\\"'); | ||
| return escapeString(`${str}`).replace(/"/g, '\\"'); |
There was a problem hiding this comment.
I think this will still throw for Symbol values. I'm not sure if it's worth trying to handle those or not.
|
Updated |
ab68655 to
dcd68f6
Compare
Coerce label values with String() before escape so quotes and newlines in array (and other non-string) values cannot break Prometheus text format, and Symbol values do not throw. Signed-off-by: Hashim Khan <sardarhashim30@gmail.com>
dcd68f6 to
e9a077c
Compare
|
Updated to use |
There was a problem hiding this comment.
Here the bill comes due.
If you want a sanity check done here in the code, you're paying for it by a 1/6 to 1/4 increase in metrics processing time. Every collection, every minute, of every hour of every day, for a problem that should have been corrected at PR time.
For some users, especially power users, this function is on the critical path in a prometheus-enabled app, because we do the most damage to the p95 response times of the host application every time the metrics collection kicks in. Heavy math being done periodically in a language not meant for heavy math.
Performance Regressions:
------------------------
registry ⇒ metrics() 1 x 64
⇒ @prometheus/client@latest ▏████████████████████▕ 1,670 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏███████████████████▌▕ 1,643 op/s | 41 samples | (0.98x slower)
⇒ @prometheus/client@current ▏████████████████────▕ 1,351 op/s | 41 samples | (0.81x slower)
registry ⇒ metrics() 1 x 64 and openMetrics
⇒ @prometheus/client@latest ▏███████████████████▌▕ 1,638 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏████████████████████▕ 1,651 op/s | 40 samples | (1.01x faster)
⇒ @prometheus/client@current ▏████████████████────▕ 1,342 op/s | 41 samples | (0.82x slower)
registry ⇒ metrics() 2 x 4
⇒ @prometheus/client@latest ▏████████████████████▕ 6,216 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏███████████████████▌▕ 6,125 op/s | 41 samples | (0.99x slower)
⇒ @prometheus/client@current ▏███████████████─────▕ 4,738 op/s | 40 samples | (0.76x slower)
registry ⇒ metrics() 2 x 4 and openMetrics
⇒ @prometheus/client@latest ▏███████████████████▌▕ 6,192 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏████████████████████▕ 6,257 op/s | 41 samples | (1.01x faster)
⇒ @prometheus/client@current ▏███████████████─────▕ 4,765 op/s | 41 samples | (0.77x slower)
registry ⇒ metrics() 2 x 8
⇒ @prometheus/client@latest ▏████████████████████▕ 1,788 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏███████████████████▌▕ 1,779 op/s | 41 samples | (0.99x slower)
⇒ @prometheus/client@current ▏████████████████────▕ 1,433 op/s | 41 samples | (0.80x slower)
registry ⇒ metrics() 6 x 2
⇒ @prometheus/client@latest ▏███████████████████▌▕ 1,400 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏████████████████████▕ 1,415 op/s | 41 samples | (1.01x faster)
⇒ @prometheus/client@current ▏████████████████────▕ 1,163 op/s | 41 samples | (0.83x slower)
registry ⇒ metrics() 6 x 2 and openMetrics
⇒ @prometheus/client@latest ▏███████████████████▌▕ 1,391 op/s | 41 samples | (baseline)
⇒ @prometheus/client@trunk ▏████████████████████▕ 1,402 op/s | 41 samples | (1.01x faster)
⇒ @prometheus/client@current ▏████████████████────▕ 1,144 op/s | 41 samples | (0.82x slower)
So, I will not be landing this PR. This is a problem to be solved during bootstrapping, not during the hot, periodic path in the code.
That probably means fixing the problem in the Metrics constructor, or perhaps the LabelMap. Not here. Never here.
|
It is bugs like the one this PR attempts to fix that are the reason why I made the benchmarks an active part of the PR process and reproduced the library we used to use to run them. There are obvious solutions to many bugs that have a cost that everyone pays every time, for a problem 5% of users encounter. That tax is better discussed before merge rather than after. |
Coerce non-string label values with String() only when needed, and document the fix in CHANGELOG. Signed-off-by: Hashim Khan <sardarhashim30@gmail.com>
|
Thanks for the benchmark notes. I updated |
|
I'm certain that String(str) already has this exact same guard. The benchmarks did not improve. The author of the original ticket is making noises about tackling the deeper problem. You might want to coordinate with him? |
…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>
|
Thanks for the follow-up. Agreed that a typeof guard before String() does not buy much once the engine already specializes String on strings, and the benchmarks make that clear. I am fine closing this PR if you would rather wait for the deeper fix from the original issue author. Happy to coordinate or reshape this if there is a narrower change you still want here. |
3 participants
Summary
escapeLabelValueand were stringified only during template interpolation, so quotes and newlines could break Prometheus text format.`${str}`beforeescapeString/ quote escaping, matching how string labels are already handled.Fixes #791
Test plan
npx jest test/registerTest.js -t "should escape"promtool check metrics