Fix escaping for non-string label values - #793
Closed
happykawayigt wants to merge 1 commit into
Closed
Conversation
Signed-off-by: Guan Tong <255313991+happykawayigt@users.noreply.github.com>
Contributor
|
This solution has the same performance regression as #792 and I now have two competing implementations neither of which I'm willing to accept. I made a counterproposal in the ticket #791, but that suggestion is a bit more technically challenging. It does on the other hand offer a chance at a perf tag in the changelog. |
milcho0604
added a commit
to milcho0604/client_js
that referenced
this pull request
Aug 2, 2026
…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>
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
2 participants
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.
Non-string label values are now converted to strings during exposition formatting before the existing backslash, newline, and quote escaping runs. This keeps the metric-recording path unchanged and preserves the existing fast path for string values.
A regression test covers an array label whose string representation contains a quote, newline, and backslash. The test fails against the previous implementation and passes with this change.
Validation:
npm test