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.

Sign up for GitHub

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

Jump to bottom

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

Show file tree
Hide file tree
Changes from 5 commits
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
([#350](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/350))
- `opentelemetry-exporter-datadog` Fix warning when DatadogFormat encounters a request with
no DD_ORIGIN headers ([#368](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/368)).
- `opentelemetry-instrumentation-aiopg` Fix multiple nested spans when
`aiopg.pool` is used
([#336](https://github.com/open-telemetry/opentelemetry-python-contrib/pull/381)).

### Removed
- Removing support for Python 3.5
Expand Down
8 changes: 5 additions & 3 deletions ...emetry-instrumentation-aiopg/src/opentelemetry/instrumentation/aiopg/aiopg_integration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,11 @@ def acquire(self):
async def _acquire(self):
# pylint: disable=protected-access
connection = await self.__wrapped__._acquire()
return get_traced_connection_proxy(
connection, db_api_integration, *args, **kwargs
)
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
)
return connection

return TracedPoolProxy(pool, *args, **kwargs)

Expand Down
20 changes: 20 additions & 0 deletions tests/opentelemetry-docker-tests/tests/postgres/test_aiopg_functional.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,10 @@ def setUpClass(cls):
cls._cursor = None
cls._tracer = cls.tracer_provider.get_tracer(__name__)
AiopgInstrumentor().instrument(tracer_provider=cls.tracer_provider)
cls._dsn = (
f"dbname='{POSTGRES_DB_NAME}' user='{POSTGRES_USER}' password='{POSTGRES_PASSWORD}'"
f" host='{POSTGRES_HOST}' port='{POSTGRES_PORT}'"
)
cls._pool = async_call(
aiopg.create_pool(
dbname=POSTGRES_DB_NAME,
Expand Down Expand Up @@ -185,3 +189,19 @@ def test_callproc(self):
):
async_call(self._cursor.callproc("test", ()))
self.validate_spans("test")

def test_instrumented_pool_with_multiple_acquires(self, *_, **__):
async def double_asquire():
sartx marked this conversation as resolved.
Show resolved Hide resolved
pool = await aiopg.create_pool(dsn=self._dsn)
async with pool.acquire() as conn:
async with conn.cursor() as cursor:
query = "SELECT 1"
await cursor.execute(query)
async with pool.acquire() as conn:
async with conn.cursor() as cursor:
query = "SELECT 1"
await cursor.execute(query)

async_call(double_asquire())
spans = self.memory_exporter.get_finished_spans()
self.assertEqual(len(spans), 2)