Fix escaping for non-string label values

[Hashim1999164]

Summary

• Non-string label values (for example arrays) skipped escapeLabelValue and were stringified only during template interpolation, so quotes and newlines could break Prometheus text format.
• Coerce with `${str}` before escapeString / quote escaping, matching how string labels are already handled.
• Adds a regression test for quote and newline cases from Non-string label values bypass escaping and can produce malformed exposition #791.

Fixes #791

Test plan

• npx jest test/registerTest.js -t "should escape"
• Confirm exposition with array label values passes promtool check metrics

[cjihrig]

I think this will still throw for Symbol values. I'm not sure if it's worth trying to handle those or not.

[Hashim1999164]

Updated escapeLabelValue to use String(value) so Symbol values no longer throw, and added a regression test. Also rewrote the commit with a DCO sign-off.

[Hashim1999164]

Updated to use String(value) so Symbol label values no longer throw, and added a regression test for that case. DCO sign-off is on the rewritten commit as well.

[jdmarshall]

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.

[jdmarshall]

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.

[Hashim1999164]

Thanks for the benchmark notes. I updated escapeLabelValue to keep the string fast path and only call String() for non-string values (for example Symbols), and added a CHANGELOG entry.

[jdmarshall]

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?