Escape prometheus core metric label values

[gomoripeti]

Proposed Changes

For example special characters like double quotes are allowed in queue
names, in which case detailed metrics could produce unparsable text
format output.

Fixes #9648

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

• Bug fix (non-breaking change which fixes issue #NNNN)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause an observable behavior change in existing systems)
• Documentation improvements (corrections, new content, etc)
• Cosmetic change (whitespace, formatting, etc)
• Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

• I have read the CONTRIBUTING.md document
• I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
• I have added tests that prove my fix is effective or that my feature works
• All tests pass locally with my changes
• If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
• If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

• I would like to get help on the test case. Although {collect_statistics, fine} is set in init_per_group still there are no non-coarse metrics, for example the table channel_exchange_metrics is empty. I must be missing something?

• adding escaping probably will have a slight performance impact. But I believe the main reason for using preformatted labels is to avoid the intermediate #LabelPair record representation, not to skip escaping.

[gomoripeti]

hm, the new test case consistently fails for me locally, but seems like it passed in the CI. Maybe I should start from a clean repo.

[michaelklishin]

@gomoripeti when in doubt, run

bazel clean
killall -9 erl; killall -9 epmd; killall -9 beam.smp

You may have stray nodes running in the background from the earlier (likely failing at first) runs.

[michaelklishin]

@tvhong-amazon can you please have a look, does this escape label values the way you'd expect? Thank you.

[gomoripeti]

I forgot to add the new test group to all() test groups, that is why CI tests did not fail previously.

I would like to get help on the test case. Although {collect_statistics, fine} is set in init_per_group still there are no non-coarse metrics, for example the table channel_exchange_metrics is empty. (this line fails https://github.com/rabbitmq/rabbitmq-server/pull/9656/files#diff-18cc5f075e5ea0bf5171dfb8bfb339bb021ed81bf0fa779909975cc080ce7d88R608) I must be missing something?

[tvhong-amazon]

Thank you for the quick turn around on this! The escaped labels look correct to me.

However, I see that there are other label methods that don't call escape_label_value such as:

label(L) when is_binary(L) ->

label(M) when is_map(M) ->

label({RemoteAddress, Username, Protocol}) when is_binary(RemoteAddress), is_binary(Username),
                                                is_atom(Protocol) ->

I think we should also escape Username as it can include special characters. As for the other functions and variables, are we positive that they don't need to be escaped?

[michaelklishin]

@tvhong-amazon agreed, since usernames can be generated in RabbitMQ-aaS environments. Connection names would be another obvious candidate.

[gomoripeti]

thank you for the review and feedback. I thoroughly reviewed the label function again

• label({RemoteAddress, Username, Protocol}) this clause returns a key-value list (not a preformatted binary) so it will be escaped by prometheus_text_format
• label(L) when is_binary(L) -> as a label should consist of a key and value (or a list of them) a single binary cannot be just a value, but it must be already preformatted. We don't want to double escape it. It corresponds to this clause: https://github.com/deadtrickster/prometheus.erl/blob/b78e17eda43c7fcb2c05f76c7dd80db969cd8479/src/formats/prometheus_text_format.erl#L188
• label(M) when is_map(M) -> the values are escaped in this PR
• pid and protocol types are safe, should have no special characters

So to sum up, I think all cases, where necessary, are escaped

[tvhong-amazon]

@gomoripeti Thank you for double checking.

What you point out here is quite interesting:

a key-value list (not a preformatted binary) so it will be escaped by prometheus_text_format

If this is the case, would it make sense to change all other methods to return a key-value list instead of a binary? That way, we don't need to duplicate the label escaping logic in RabbitMQ.

[gomoripeti]

Pre-rendering was an intentional performance optimisation in #3587

I just see this note from the PR which was a wrong assumption

Character escaping is not needed, as AMQP object names are quite restricted in what characters they can have.

But it is true if the escaping function would be exported by the prometheus lib, then we could avoid some code duplication. (I just wanted to quickly submit a fix, but that would be the proper way)

[SimonUnge]

Any reason you copied the functions instead of just calling the ones in prometheus_text_format? I think they are exported?

[gomoripeti]

unfortunately only exported when compiled for tests https://github.com/deadtrickster/prometheus.erl/blob/master/src/formats/prometheus_text_format.erl#L27

[michaelklishin]

You are welcome to contribute to prometheus.erl, it is maintained by a RabbitMQ core team member

[aaron-seo]

Was poking around, decided to create relevant PR in prometheus.erl to move that export out of the TEST def -- PR here in deadtrickster/prometheus.erl#158

[gomoripeti]

thank you! will update this PR when that is merged

[binarin]

The reasoning behind removing escaping was that via AMQP 0.9.1 it's not possible to create queue/exchange/... that needs escaping, and this was one of the worst performance offenders. So removing it seemed reasonable. You'll need to check that performance (and memory usage) is still acceptable with escaping restored - I suggest measuring the time it takes to generate stats on a broker with 10000 of exchanges/queues/channels each.

[gomoripeti]

thanks @binarin for the historic context. I assumed the intermediate record representation was the main concern.
Maybe escaping could be optimised to first just scan through the binary if it has any special chars, and only copy-and-escape if needed. This would avoid copying the binary in most cases. Will measure...

[gomoripeti]

I did some benchmarks of escaping. The baseline is RabbitMQ 3.12.6 unchanged. The "escape" version is using the unchanged escape_lable_value. The "escape-opt" is the version that is visible in deadtrickster/prometheus.erl#160.

I first did a quick microbenchmark of directly calling escape_label_value. Calling it 1M times with random strings of length 1..80 and no special characters took about 2 seconds. Calling the optimised version took about 0.2 seconds.

Then I made the following benchmark: Created 1 connection with 10K channels. Each creating 1 exchange, 1 queue, subscribed to the queue and published 1 message to the exchange. Queue and exchange names were in the form "q_<index>" ie 3-7 characters long.

Object counts:
Connections: 2
Channels: 10001
Exchanges: 10020
Queues: 10001
Consumers: 10001

Used below command to query prometheus metrics and measure duration:

$ curl -s -w 'Total: %{time_total}s\n' -u ${AUTH} https://${BROKER}/metrics/per-object -o metrics.log

Number of different labels in the output:

$ grep -c 'queue=' metrics.log; grep -c 'exchange=' metrics.log; grep -c 'vhost=' metrics.log; grep -c 'channel=' metrics.log
300029
50000
340029
200022

Measured 5 times the total time of the HTTP call as well as the function call prometheus_rabbitmq_core_metrics_collector:collect_mf/2

Results

collect_mf/2	avg (sec)	min (sec)	max (sec)	samples
baseline	2.0873628	1.819755	2.691796	2.691796,1.987629,1.819755,2.000737,1.936897
escape	4.2683968	3.174599	5.134498	5.134498,4.835701,4.117033,4.080153,3.174599
escape-opt	2.6438805999999997	2.103326	3.519962	3.519962,3.234765,2.167548,2.103326,2.193802

HTTP duration	samples
basline	2.887208s, 2.180847s, 1.981343s, 2.183184s, 2.081882s
escape	5.914040s, 5.069289s, 4.302007s, 4.246539s, 3.364625s
escape-opt	3.730437s, 3.402486s, 2.350659s, 2.315713s, 2.349219s

[deadtrickster]

tagged prometheus.erl 4.11

[gomoripeti]

thanks a lot, Iliia!

[gomoripeti]

this might have been forgotten (I did forget about it)

this PR is ready for review and merge.

[michaelklishin]

@tvhong-amazon @SimonUnge @illotum this is a PR that seeks to address an issue your team has reported. Can you please give it a shot?

[illotum]

I ran several (naive) benchmarks and manual tests. LGTM.

1000 bad queue names:

~/r/s/d/r/bin (prometheus_escape_label)> hyperfine -w 100 "curl localhost:15692/metrics/per-object"
Benchmark 1: curl localhost:15692/metrics/per-object
  Time (mean ± σ):      53.6 ms ±   5.7 ms    [User: 5.5 ms, System: 9.7 ms]
  Range (min … max):    47.7 ms …  72.4 ms    44 runs

~/r/s/d/r/bin (main)> hyperfine -w 100 "curl localhost:15692/metrics/per-object"
Benchmark 1: curl localhost:15692/metrics/per-object
  Time (mean ± σ):      50.9 ms ±   3.1 ms    [User: 5.3 ms, System: 8.8 ms]
  Range (min … max):    47.4 ms …  64.5 ms    44 runs