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

Fix ETS mappings rabbitmq core collector #80

Merged
merged 4 commits into from
May 8, 2019
Merged

Fix ETS mappings rabbitmq core collector #80

merged 4 commits into from
May 8, 2019

Conversation

gerhard
Copy link
Contributor

@gerhard gerhard commented May 6, 2019

Fix metric mappings to ETS tables

In RabbitMQ v3.7.x, the following mappings were not valid and would
result in crashes when a produce/consumer would connect to RabbitMQ:

  • connection_metrics

    • connection_recv_count, recv_count
    • connection_send_count, send_count
  • channel_queue_metrics

    • channel_queue_get, get
    • channel_queue_get_no_ack, get_no_ack
    • channel_queue_deliver, deliver
    • channel_queue_deliver_no_ack, deliver_no_ack
    • channel_queue_redeliver, redeliver
    • channel_queue_ack, ack
    • channel_queue_get_empty, get_empty

The following metric was mapped incorrectly:

  • connection_churn_metrics
    • 7, queue_deleted

There is more line churn in this commit than necessary. I was
re-formatting all metrics as I was going over all mappings. This
confirms that I've checked that all mappings are accurate.

I've tested manually against current v3.7.x branch via make run-broker
as well as rabbitmq:3.7.9 Docker image:

make distclean

make otp-21.2.5-build-context

  make /usr/local/bin/elixir
  apt update && apt install zip
  make ezs
  ^D

make docker_build DOCKER_BASE_IMAGE=rabbitmq:3.7.9-management
docker run -it -p 5672:5672 -p 15672:15672 deadtrickster/rabbitmq_prometheus:3.7

cd ~/github.com/rabbitmq/rabbitmq-perf-test
make run ARGS="-x 1 -y 1 -r 1"

curl localhost:15672/api/metrics
# NO CRASHES!

Fixes #75

cc @dcorbacho

This was necessary to test that the fixes for #75 work in RabbitMQ
v3.7.9 & OTP v21.2.5, the combo that ships in Docker image
rabbitmq:3.7.9. This is the first RabbitMQ Docker image that supports
prometheus_rabbitmq_exporter (compiled with OTP v21.0.2). Docker image
rabbitmq:3.7.8 ships with OTP v20.3.8.5

Notice that I disable prometheus_process_collector in the Docker build
context. This fails to build due to Docker image erlang:21.2.5 missing
native libs. Didn't want to shave this yak as well.
In RabbitMQ v3.7.x, the following mappings were not valid and would
result in crashes when a produce/consumer would connect to RabbitMQ:

* connection_metrics
  * connection_recv_count, recv_count
  * connection_send_count, send_count

* channel_queue_metrics
  * channel_queue_get, get
  * channel_queue_get_no_ack, get_no_ack
  * channel_queue_deliver, deliver
  * channel_queue_deliver_no_ack, deliver_no_ack
  * channel_queue_redeliver, redeliver
  * channel_queue_ack, ack
  * channel_queue_get_empty, get_empty

The following metric was mapped incorrectly:

* connection_churn_metrics
  * 7, queue_deleted

There is more line churn in this commit than necessary. I was
re-formatting all metrics as I was going over all mappings. This
confirms that I've checked that all mappings are accurate.

I've tested manually against current v3.7.x branch via `make run-broker`
as well as rabbitmq:3.7.9 Docker image:

    make distclean

    make otp-21.2.5-build-context

      make /usr/local/bin/elixir
      apt update && apt install zip
      make ezs
      ^D

    make docker_build DOCKER_BASE_IMAGE=rabbitmq:3.7.9-management
    docker run -it -p 5672:5672 -p 15672:15672 deadtrickster/rabbitmq_prometheus:3.7

    cd ~/github.com/rabbitmq/rabbitmq-perf-test
    make run ARGS="-x 1 -y 1 -r 1"

    curl localhost:15672/api/metrics
    # NO CRASHES!

Fixes #75

cc @dcorbacho
@deadtrickster
Copy link
Owner

Awesome! version will be 3.7.9.1?

@gerhard
Copy link
Contributor Author

gerhard commented May 6, 2019

Yes, 3.7.9.1 makes sense to me 👍🏻

@gerhard
Copy link
Contributor Author

gerhard commented May 6, 2019

Before you cut a new release, let me share the Docker image which I've built locally. Maybe someone in #75 can confirm the fix before shipping a new prometheus_rabbitmq_exporter.

Done: docker run -it -p 5672:5672 -p 15672:15672 --rm pivotalrabbitmq/prometheus_rabbitmq_exporter:80

@deadtrickster deadtrickster merged commit bea0f31 into deadtrickster:master May 8, 2019
@deadtrickster
Copy link
Owner

Thank you, Gerhard :-)

@gerhard gerhard deleted the fix-ets-mappings-rabbitmq-core-collector branch May 8, 2019 09:37
@gerhard
Copy link
Contributor Author

gerhard commented May 8, 2019

@deadtrickster there is another PR like this one coming soon. Found other typos, incorrect descriptions and some missing metrics. Going over everything like a🕵🏻‍♀️. There will be no line churn, all corrections will be easy to spot.

@gerhard
Copy link
Contributor Author

gerhard commented May 9, 2019

I didn't have time to PR this yet, but considering the amount of changes, it's safer to ask if it's worth putting one together: rabbitmq/rabbitmq-prometheus@0057f07#diff-fa0286c717bf744481489229d1e4ab5e.

Notice that metric names have changed as well, as well as descriptions.

Still want this new PR?

@deadtrickster
Copy link
Owner

PRs are always welcome!

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