-
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
Conversation
You will define an env for each version and its corresponding deps. Please take a look at the tox file https://github.com/open-telemetry/opentelemetry-python-contrib/blob/main/tox.ini and how it is done for Django. |
9bd55b1
to
4725e2f
Compare
986f31d
to
20700ba
Compare
20700ba
to
cda8718
Compare
- Fix tests for new shape of the AbstractConnection class - Run tests against aio_pika 7 and 8
cda8718
to
fa11167
Compare
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.
A few additional notes:
- I included tests by copy/pasting and doing a
skipif
. I don't mind refactoring a bit to reduce code duplication if requested. I didn't have a super strong opinion either way but this way was the easiest to implement. - I can separate the
black
formatting changes if it's too distracting to review - I'm reasonably confident that this encapsulates the changes between aio_pika 7 and 8. I took a look at the classes we were using for the instrumentation and the
Connection
andExchange
were the big ones that had changed. I spot-checked the other abstractions we depend on likeAbstractMessage
and those had not changed.
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}" |
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.
I'm not sure how this was passing the linter before. This (and other related changes) are the result of me:
- Modifying
tox.ini
to remove the--check-only
flag toeachdist.py
- Run
tox -e lint
CONNECTION_7 = Namespace(connection=Namespace(url=SERVER_URL)) | ||
CONNECTION_8 = Namespace(url=SERVER_URL) | ||
CHANNEL_7 = Namespace(connection=CONNECTION_7, loop=None) | ||
CHANNEL_8 = Namespace(connection=CONNECTION_8, loop=None) |
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 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.
CONNECTION_7 = Namespace(connection=Namespace(url=SERVER_URL)) | ||
CONNECTION_8 = Namespace(url=SERVER_URL) |
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 namespace was a change between 7 and 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.
@nozik this is the namespace change I was describing above
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 comment
The reason will be displayed to describe this comment to others. Learn more.
This Exchange
constructor was a change between 7 and 8, and this is also what prompted me to decorate with skipIf
.
@srikanthccv thanks for the tips! I added separate tox environments for 7 and 8 and iterated a bit on some of the differences in the implementations. This is now ready for review. |
SpanAttributes.NET_PEER_NAME: url.host, | ||
SpanAttributes.NET_PEER_PORT: url.port | ||
}) | ||
connection = channel.connection |
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 necessary
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.
@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.
That change you made there is incomplete. The change I made here is necessary for aiopika 8.x, the call here fails otherwise:
url = channel.connection.connection.url
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 just channel.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
- the Exchange
class no longer receives AbstractConnection
as its first constructor argument. However, in test_get_publish_span
we're still sending wrongfully sending it as the first argument. If you just check the condition of the aio-pika
version and adapt the creation of the Exchange
(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:
- The change you are describing to the constructor in the test as aio_pika 8 changed the signature
- The change in this method to deal with the changed shape of AbstractConnection
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.
Ping @ofek1weiss as component owner and the original author. |
Another friendly ping |
Hey, sorry I missed the first ping |
Merging this as it got LGTM from the codeowners. |
Description
I am using aio_pika 8.2 and otel currently cannot instrument it:
Briefly skimming the 8.0.0 release notes it doesn't seem like the breaking changes would affect instrumentation.
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
I would like a bit more information on the right way to do this. Ideally I would like to emit tests for both aio_pika 7.x and 8.x. Tox is not exactly my forte and I'm not sure how to configure this to happen.
That said, since the dependency version is relaxed theoretically the tests will run against 8.x. Need to figure out how to fully verify this (probably related to allowing 7.x and 8.x testing).
Does This PR Require a Core Repo Change?
Checklist:
See contributing.md for styleguide, changelog guidelines, and more.