Support aio_pika 8.x by phillipuniverse · Pull Request #1481 · open-telemetry/opentelemetry-python-contrib

phillipuniverse · 2022-12-09T18:15:22Z

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.

New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
This change requires a documentation update

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?

No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

Followed the style guidelines of this project
Changelogs have been updated
Unit tests have been added - (yes but need an assist)
Documentation has been updated

srikanthccv · 2022-12-10T01:45:23Z

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.

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.

- Fix tests for new shape of the AbstractConnection class - Run tests against aio_pika 7 and 8

phillipuniverse

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 and Exchange were the big ones that had changed. I spot-checked the other abstractions we depend on like AbstractMessage and those had not changed.

phillipuniverse · 2023-01-16T17:58:27Z

...elemetry-instrumentation-aio-pika/src/opentelemetry/instrumentation/aio_pika/span_builder.py

-        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}"


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 to eachdist.py

Run tox -e lint

phillipuniverse · 2023-01-16T17:59:13Z

instrumentation/opentelemetry-instrumentation-aio-pika/tests/consts.py

+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)


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.

phillipuniverse · 2023-01-16T18:00:33Z

instrumentation/opentelemetry-instrumentation-aio-pika/tests/consts.py

+CONNECTION_7 = Namespace(connection=Namespace(url=SERVER_URL))
+CONNECTION_8 = Namespace(url=SERVER_URL)


This namespace was a change between 7 and 8

@nozik this is the namespace change I was describing above

phillipuniverse · 2023-01-16T18:01:11Z

instrumentation/opentelemetry-instrumentation-aio-pika/tests/test_publish_decorator.py

+        asyncio.set_event_loop(self.loop)
+
+    def test_get_publish_span(self):
+        exchange = Exchange(CHANNEL_8, EXCHANGE_NAME)


This Exchange constructor was a change between 7 and 8, and this is also what prompted me to decorate with skipIf.

phillipuniverse · 2023-01-16T18:15:41Z

@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.

nozik · 2023-01-30T06:00:33Z

...elemetry-instrumentation-aio-pika/src/opentelemetry/instrumentation/aio_pika/span_builder.py

-            SpanAttributes.NET_PEER_NAME: url.host,
-            SpanAttributes.NET_PEER_PORT: url.port
-        })
+        connection = channel.connection


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

@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?

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:

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

@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 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

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.

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.

srikanthccv · 2023-02-02T15:43:33Z

Ping @ofek1weiss as component owner and the original author.

srikanthccv · 2023-02-04T19:20:51Z

Another friendly ping

ofek1weiss · 2023-02-04T21:13:38Z

Hey, sorry I missed the first ping
It looks like one of the checks failed :(
Once that is fixed everything looks good to me 👍

CHANGELOG.md

srikanthccv · 2023-02-05T06:24:15Z

Merging this as it got LGTM from the codeowners.

phillipuniverse requested a review from a team December 9, 2022 18:15

phillipuniverse force-pushed the aiopika-8 branch 3 times, most recently from 9bd55b1 to 4725e2f Compare January 16, 2023 13:50

phillipuniverse marked this pull request as draft January 16, 2023 14:30

phillipuniverse force-pushed the aiopika-8 branch 3 times, most recently from 986f31d to 20700ba Compare January 16, 2023 15:28

phillipuniverse changed the title ~~Relax aio_pika version to allow 8.x~~ Support aio_pika 8.x Jan 16, 2023

phillipuniverse force-pushed the aiopika-8 branch from 20700ba to cda8718 Compare January 16, 2023 16:37

Support aio_pika 8

fa11167

- Fix tests for new shape of the AbstractConnection class - Run tests against aio_pika 7 and 8

phillipuniverse force-pushed the aiopika-8 branch from cda8718 to fa11167 Compare January 16, 2023 17:32

phillipuniverse commented Jan 16, 2023

View reviewed changes

phillipuniverse marked this pull request as ready for review January 16, 2023 18:14

Merge branch 'main' into aiopika-8

3667090

srikanthccv mentioned this pull request Jan 30, 2023

Support aio-pika v8 #1605

Closed

10 tasks

nozik reviewed Jan 30, 2023

View reviewed changes

phillipuniverse requested a review from nozik January 30, 2023 06:36

Merge branch 'main' into aiopika-8

2e87040

Merge branch 'main' into aiopika-8

2ec3d0c

github-actions bot assigned ofek1weiss Feb 4, 2023

srikanthccv reviewed Feb 4, 2023

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

Update CHANGELOG.md

dad4480

srikanthccv approved these changes Feb 5, 2023

View reviewed changes

srikanthccv merged commit 66ceef5 into open-telemetry:main Feb 5, 2023

phillipuniverse deleted the aiopika-8 branch February 5, 2023 15:37

phillipuniverse mentioned this pull request Feb 14, 2023

Support aio_pika 9 #1670

Merged

11 tasks

		CONNECTION_7 = Namespace(connection=Namespace(url=SERVER_URL))
		CONNECTION_8 = Namespace(url=SERVER_URL)

Conversation

phillipuniverse commented Dec 9, 2022

Description

Type of change

How Has This Been Tested?

Does This PR Require a Core Repo Change?

Checklist:

Uh oh!

srikanthccv commented Dec 10, 2022

Uh oh!

phillipuniverse left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phillipuniverse commented Jan 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

phillipuniverse Jan 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

srikanthccv commented Feb 2, 2023

Uh oh!

srikanthccv commented Feb 4, 2023

Uh oh!

ofek1weiss commented Feb 4, 2023

Uh oh!

Uh oh!

srikanthccv commented Feb 5, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

phillipuniverse left a comment •

edited

Loading

phillipuniverse Jan 30, 2023 •

edited

Loading