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

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

Merged
merged 8 commits into from
Apr 2, 2021

Conversation

sartx
Copy link
Contributor

@sartx sartx commented Mar 25, 2021

…le times

Description

Fix appearing multiple nested spans when instrumented aiopg.pool was used. The reason of the bug was that pool.acquire instrumented connection every time. But pool reuses old connections which had already been instrumented. This fix addes checking connection. Instrumenting connection only if it was not instrumented.

Fixes #336

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

See added integration test in this PR.

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
  • Documentation has been updated

@sartx sartx requested review from a team, toumorokoshi and aabmass and removed request for a team March 25, 2021 11:55
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,.

Copy link
Member

@toumorokoshi toumorokoshi left a comment

Choose a reason for hiding this comment

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

LGTM! possible typo worth looking at.

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks for the fix! 👍

@codeboten codeboten merged commit 12da395 into open-telemetry:main Apr 2, 2021
@cdvv7788
Copy link
Contributor

cdvv7788 commented Apr 3, 2021

Thanks!

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.

aiopg instrumentation creating nested traces
5 participants