New zocalo.util.rabbitmq.RabbitMQAPI #140

rjgildea · 2021-10-27T21:56:29Z

Provide a (not fully comprehensive) Python wrapper around the RabbitMQ HTTP API. Can be used to read various metrics useful for monitoring/health checks, and configuring exchanges, policies, queues, etc.

Adds explicit dependencies on pydantic and requests modules and a test dependency on requests-mock.

Alternative to #136

Adds requests_mock dev dependency

Pass if_unused/empty parameters to queue_delete

To simplify interface

lgtm-com · 2021-10-27T22:04:27Z

This pull request introduces 1 alert when merging d7197ce into a7a1ff8 - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

lgtm-com · 2021-10-27T22:12:55Z

This pull request introduces 1 alert when merging 5b92239 into a7a1ff8 - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

This line isn't hit in the tests, and it isn't trivial to inject fake credentials via the test.

lgtm-com · 2021-10-27T22:34:51Z

This pull request introduces 1 alert when merging e91b751 into a7a1ff8 - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

HISTORY.rst

Anthchirp · 2021-10-28T15:08:00Z

src/zocalo/util/rabbitmq.py

+    @create_component.register
+    def exchange_declare(self, exchange: ExchangeSpec):


interesting use of singledispatch 🤔

Anthchirp · 2021-10-28T15:08:49Z

src/zocalo/util/rabbitmq.py

+    @delete_component.register
+    def _delete_exchange(self, exchange: ExchangeSpec, **kwargs):
+        self.exchange_delete(vhost=exchange.vhost, name=exchange.name, **kwargs)


But surely this one should just be called def _(...):?

I did have it as def _(...) but then mypy complains about it, see eg. python/mypy#2904

If this style is acceptable, I slightly prefer it - I still read the function name first, but all the examples I could find for singledispatch used this style.

Anthchirp · 2021-10-28T15:10:48Z

tests/util/test_rabbitmq.py

+    rmqapi.exchange_declare(exchange=exchange_spec(name))
+    rmqapi.create_component(exchange_spec(name))


So do these two calls not do the same thing then?

I'd added the {exchange,queue}_{declare,delete} etc methods originally, more closely mirroring either the pika or rabbitmqctl interfaces, and then later added the {create,delete}_component interfaces for compatibility with what @d-j-hatton did in his original implementation of the configuration script. E.g. queue_delete and delete_component(queue) have different interfaces, as you can call the former with just the vhost and name strings, whereas the latter expects a full QueueSpec object.

lgtm-com · 2021-10-28T15:40:24Z

This pull request introduces 1 alert when merging 24ebf4c into a7a1ff8 - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

d-j-hatton · 2021-11-03T09:50:11Z

I used a singledispatchmethod in the configuration script that this has inherited because we didn't need to worry about python versions there (singledispatchmethod was new in 3.8). Does this need a slight rewrite to fix the test failures related to this, or do we want to drop the singledispatch in favour of a more {exchange, queue}_{declare, delete} design?

rjgildea · 2021-11-03T10:27:46Z

I used a singledispatchmethod in the configuration script that this has inherited because we didn't need to worry about python versions there (singledispatchmethod was new in 3.8). Does this need a slight rewrite to fix the test failures related to this, or do we want to drop the singledispatch in favour of a more {exchange, queue}_{declare, delete} design?

Since the singledispatch mode was added after the explicit {exchange, queue}_{declare, delete} versions, it should only need a relatively straightforward revert of a single commit. The updated version of the server configuration script that uses the API would need a bit of re-working to use the explicit methods, but obviously not insurmountable.

d-j-hatton · 2021-11-03T10:40:14Z

I used a singledispatchmethod in the configuration script that this has inherited because we didn't need to worry about python versions there (singledispatchmethod was new in 3.8). Does this need a slight rewrite to fix the test failures related to this, or do we want to drop the singledispatch in favour of a more {exchange, queue}_{declare, delete} design?

Since the singledispatch mode was added after the explicit {exchange, queue}_{declare, delete} versions, it should only need a relatively straightforward revert of a single commit. The updated version of the server configuration script that uses the API would need a bit of re-working to use the explicit methods, but obviously not insurmountable.

I think either way is fairly simple. The single dispatch method could just be changed to call a single dispatch that doesn't live inside the class. I don't mind either direction. Any preferences?

functools.singledispatchmethod is Python 3.8+ only

rjgildea · 2021-11-03T14:26:48Z

I used a singledispatchmethod in the configuration script that this has inherited because we didn't need to worry about python versions there (singledispatchmethod was new in 3.8). Does this need a slight rewrite to fix the test failures related to this, or do we want to drop the singledispatch in favour of a more {exchange, queue}_{declare, delete} design?

Since the singledispatch mode was added after the explicit {exchange, queue}_{declare, delete} versions, it should only need a relatively straightforward revert of a single commit. The updated version of the server configuration script that uses the API would need a bit of re-working to use the explicit methods, but obviously not insurmountable.

I think either way is fairly simple. The single dispatch method could just be changed to call a single dispatch that doesn't live inside the class. I don't mind either direction. Any preferences?

I've removed the functools.singledispatchmethod use here, and then in our configuration script we can just extend the API to add the {create,delete}_component methods where we don't have the limitation of supporting Python prior to 3.8.

lgtm-com · 2021-11-03T14:30:22Z

This pull request introduces 1 alert when merging 7c3e26b into 5f070ee - view on LGTM.com

new alerts:

1 for Clear-text logging of sensitive information

rjgildea added 24 commits October 14, 2021 13:41

New zocalo.util.rabbitmq.RabbitMQAPI

f018165

Method -> property

61e1fa8

history

2c37a48

Use RabbitMQAPI in dlq_check

1013cdb

Fix test

7dcc637

Remove broken attempt at filtering useful keys

dfe8034

WIP advanced API

212c54c

nodes and connections

0201dfb

Remove underscore prefix

8bab661

Fix tests for new API

45d688d

Adds requests_mock dev dependency

Tests for queue_declare/delete

ece517d

Pass if_unused/empty parameters to queue_delete

Tests for exchanges endpoints

7a19152

Test for empty exchange name

55b3177

Test for rmqapi.connections()

5c1f1f2

get/add/delete users

94e0d6f

get/set/clear policies

2e68810

dict -> Dict

73094b2

Add vhost to Exchange/Policy/QueueSpec

77d5b2a

To simplify interface

Test exchange_delete

11919ea

Add {create,delete}_component singledispatchmethods

ec0e219

Add requests, pydantic to dependencies

3f123de

Updated RabbitMQAPI interface

fd63b3a

Merge branch 'main' into rabbitmqapi2

455984b

Remove debugging code

d7197ce

rjgildea requested review from Anthchirp and d-j-hatton October 27, 2021 21:56

Add dependencies here too

5b92239

Move RabbitMQAPI instantiation in line with use

e91b751

This line isn't hit in the tests, and it isn't trivial to inject fake credentials via the test.

Remove duplicate key

bca16dd

Anthchirp reviewed Oct 28, 2021

View reviewed changes

Too much history is a bad thing

24ebf4c

Partial revert of ec0e219

7c3e26b

functools.singledispatchmethod is Python 3.8+ only

Remove logging of response to silence lgtm warnings

e517160

d-j-hatton approved these changes Nov 3, 2021

View reviewed changes

rjgildea merged commit d0ea600 into DiamondLightSource:main Nov 3, 2021

rjgildea deleted the rabbitmqapi2 branch November 3, 2021 16:08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New zocalo.util.rabbitmq.RabbitMQAPI #140

New zocalo.util.rabbitmq.RabbitMQAPI #140

rjgildea commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

Anthchirp Oct 28, 2021

Anthchirp Oct 28, 2021

rjgildea Oct 28, 2021

ndevenish Oct 28, 2021

Anthchirp Oct 28, 2021

rjgildea Oct 28, 2021

lgtm-com bot commented Oct 28, 2021

d-j-hatton commented Nov 3, 2021

rjgildea commented Nov 3, 2021

d-j-hatton commented Nov 3, 2021

rjgildea commented Nov 3, 2021

lgtm-com bot commented Nov 3, 2021

		@create_component.register
		def exchange_declare(self, exchange: ExchangeSpec):

		rmqapi.exchange_declare(exchange=exchange_spec(name))
		rmqapi.create_component(exchange_spec(name))

New zocalo.util.rabbitmq.RabbitMQAPI #140

New zocalo.util.rabbitmq.RabbitMQAPI #140

Conversation

rjgildea commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

lgtm-com bot commented Oct 27, 2021

Anthchirp Oct 28, 2021

Choose a reason for hiding this comment

Anthchirp Oct 28, 2021

Choose a reason for hiding this comment

rjgildea Oct 28, 2021

Choose a reason for hiding this comment

ndevenish Oct 28, 2021

Choose a reason for hiding this comment

Anthchirp Oct 28, 2021

Choose a reason for hiding this comment

rjgildea Oct 28, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Oct 28, 2021

d-j-hatton commented Nov 3, 2021

rjgildea commented Nov 3, 2021

d-j-hatton commented Nov 3, 2021

rjgildea commented Nov 3, 2021

lgtm-com bot commented Nov 3, 2021