Skip to content

Conversation

@ghost
Copy link

@ghost ghost commented Mar 13, 2022

Resolves #354.

Also removes duplicate call to span.set_tag as it seems to already be done at _extract_broker_tags.

@Ferenc-
Copy link
Member

Ferenc- commented Mar 14, 2022

Hi,
Thank you for your great contribution!
I noticed, that the main reason why we can have such bugs, is that we have no unit tests,
which would test with BlockingConnection.
Even our TestPikaBlockingChannel class only uses Connection and no BlockingConnection.
Do you think you would have the capacity to extend your PR with a change in tests/clients/test_pika.py, so it includes coverage for a BlockingConnection scenario?

@ghost
Copy link
Author

ghost commented Mar 14, 2022

Hi @Ferenc-,

I don't have much availability right now, and I'm not that comfortable with the unittest framework, but I will give it a try.

@ghost
Copy link
Author

ghost commented Mar 21, 2022

Sorry for the delay, had to take some time to look into pika implementation details. Let me know what you think about these changes.

@Ferenc-
Copy link
Member

Ferenc- commented Mar 22, 2022

Sorry for the delay, had to take some time to look into pika implementation details. Let me know what you think about these changes.

Looks very good to me.
My only comment would have been the unused class attributes, that you have also fixed in the meantime, before I sent the comment.
So I am merging this now. Many thanks for the contribution.
If you have further improvements then feel free to open new PRs.

@Ferenc- Ferenc- merged commit 14ab5ea into instana:master Mar 22, 2022
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.

AttributeError when extracting tags from pika BlockingChannel

2 participants