Updating to amqp version 2.4.2 #119
Conversation
|
Would it be possible to add some instrumentation testing for the |
|
It would be an integration test, using an instance of rabbitmq. They are not in place, so it is not trivial to implement, which is why I did not include tests yet. |
|
Doing a integration test would be preferred. As it also give a implicit example on how to setup graypy -> rabbitmq -> graylog. Good luck with working off of that past branch. I sadly was struggling with setting up a rabbitmq -> graylog environment with docker that was performant and effective for testing. |
graypy/rabbitmq.py
Outdated
| @@ -93,6 +93,7 @@ def __init__(self, cn_args, timeout, exchange, exchange_type, routing_key): | |||
| self.routing_key = routing_key | |||
| self.connection = amqp.Connection( | |||
| connection_timeout=timeout, **self.cn_args) | |||
There was a problem hiding this comment.
Notice that the amqp.Connection class expects connect_timeout instead of connection_timeout keyword parameter. This is actually a bug also with current amqplib.
Codecov Report
@@ Coverage Diff @@
## master #119 +/- ##
==========================================
+ Coverage 95.97% 97.44% +1.47%
==========================================
Files 3 3
Lines 273 274 +1
==========================================
+ Hits 262 267 +5
+ Misses 11 7 -4
Continue to review full report at Codecov.
|
|
@martinvy any updates on this PR? This seems to fix an issue I've been having with graypy/amqp. The amqplib module is so old that is requires you to stick to a version of setuptools no newer than 59.8.0 in order to install graypy with the amqp extra. Otherwise after doing an install of graypy will get you this issue: >>> import graypy
>>> graypy.GELFRabbitHandler()
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
AttributeError: module 'graypy' has no attribute 'GELFRabbitHandler'It would be fantastic if we could get a new graypy release in pypi with this fix. |
|
@cyber-conor Good question, I think this pull request is good to go. At the time nobody seemed to work on this project anymore, but I see some recent updates in the last months. @nklapste, could you have a look at this, and if you have the “authority” merge this if you are content? EDIT: @nklapste I even added the integration test you requested😉 |
As amqplib is quite old (https://pypi.org/project/amqplib/#history) and I had some issues with reconnecting, I propose to update it to amqp, which has the same interface, but is maintained and widely used.