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

Add instrumentation for aiopg #801

Merged
merged 53 commits into from
Jul 24, 2020

Conversation

sartx
Copy link
Contributor

@sartx sartx commented Jun 10, 2020

Hi, everyone!

This is opentelemetry integration to aiopg. It based on dbapi extension with async wrapper and instumentor.

@sartx sartx requested a review from a team June 10, 2020 09:16
@sartx
Copy link
Contributor Author

sartx commented Jun 10, 2020

I signed it

@sartx sartx changed the title Add instrumentation for aiopg WIP: Add instrumentation for aiopg Jun 10, 2020
@sartx sartx changed the title WIP: Add instrumentation for aiopg Add instrumentation for aiopg Jun 11, 2020
@cnnradams
Copy link
Member

It seems like a significant amount of the instrumentation code here very similar to what's in the dbapi instrumentation library. I understand that because aiopg is async we won't be able to fully use the dbapi helpers, but is there some way to break down the dbapi functions so aiopg can generally use them instead of copying the code? Would make this much easier to review

@sartx
Copy link
Contributor Author

sartx commented Jun 22, 2020

@cnnradams
Thanks for comment. It really looks like that it is better to extend dbapi integration. Firstly I tried to do this. But in the implementation I met a couple of difficulties:

  1. Async function can be called only from another async function. In dbapi def wrap_connect_ is a synchronous function.
    In def wrap_connect appears need to determine what kind of function we want to wrap. It can be done by funciton iscoroutinefunction in inspect module - it works only with async def syntax, but aiopg use it only since 1.0.0 version, older versions use @coroutine decorators and it becomes more difficult to define that funciton is async.
    Another way is manual throwing arg, which defines type of wrapped function - but it needs throwing it from trace_integration and wrap_connect.

  2. dbapi extension uses wrapt lib, which does not wrap async magic methods, as 'await', 'aenter', 'aexit' and so on. I extended ObjectProxy by creating AsyncObjectProxy. Replace ObjectProxy by AsyncObjectProxy in dbapi does not look like a big problem.

  3. In DatabaseApiIntegration we need to add async version fordef wrapped_connection function

  4. We can not reuse def get_traced_connection_proxy function, because calling wrapped cursor function won't wrap def _cursor function of aiopg, but we need to do it.

  5. Most of classes need to add async version for methods, as TracedCursorProxy, TracedCursor.

All this things will make dbapi extension is too complex as for me.

But I have an idea how to reduce duplicated code.

  1. Inherit AiopgIntegration from DatabaseApiIntegration - it give us opportunity to reuse def get_connection_attributes
  2. In dbapi extension take out from method TracedCursor.traced_execution setting span attributes to separate method
    If you agree, then I will do this revision as part of this PR

@cnnradams
Copy link
Member

cnnradams commented Jun 23, 2020

But I have an idea how to reduce duplicated code.

  1. Inherit AiopgIntegration from DatabaseApiIntegration - it give us opportunity to reuse def get_connection_attributes
  2. In dbapi extension take out from method TracedCursor.traced_execution setting span attributes to separate method
    If you agree, then I will do this revision as part of this PR

Sounds like a good plan to me! Also thanks for the explanation, it definitely seems like trying to use all of dbapi would be way too much hassle for what its worth.

@sartx sartx requested review from cnnradams and removed request for a team July 10, 2020 11:49
@sartx sartx requested a review from cnnradams July 21, 2020 04:21
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 PR, I updated the package name to opentelemetry-instrumentation-aiopg.

@sartx
Copy link
Contributor Author

sartx commented Jul 22, 2020

@codeboten @lzchen Thanks for review
@cnnradams Thanks for promoting of this PR. Now it is waiting your review.

@codeboten codeboten merged commit f2c6c85 into open-telemetry:master Jul 24, 2020
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
* chore: update Release Schedule

* chore: target -> Release
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.

4 participants