Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
patagonaa opened this issue May 13, 2024 · 9 comments
Open

Comments

@patagonaa
Copy link

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
Copy link
Contributor

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
Copy link
Author

patagonaa commented May 13, 2024

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):

  • everything grouped under channel_exchange_metrics
  • everything grouped under channel_queue_metrics
  • everything grouped under channel_queue_exchange_metrics

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

  • everything grouped under connection_coarse_metrics
  • everything grouped under connection_metrics
  • everything grouped under channel_metrics
  • everything grouped under channel_process_metrics

@mkuratczyk
Copy link
Contributor

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
Copy link
Author

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
Copy link
Member

Coming up with a list of specific actionable changes (like in #11217 (comment) 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
Copy link
Author

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:

  • group queue_metrics (already exists)
    • rabbitmq_detailed_queue_get_ack_total
    • rabbitmq_detailed_queue_get_total
    • rabbitmq_detailed_queue_messages_delivered_ack_total
    • rabbitmq_detailed_queue_messages_delivered_total
    • rabbitmq_detailed_queue_messages_redelivered_total
    • rabbitmq_detailed_queue_messages_acked_total
    • rabbitmq_detailed_queue_get_empty_total
  • group exchange_metrics (new)
    • rabbitmq_detailed_exchange_messages_published_total
    • rabbitmq_detailed_exchange_messages_confirmed_total
    • rabbitmq_detailed_exchange_messages_unroutable_returned_total
    • rabbitmq_detailed_exchange_messages_unroutable_dropped_total
  • group queue_exchange_metrics (new)
    • rabbitmq_detailed_queue_exchange_messages_published_total

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

@LoisSotoLopez
Copy link
Contributor

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
Copy link
Contributor

It sounds like a good approach to me

@michaelklishin
Copy link
Member

#11559 needs another around: the tests were failing often and were not excluded from mixed version runs that they cannot pass (with the assertions they have against a 3.13.x node).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants