Skip to content
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

Feature/wait reopen channel state #533

Merged
merged 25 commits into from Jun 1, 2023
Merged

Conversation

mosquito
Copy link
Owner

@mosquito mosquito commented Mar 27, 2023

Fixes #505

@coveralls
Copy link

coveralls commented Mar 27, 2023

Coverage Status

Coverage: 88.26% (-0.5%) from 88.755% when pulling 87617bf on featire/wait-reopen-channel-state into 6d4c8a9 on master.

Alviner
Alviner previously approved these changes Mar 28, 2023
aio_pika/channel.py Outdated Show resolved Hide resolved
aio_pika/channel.py Show resolved Hide resolved
aio_pika/channel.py Outdated Show resolved Hide resolved
tzoiker
tzoiker previously approved these changes Mar 28, 2023
@mosquito mosquito force-pushed the featire/wait-reopen-channel-state branch from 886b18b to e3d0144 Compare March 28, 2023 13:08
@mosquito mosquito requested review from decaz, Pavkazzz, Alviner and tzoiker and removed request for Pavkazzz and tzoiker March 28, 2023 13:10
@mosquito mosquito force-pushed the featire/wait-reopen-channel-state branch from e3d0144 to fccdefd Compare March 28, 2023 13:11
aio_pika/robust_channel.py Outdated Show resolved Hide resolved
aio_pika/robust_exchange.py Outdated Show resolved Hide resolved
aio_pika/robust_queue.py Outdated Show resolved Hide resolved
aio_pika/robust_channel.py Show resolved Hide resolved
@decaz decaz changed the title Featire/wait reopen channel state Feature/wait reopen channel state Mar 28, 2023
@mosquito mosquito force-pushed the featire/wait-reopen-channel-state branch from 0f116f8 to ffc8435 Compare May 10, 2023 19:06
@mosquito mosquito requested review from tzoiker and decaz May 10, 2023 20:42
@mosquito mosquito force-pushed the featire/wait-reopen-channel-state branch from 661938c to 9a27311 Compare May 11, 2023 20:44
Alviner
Alviner previously approved these changes May 19, 2023
@phillipuniverse
Copy link

@mosquito question about this with the removed connection construct.

In the aio_pika OpenTelemetry instrumentation located here there are a few fallbacks to getting the active URL from the channel. The connection url is being retrieved from the channel since that's all that is really available at publish time, which otel decorates here.

I'm wondering if you have some advice on what the correct thing to do in this case would be to support the changes in 9.1 while still supporting 7.x, 8.x and 9.1+?

I would hate to drop down to use channel._connection (although that will work most of the time), do you think change the method to async and use the get_underlay_channel()? Like this:

     async def set_channel(self, channel: AbstractChannel):
        # aio_rmq 9.1 removed the connection attribute from the abstract listings
        if hasattr(channel, "get_underlay_channel"):
            connection = (await channel.get_underlay_channel()).connection
        else:
            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,
            }
        )

Or would it be better in your opinion to keep this sync and check for channel._connection like this:

     async def set_channel(self, channel: AbstractChannel):
        # aio_rmq 9.1 removed the connection attribute from the abstract listings
        if hasattr(channel, "_connection"):
            connection = channel._connection
        else:
            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,
            }
        )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reconnect does not work with version 8.2.4 but works with version 8.2.3 (8.2.2, 8.2.1, 8.2.0 as well)
7 participants