search

rabbitmq / rabbitmq-server

Open source RabbitMQ: core server and tier 1 (built-in) plugins
https://www.rabbitmq.com/
Other
11.85k stars 3.9k forks source link

[Prometheus] Reduce number of series for per-exchange/per-queue metrics #11217

Open patagonaa opened 1 month ago

patagonaa commented 1 month ago

Is your feature request related to a problem? Please describe.

As of now, a lot of useful per-exchange/per-queue-metrics (for example the number of unroutable messages, number of delivered messages, etc.) are only included in the channel_exchange_metrics/channel_queue_metrics/channel_queue_exchange_metrics, which can't be easily used with Prometheus because of their high cardinality.

Practically, all per-channel-metrics (including channel_exchange_metrics and channel_queue_metrics and channel_queue_exchange_metrics) can't be used with Prometheus, as they include the channel ID as a label value which is something Prometheus explicitly warns against, because it causes a lot of active time series, which are, by default, limited to around 200.000 and causes high memory usage.

Combined with the lack of aggregation features in metrics_relabel_config in Prometheus, this forces users to disable useful metrics like messages delivered per queue (as included in channel_queue_metrics), messages published/confirmed/unroutable per exchange (as included in channel_exchange_metrics) or messages published from exchange to queue (as included in channel_queue_exchange_metrics).

Describe the solution you'd like

Ideally, there would be groups group for queue, exchange and queue-exchange metrics, not including the channel id as a label value. Queues even have the group queue_metrics already, which, however, does not include metrics like the number of delivered/acked messages.

These new groups should just expose the (relevant) metrics of channel_exchange_metrics, channel_queue_metrics and channel_queue_exchange_metrics but aggregated only by queue/exchange instead of queue+channel/exchange+channel).

Describe alternatives you've considered

No response

Additional context

No response

mkuratczyk commented 1 month ago

We discuss this every now and then and overall I think we agree - we expose too many metrics and the high-cardinality metrics shouldn't be there at all probably. As always, it's a matter of time and effort required to clean this up. Perhaps you'd be interested in contributing this? As a start, I'd say the exact list of changes would be nice. Your overall description is good, but I'm thinking more about a TODO list:

  1. metric a_b_c should not have label L
  2. metric x_y_z should be removed altogether
  3. ...

This would allow us to discuss individual changes if needed and would guide the implmentation of these changes.

patagonaa commented 1 month ago

As far as I'm concerned, the only metric label that causes issues here is channel, so all metrics that only have this label should be removed (I've verified that all of them are already available aggregated in /metrics), all metrics that also have other labels should have this label removed (so they're grouped by exchange/queue).

Alternatively, metrics that make sense without this label (see below) could be exposed under another name without the channel label, keeping the existing ones, however I don't really see a use case where it makes sense to look at per-channel metrics (as the channel id is basically meaningless as it is).

In some cases (like people creating temporary queues with UUIDs in the name or something) other metrics could cause high cardinality as well, but in that case, those metrics can be easily dropped in Prometheus using metric_relabel_configs.

(Documentation for reference: https://www.rabbitmq.com/docs/prometheus#detailed-endpoint) Listing each metric individually doesn't make sense as it always affects entire groups of metrics:

Metrics including channel that make sense without it (channel should be removed):

Metrics including channel that don't make sense without it (can be removed altogether as available aggregated through /metrics):

mkuratczyk commented 1 month ago

Thanks. As I said - I think it's a valid request and it concurs our own experience. Therefore, I will leave this open but note that we are not actively working on this at this time. Contributions would be very much appreciated. It's mostly about deleting stuff, so shouldn't be too hard. :)

patagonaa commented 1 month ago

If I was an Erlang dev I'd be happy to contribute code, but getting used to Erlang and setting up a dev environment to test the changes is too much for me right now I think.

Maybe I'll get annoyed enough at the lack of metrics I'll take a look at it some day 😅

michaelklishin commented 1 month ago

Coming up with a list of specific actionable changes (like in https://github.com/rabbitmq/rabbitmq-server/issues/11217#issuecomment-2107254213 but I assume the list is not necessarily complete) is considered to be a partial contribution by our team.

Those seven is a good starting point. Well, at least the first four, I am less certain about removing entire metrics as it is always particularly hard to remove features/data. But for 4.x, that's certainly a good idea.

patagonaa commented 1 month ago

My suggestion to make this a non-breaking change would be to add these metrics under new names (as all of them except rabbitmq_detailed_queue_messages_published_total contain channel in the metric name, which doesn't make sense if it's not channel-specific anymore) and group them in new categories:

this doesn't cause name collisions when exporting all metrics and in the 4.x release, all per-channel metrics could be removed.

LoisSotoLopez commented 1 month ago

I can take care of this issue. I'll be waiting to hear core dev team opinion on whether the strategy suggested on #11217 (comment) should be taken.

mkuratczyk commented 1 month ago

It sounds like a good approach to me