RabbitMQ Healthcheck Contribution #197

ipeternella · 2018-07-12T02:39:13Z

This pull request contains the following implementations:

A class to perform the healthcheck of a RabbitMQ service;
Tests for the RabbitMQ Healthchecker;
An updated README.rst with the instructions on how to configure the RabbitMQ healthchecker.

codingjoe

Hi @IgooorGP great work! Really appreciate the effort. Especially about including tests!

I do have some comments. Please also make sure to add documentation.

codingjoe · 2018-07-12T09:34:49Z

.gitignore

@@ -102,3 +102,8 @@ ENV/

 # pytest
 .pytest_cache/
+


Please add editor specific ignores to you global git ignore. Not to a repository.

Hello there @codingjoe! I have removed these files to a global git ignore. Sorry for this mistake.

codingjoe · 2018-07-12T09:35:14Z

health_check/contrib/rabbitmq/apps.py

+
+    def ready(self):
+        from .backends import RabbitMQHealthCheck
+        plugin_dir.register(RabbitMQHealthCheck)


missing newline

codingjoe · 2018-07-12T09:35:36Z

health_check/contrib/rabbitmq/backends.py

+class RabbitMQHealthCheck(BaseHealthCheckBackend):
+    """
+    Healthchecker for RabbitMQ.
+


Please drop trailing blank line

codingjoe · 2018-07-12T09:35:41Z

health_check/contrib/rabbitmq/backends.py

+    def __get_broker_url(self):
+        """
+        Gets the broker url based on django.confg.settings dictionary.
+


Please drop trailing blank line

I have removed this method due to the mentioned complexity of the configuration.

codingjoe · 2018-07-12T09:38:26Z

health_check/contrib/rabbitmq/backends.py

+        """
+        broker_url = getattr(settings, "BROKER_URL", None)
+
+        if broker_url is None:


I have the feeling this drives complexity. I'd rather have only one approach on how to configure this correctly.

I have removed this method due to the mentioned complexity. Right now, the RabbitMQ healthchecker only looks for a BROKER_URL variable on django settings dict.

codingjoe · 2018-07-12T09:42:48Z

health_check/contrib/rabbitmq/backends.py

+
+            logger.info("Got broker_url: %s. Creating a connection to the broker...", broker_url)
+
+            conn = pika.BlockingConnection(pika.URLParameters(broker_url))


I am not so sure about pika. Most people will use celery and it's amqplib fork.

I have removed pika from the dependencies. Instead of it, I use Kombu which is a dependency of the Celery package and therefore comes together with Celery! Kombu uses pyamqp by default according to its docs http://docs.celeryproject.org/projects/kombu/en/latest/userguide/connections.html

codingjoe · 2018-07-12T09:43:44Z

health_check/contrib/rabbitmq/backends.py

+            logger.info("RabbitMQ is healthy.")
+
+        except ConnectionClosed as e:
+            self.add_error(ServiceUnavailable("Unable to connect to RabbitMQ: Connection refused."), e)


You can also just use raise ServiceUnavailable("your message") from e

codingjoe · 2018-07-12T09:44:00Z

requirements-dev.txt

@@ -9,3 +9,4 @@ pydocstyle
 pep8-naming
 pytest
 pytest-django
+pika


yet another dependency...

Removed pika dependency.

codingjoe · 2018-07-12T09:44:32Z

setup.cfg

@@ -35,6 +35,9 @@ norecursedirs=venv env .eggs
 DJANGO_SETTINGS_MODULE=tests.testapp.settings
 addopts = --tb=short -rxs

+[pep8]
+max-line-length = 119


Unrelated change.

Removed this unnecessary change.

codingjoe · 2018-07-12T09:45:14Z

tests/test_rabbitmq.py

+
+
+if __name__ == "__main__":
+    unittest.main()


Kudos for writing tests. I really appreciate it. But we use pytest as a test runner.

Removed from the tests.

…and README.

ipeternella · 2018-07-18T00:35:36Z

Hello @codingjoe!

Thanks for the code review. I have implemented the requested changes. Please, could you take another look to see if it is acceptable now? Should you have any comments or new requests, please do so.

Thanks a lot!

codingjoe

Almost there! Just some small changes and this is ready to roll. Thanks 👍

codingjoe · 2018-07-23T14:42:20Z

health_check/contrib/rabbitmq/backends.py

+        broker_url = getattr(settings, "BROKER_URL", None)
+
+        try:
+            logger.info("Got %s as the broker_url. Connecting to rabbit...")


Argument for %s is missing

codingjoe · 2018-07-23T14:42:35Z

health_check/contrib/rabbitmq/backends.py

+        broker_url = getattr(settings, "BROKER_URL", None)
+
+        try:
+            logger.info("Got %s as the broker_url. Connecting to rabbit...")


please use debug level here

codingjoe · 2018-07-23T14:42:42Z

health_check/contrib/rabbitmq/backends.py

+            conn = Connection(broker_url)
+
+            # opens and closes a channel to rabbitmq to make sure the service is up
+            logger.info("Openning a channel to RabbitMQ...")


same here, debug please

codingjoe · 2018-07-23T14:43:29Z

health_check/contrib/rabbitmq/backends.py

+            conn.channel()
+
+            logger.info("Closing the channel to RabbitMQ...")
+            conn.close()


is there a context manager you could use? Even if something fails the connection should be closed

codingjoe · 2018-07-23T14:44:09Z

health_check/contrib/rabbitmq/backends.py

+
+        broker_url = getattr(settings, "BROKER_URL", None)
+
+        try:


There is too much happening in the try block. Please be more narrow, where you expect the exception to occour.

codingjoe · 2018-07-23T14:44:22Z

health_check/contrib/rabbitmq/backends.py

+        Checks if RabbitMQ service is up by opening and closing
+        a broker channel.
+        """
+        logger.info("Checking for a broker_url on django settings...")


debug please

codingjoe · 2018-07-23T14:45:41Z

tests/test_rabbitmq.py

+from health_check.contrib.rabbitmq.backends import RabbitMQHealthCheck
+
+
+class TestRabbitMQHealthCheck(unittest.TestCase):


Again, we don't use UnitTest but pytest. Please just use a call and switch the assert to simple assert instead of self.assertEqual and co

…k test with the connect method.]

ipeternella · 2018-07-24T02:30:18Z

Hey there @codingjoe ! Thanks again for the review. I have changed the code to use the kombu connection as a context to release opened resources. Also, I decided to use the connect method of the kombu library for the healthcheck because it is easier to understand, produces the same result as before and solves the problem with too many statements in the try/except blocks. Oh, of course, I have changed the tests as well. Please, could you review the code again to make sure everything's all right?

Thanks a lot!

codecov-io · 2018-07-30T11:43:39Z

Codecov Report

Merging #197 into master will decrease coverage by 1.12%.
The diff coverage is 63.63%.

@@            Coverage Diff             @@
##           master     #197      +/-   ##
==========================================
- Coverage   75.39%   74.27%   -1.13%     
==========================================
  Files          29       32       +3     
  Lines         313      346      +33     
==========================================
+ Hits          236      257      +21     
- Misses         77       89      +12

Impacted Files	Coverage Δ
health_check/contrib/rabbitmq/apps.py	`0% <0%> (ø)`
health_check/contrib/rabbitmq/__init__.py	`100% <100%> (ø)`
health_check/contrib/rabbitmq/backends.py	`80% <80%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6064578...c248647. Read the comment docs.

codingjoe · 2018-08-05T09:16:59Z

Released in 3.7.0

ipeternella added 2 commits July 5, 2018 23:21

implemented rabbitmq healthcheck and unit tests

671051d

added new tests for rabbitmq. Updated README.rst

f2c1fef

codingjoe reviewed Jul 12, 2018

View reviewed changes

codingjoe assigned codingjoe and ipeternella Jul 12, 2018

Removed pika dependency by using Kombu connection. Changed gitignore …

3cdd8f4

…and README.

Update .gitignore

185722d

codingjoe requested changes Jul 23, 2018

View reviewed changes

ipeternella added 2 commits July 23, 2018 23:10

Changed kombu connection to be used as context. Simplified healthchec…

c5969df

…k test with the connect method.]

Merge branch 'master' of https://github.com/IgooorGP/django-health-check

73fb071

codingjoe added 5 commits July 30, 2018 13:18

improve try except block

21346fd

fix doc stings

aa7eca2

fix doc strings

7d17526

fix doc strings

649b5cc

fix doc strings

c248647

codingjoe merged commit e49faf9 into revsys:master Jul 30, 2018

GabLeRoux mentioned this pull request Nov 24, 2018

RabbitMQ #196

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RabbitMQ Healthcheck Contribution #197

RabbitMQ Healthcheck Contribution #197

ipeternella commented Jul 12, 2018

codingjoe left a comment

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

codingjoe Jul 12, 2018

ipeternella Jul 18, 2018

ipeternella commented Jul 18, 2018 •

edited

Loading

codingjoe left a comment

codingjoe Jul 23, 2018

codingjoe Jul 23, 2018

codingjoe Jul 23, 2018

codingjoe Jul 23, 2018

codingjoe Jul 23, 2018

codingjoe Jul 23, 2018

codingjoe Jul 23, 2018

ipeternella commented Jul 24, 2018

codecov-io commented Jul 30, 2018 •

edited

Loading

codingjoe commented Aug 5, 2018


		logger.info("Got broker_url: %s. Creating a connection to the broker...", broker_url)

		conn = pika.BlockingConnection(pika.URLParameters(broker_url))

		from health_check.contrib.rabbitmq.backends import RabbitMQHealthCheck


		class TestRabbitMQHealthCheck(unittest.TestCase):

RabbitMQ Healthcheck Contribution #197

RabbitMQ Healthcheck Contribution #197

Conversation

ipeternella commented Jul 12, 2018

codingjoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipeternella commented Jul 18, 2018 • edited Loading

codingjoe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ipeternella commented Jul 24, 2018

codecov-io commented Jul 30, 2018 • edited Loading

Codecov Report

codingjoe commented Aug 5, 2018

ipeternella commented Jul 18, 2018 •

edited

Loading

codecov-io commented Jul 30, 2018 •

edited

Loading