Enable "rabbitmq_prometheus" in RabbitMQ 3.8 by default

[tianon]

Closes #419

[tianon]

Arg, we're butting up against:

rabbitmq/docker-entrypoint.sh

Lines 225 to 228 in e5da520

	if [ -z "$shouldWriteConfig" ] && [ ! -f "$oldConfigFile" ] && [ ! -f "$newConfigFile" ]; then
	# no config files, we should write one
	shouldWriteConfig=1
	fi

I really don't like this script... 🤦

[gerhard]

I really don't like this script... 🤦

I couldn't agree more.

I would definitely advocate for its removal and reduce the sprawl of environment variables by requiring users to configure most aspects of RabbitMQ using the sysctl config format. This has built-in validation and many improvements over env vars or the classic config: https://www.rabbitmq.com/configure.html#config-file

A relevant improvement that shipped in 3.8.4 is improved support for multiple sysctl config files: rabbitmq/rabbitmq-server#2258 & rabbitmq/rabbitmq-server#2277 . This allows users to specify multiple config files in sysctl format and not mutate a single file.

There has never been a better time to put an end to the docker-entrypoint,.sh madness!

---

As extra context, this is exactly what we are doing in the commercial version of RabbitMQ, which ships as an OCI.

[gerhard]

All changes look good to me. The docker-entrypoint.sh is even more complex than it was before, and my previous comment stills stands, but I don't think that we should leave that distract from this.

Let me know if you would like me to QA when the checks are green and it's ready to merge.

This is a fantastic change, I am very excited to see it take shape so quickly. It's great to see you move so fast on this @tianon 👍🏻

[tianon]

The only reason I'm touching docker-entrypoint.sh here is for:

SSL will need to be configured for rabbitmq-prometheus on top of rabbitmq-management

... and that's also why it only supports the SSL-related flags that are supported for the server and for the management plugin.

If you think we don't need that (and that if users configure the server to use SSL, we can leave it up to them to configure SSL for prometheus), we can drop all but the change to disable_metrics_collector.

[tianon]

We could also skip the docker-entrypoint.sh changes here and add a deprecation warning to docker-entrypoint.sh separately so that users who would be affected by the removal of it know that we intend to remove it / delete most of it at some point.

[tianon]

Also, I guess moving disable_metrics_collector to use a conf.d file instead makes it both easier to add/remove and makes this script unaware of it, so that's probably a better solution to what I pushed in https://github.com/docker-library/rabbitmq/compare/6bda400327dce532bacff769ea8b908125c7515d..e2cf7d1bbc5825230b5bcc79725816fc38bb55cf.

[nesc58]

How can I configure return_per_object_metrics configurable? For me it's a need to configure it.

Thank you for initiating this PR 👍

[tianon]

@nesc58 I honestly find joy in the fact that you're asking, because that very variety of question is why I hesitated even adding the configuration bits I did here 😄 -- the end goal is to not provide any custom configuration behavior anymore, so the direct answer to your question is that it should be put in /etc/rabbitmq/rabbitmq.conf or /etc/rabbitmq/conf.d/foo.conf (as per standard upstream RabbitMQ configuration) 👍

[gerhard]

@nesc58: How can I configure return_per_object_metrics configurable? For me it's a need to configure it.

For Docker Compose, I would recommend mounting the config file at /etc/rabbitmq/conf.d/prometheus.return_per_object_metrics.conf. The contents of the file would be:

prometheus.return_per_object_metrics = true

For Docker Swarm, I would recommend using a service config to generate that config file.

For K8S, I would recommend a ConfigMap to generate that config file.

[nesc58]

For Docker Compose, I would recommend mounting the config file at /etc/rabbitmq/conf.d/prometheus.return_per_object_metrics.conf. The contents of the file would be:

Thank you. That was what I am looking for :-)

[michaelklishin]

What's blocking this from being merged @tianon @gerhard?

[tianon]

I think just your eyeballs to ensure my final (simplified) version is acceptable as a default. 😅

[gerhard]

Almost there, will keep an eye on this PR until it makes it through 👀

[gerhard]

rabbitmq:3.8 & rabbitmq:3.8-management image variants work as expected, OK to merge 👍

[jwillmer]

@tianon I'm using the script below as entrypoint in my docker-compose file via: entrypoint: ["bash", "/scripts/cluster-entrypoint.sh" ]. How would I need to change it in order to support new versions of rabbitmq?

#!/bin/bash

set -e

# Start RMQ from entry point.
# This will ensure that environment variables passed
# will be honored
/usr/local/bin/docker-entrypoint.sh rabbitmq-server -detached

# Do the cluster dance
rabbitmqctl stop_app
rabbitmqctl join_cluster rabbit@rabbitmq1

# Stop the entire RMQ server. This is done so that we
# can attach to it again, but without the -detached flag
# making it run in the forground
rabbitmqctl stop

# Wait a while for the app to really stop
sleep 2s

# Start it
rabbitmq-server