Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix instrumentation of connection when pool.acquire was called multip… #381

Merged
merged 8 commits into from
Apr 2, 2021
Merged

fix instrumentation of connection when pool.acquire was called multip… #381

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0206312
fix instrumentation of connection when pool.acquire was called multip…
sartx Mar 25, 2021
1fa67b8
add changelog
sartx Mar 25, 2021
566899a
fix linting
sartx Mar 25, 2021
b324b40
replace tests to correct file
sartx Mar 25, 2021
5a2038a
fix typo in test init
sartx Mar 25, 2021
9879b28
make more precisely check, fix typo
sartx Mar 26, 2021
90c3ef7
Merge branch 'main' into fix_336
sartx Mar 29, 2021
3158bf4
Merge branch 'main' into fix_336
Apr 2, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fix linting
  • Loading branch information
@sartx
sartx committed Mar 25, 2021
commit 566899a3b4923637a04cfddf88f8dad80b56b9ca
2 changes: 1 addition & 1 deletion ...emetry-instrumentation-aiopg/src/opentelemetry/instrumentation/aiopg/aiopg_integration.py
View file
Original file line number Diff line number Diff line change
@@ -86,7 +86,7 @@ def acquire(self):
async def _acquire(self):
# pylint: disable=protected-access
connection = await self.__wrapped__._acquire()
if not hasattr(connection, '__wrapped__'):
if not hasattr(connection, "__wrapped__"):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably could check if the wrapper type is the same as the instrumented type but not sure if it's worth it. This is probably enough,.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd probably have argued for something a bit clearer that it was wrapped by opentelemetry, such as __wrapped_by_opentelemetry__. But agreed it's probably overkill: highly unlikely to overlap.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with you. if not isinstance(connection, AsyncProxyObject): - I think that this option more accurately defines the case of verification.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does but not sure if it is worth the extra check :) Probably we should do what @toumorokoshi suggested in a separate PR so all instrumentations benefit from it,.

connection = get_traced_connection_proxy(
connection, db_api_integration, *args, **kwargs
)