-
Notifications
You must be signed in to change notification settings - Fork 599
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
Support aio_pika 8.x #1481
Support aio_pika 8.x #1481
Changes from 1 commit
fa11167
3667090
2e87040
2ec3d0c
dad4480
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ | |
) | ||
from opentelemetry.trace import Span, SpanKind, Tracer | ||
|
||
_DEFAULT_ATTRIBUTES = {SpanAttributes.MESSAGING_SYSTEM: 'rabbitmq'} | ||
_DEFAULT_ATTRIBUTES = {SpanAttributes.MESSAGING_SYSTEM: "rabbitmq"} | ||
|
||
|
||
class SpanBuilder: | ||
|
@@ -49,18 +49,30 @@ def set_destination(self, destination: str): | |
self._attributes[SpanAttributes.MESSAGING_DESTINATION] = destination | ||
|
||
def set_channel(self, channel: AbstractChannel): | ||
url = channel.connection.connection.url | ||
self._attributes.update({ | ||
SpanAttributes.NET_PEER_NAME: url.host, | ||
SpanAttributes.NET_PEER_PORT: url.port | ||
}) | ||
connection = channel.connection | ||
if getattr(connection, "connection", None): | ||
# aio_rmq 7 | ||
url = connection.connection.url | ||
else: | ||
# aio_rmq 8 | ||
url = connection.url | ||
self._attributes.update( | ||
{ | ||
SpanAttributes.NET_PEER_NAME: url.host, | ||
SpanAttributes.NET_PEER_PORT: url.port, | ||
} | ||
) | ||
|
||
def set_message(self, message: AbstractMessage): | ||
properties = message.properties | ||
if properties.message_id: | ||
self._attributes[SpanAttributes.MESSAGING_MESSAGE_ID] = properties.message_id | ||
self._attributes[ | ||
SpanAttributes.MESSAGING_MESSAGE_ID | ||
] = properties.message_id | ||
if properties.correlation_id: | ||
self._attributes[SpanAttributes.MESSAGING_CONVERSATION_ID] = properties.correlation_id | ||
self._attributes[ | ||
SpanAttributes.MESSAGING_CONVERSATION_ID | ||
] = properties.correlation_id | ||
|
||
def build(self) -> Optional[Span]: | ||
if not is_instrumentation_enabled(): | ||
|
@@ -69,9 +81,11 @@ def build(self) -> Optional[Span]: | |
self._attributes[SpanAttributes.MESSAGING_OPERATION] = self._operation.value | ||
else: | ||
self._attributes[SpanAttributes.MESSAGING_TEMP_DESTINATION] = True | ||
span = self._tracer.start_span(self._generate_span_name(), kind=self._kind, attributes=self._attributes) | ||
span = self._tracer.start_span( | ||
self._generate_span_name(), kind=self._kind, attributes=self._attributes | ||
) | ||
return span | ||
|
||
def _generate_span_name(self) -> str: | ||
operation_value = self._operation.value if self._operation else 'send' | ||
return f'{self._destination} {operation_value}' | ||
operation_value = self._operation.value if self._operation else "send" | ||
return f"{self._destination} {operation_value}" | ||
Comment on lines
-76
to
+91
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how this was passing the linter before. This (and other related changes) are the result of me:
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,8 +15,10 @@ | |
SERVER_URL = URL( | ||
f"amqp://{SERVER_USER}:{SERVER_PASS}@{SERVER_HOST}:{SERVER_PORT}/" | ||
) | ||
CONNECTION = Namespace(connection=Namespace(url=SERVER_URL)) | ||
CHANNEL = Namespace(connection=CONNECTION, loop=None) | ||
CONNECTION_7 = Namespace(connection=Namespace(url=SERVER_URL)) | ||
CONNECTION_8 = Namespace(url=SERVER_URL) | ||
Comment on lines
+18
to
+19
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This namespace was a change between 7 and 8 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @nozik this is the namespace change I was describing above |
||
CHANNEL_7 = Namespace(connection=CONNECTION_7, loop=None) | ||
CHANNEL_8 = Namespace(connection=CONNECTION_8, loop=None) | ||
Comment on lines
+18
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This isn't a great situation to completely mock this. Probably the correct evolution is to add a docker test that ensures that we can really publish and consume with both aio_pika 7 and 8. I can add that if required, but it's a little more involved. |
||
MESSAGE = Namespace( | ||
properties=Namespace( | ||
message_id=MESSAGE_ID, correlation_id=CORRELATION_ID, headers={} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,9 +13,9 @@ | |
# limitations under the License. | ||
import asyncio | ||
from typing import Type | ||
from unittest import TestCase, mock | ||
from unittest import TestCase, mock, skipIf | ||
|
||
from aio_pika import Exchange, RobustExchange | ||
from aio_pika import Exchange, RobustExchange, version_info | ||
|
||
from opentelemetry.instrumentation.aio_pika.publish_decorator import ( | ||
PublishDecorator, | ||
|
@@ -24,8 +24,10 @@ | |
from opentelemetry.trace import SpanKind, get_tracer | ||
|
||
from .consts import ( | ||
CHANNEL, | ||
CONNECTION, | ||
CHANNEL_7, | ||
CHANNEL_8, | ||
CONNECTION_7, | ||
CONNECTION_8, | ||
CORRELATION_ID, | ||
EXCHANGE_NAME, | ||
MESSAGE, | ||
|
@@ -37,7 +39,8 @@ | |
) | ||
|
||
|
||
class TestInstrumentedExchange(TestCase): | ||
@skipIf(version_info >= (8, 0), "Only for aio_pika 7") | ||
class TestInstrumentedExchangeAioRmq7(TestCase): | ||
EXPECTED_ATTRIBUTES = { | ||
SpanAttributes.MESSAGING_SYSTEM: MESSAGING_SYSTEM, | ||
SpanAttributes.MESSAGING_DESTINATION: f"{EXCHANGE_NAME},{ROUTING_KEY}", | ||
|
@@ -54,7 +57,7 @@ def setUp(self): | |
asyncio.set_event_loop(self.loop) | ||
|
||
def test_get_publish_span(self): | ||
exchange = Exchange(CONNECTION, CHANNEL, EXCHANGE_NAME) | ||
exchange = Exchange(CONNECTION_7, CHANNEL_7, EXCHANGE_NAME) | ||
tracer = mock.MagicMock() | ||
PublishDecorator(tracer, exchange)._get_publish_span( | ||
MESSAGE, ROUTING_KEY | ||
|
@@ -66,7 +69,60 @@ def test_get_publish_span(self): | |
) | ||
|
||
def _test_publish(self, exchange_type: Type[Exchange]): | ||
exchange = exchange_type(CONNECTION, CHANNEL, EXCHANGE_NAME) | ||
exchange = exchange_type(CONNECTION_7, CHANNEL_7, EXCHANGE_NAME) | ||
with mock.patch.object( | ||
PublishDecorator, "_get_publish_span" | ||
) as mock_get_publish_span: | ||
with mock.patch.object( | ||
Exchange, "publish", return_value=asyncio.sleep(0) | ||
) as mock_publish: | ||
decorated_publish = PublishDecorator( | ||
self.tracer, exchange | ||
).decorate(mock_publish) | ||
self.loop.run_until_complete( | ||
decorated_publish(MESSAGE, ROUTING_KEY) | ||
) | ||
mock_publish.assert_called_once() | ||
mock_get_publish_span.assert_called_once() | ||
|
||
def test_publish(self): | ||
self._test_publish(Exchange) | ||
|
||
def test_robust_publish(self): | ||
self._test_publish(RobustExchange) | ||
|
||
|
||
@skipIf(version_info <= (8, 0), "Only for aio_pika 8") | ||
class TestInstrumentedExchangeAioRmq8(TestCase): | ||
EXPECTED_ATTRIBUTES = { | ||
SpanAttributes.MESSAGING_SYSTEM: MESSAGING_SYSTEM, | ||
SpanAttributes.MESSAGING_DESTINATION: f"{EXCHANGE_NAME},{ROUTING_KEY}", | ||
SpanAttributes.NET_PEER_NAME: SERVER_HOST, | ||
SpanAttributes.NET_PEER_PORT: SERVER_PORT, | ||
SpanAttributes.MESSAGING_MESSAGE_ID: MESSAGE_ID, | ||
SpanAttributes.MESSAGING_CONVERSATION_ID: CORRELATION_ID, | ||
SpanAttributes.MESSAGING_TEMP_DESTINATION: True, | ||
} | ||
|
||
def setUp(self): | ||
self.tracer = get_tracer(__name__) | ||
self.loop = asyncio.new_event_loop() | ||
asyncio.set_event_loop(self.loop) | ||
|
||
def test_get_publish_span(self): | ||
exchange = Exchange(CHANNEL_8, EXCHANGE_NAME) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
tracer = mock.MagicMock() | ||
PublishDecorator(tracer, exchange)._get_publish_span( | ||
MESSAGE, ROUTING_KEY | ||
) | ||
tracer.start_span.assert_called_once_with( | ||
f"{EXCHANGE_NAME},{ROUTING_KEY} send", | ||
kind=SpanKind.PRODUCER, | ||
attributes=self.EXPECTED_ATTRIBUTES, | ||
) | ||
|
||
def _test_publish(self, exchange_type: Type[Exchange]): | ||
exchange = exchange_type(CONNECTION_8, CHANNEL_8, EXCHANGE_NAME) | ||
with mock.patch.object( | ||
PublishDecorator, "_get_publish_span" | ||
) as mock_get_publish_span: | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice that this change shouldn't be necessary - the constructor of
Exchange
doesn't get the connection as its first argument anymore (mosquito/aio-pika@c2ee4be). If you take that into consideration, this change would not be necessaryThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nozik I'm not sure I'm following, could you be a bit more specific with what you would think this
set_channel
method needs to change to if not what I have here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see, over in https://github.com/open-telemetry/opentelemetry-python-contrib/pull/1605/files#diff-500514cf8be14e4864fdf49daa4c761c0b1ad0c358cec84b4e26a2c62c1c5dffR57.
That change you made there is incomplete. The change I made here is necessary for aiopika 8.x, the call here fails otherwise:
In your PR, you are still using an aio_pika v7 mock that assumes a property path from
channel.connection.connection.url
. In aio_pika v8, the extra.connection
is removed, and it's justchannel.connection.url
.The change occurred actually in the same commit you linked, these lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phillipuniverse Notice here the changes to
exchange.py
- theExchange
class no longer receivesAbstractConnection
as its first constructor argument. However, intest_get_publish_span
we're still sending wrongfully sending it as the first argument. If you just check the condition of theaio-pika
version and adapt the creation of theExchange
(simply remove the first ctor argument for version >= 8.0.0), all the other code changes will become redundant. I hope this helps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are 2 changes that needed to happen to support aio_pika 8:
There aren’t good integration tests for aio_pika yet in this repo but I observed the problem in this method in my integration environment after patching the instrumentation version check to indiscriminately apply to aio_pika 8
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi there,
I'm also looking for support of aio_pika 8 so I wonder, when optimistically this PR could be merged and released? Thank you in advance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This instrumentation is supported and maintained by community members. Maintainers don't have any expertise in this instrumentation to judge the changes. The approval from the component owner gets it merged immediately and will be part of the next scheduled release.