Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 4 commits
fa11167
3667090
2e87040
2ec3d0c
dad4480
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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:
tox.ini
to remove the--check-only
flag toeachdist.py
tox -e lint
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
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.
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 withskipIf
.