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😉 |
|
Hi guys, I am sorry for the late reply, I missed the notification. I suggest to use newer My fork contains only one more change (comparing to the main repo) and that's allowing to set timeouts on The PR looks good otherwise. It is nice that you created also the test 👍 |
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.