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

ext/psycopg2: Implement BaseInstrumentor interface #694

Merged
merged 2 commits into from
May 21, 2020

Conversation

cnnradams
Copy link
Member

@cnnradams cnnradams commented May 14, 2020

  1. Implemented BaseInstrumentor interface to enable auto-instrumentation
  2. Added integration tests (same tests as other db integrations)

Since the psycopg2 connection object can't have properties added to it (for example, self._db_api_integration = db_api_integration throws an error), I instead closed these properties around the tracing wrappers.

Resolves #633

@cnnradams cnnradams requested a review from a team May 14, 2020 17:35
@cnnradams cnnradams force-pushed the psycopg2-baseinstrumentor branch 2 times, most recently from 4fb5eb9 to 832f604 Compare May 14, 2020 18:24
@codeboten codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 14, 2020
@ocelotl
Copy link
Contributor

ocelotl commented May 15, 2020

Hello @cnnradams

What error do you get when you

  1. Implemented BaseInstrumentor interface to enable auto-instrumentation
  2. Added integration tests (same tests as other db integrations)

Since the psycopg2 connection object can't have properties added to it (for example, self._db_api_integration = db_api_integration throws an error), I instead closed these properties around the tracing wrappers.

Resolves #633

Hey, @cnnradams, what is the error you mention above?

@cnnradams
Copy link
Member Author

Hello @cnnradams

What error do you get when you

  1. Implemented BaseInstrumentor interface to enable auto-instrumentation
  2. Added integration tests (same tests as other db integrations)

Since the psycopg2 connection object can't have properties added to it (for example, self._db_api_integration = db_api_integration throws an error), I instead closed these properties around the tracing wrappers.
Resolves #633

Hey, @cnnradams, what is the error you mention above?

postgres/test_psycopg_functional.py:44: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../.tox/docker-tests/lib/python3.6/site-packages/wrapt/wrappers.py:567: in __call__
    args, kwargs)
../../opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py:118: in _wrap_connect
    return db_integration.wrapped_connection(wrapped, args, kwargs)
../../opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py:224: in wrapped_connection
    return TracedConnectionProxy(connection, self)
../../opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py:268: in __init__
    self._db_api_integration = db_api_integration
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <TracedConnectionProxy at 0x7faae3621208 for connection at 0x7faae3928660>
name = '_db_api_integration'
value = <opentelemetry.ext.dbapi.DatabaseApiIntegration object at 0x7faae3647898>

    def __setattr__(self, name, value):
        if name.startswith('_self_'):
            object.__setattr__(self, name, value)
    
        elif name == '__wrapped__':
            object.__setattr__(self, name, value)
            try:
                object.__delattr__(self, '__qualname__')
            except AttributeError:
                pass
            try:
                object.__setattr__(self, '__qualname__', value.__qualname__)
            except AttributeError:
                pass
    
        elif name == '__qualname__':
            setattr(self.__wrapped__, name, value)
            object.__setattr__(self, name, value)
    
        elif hasattr(type(self), name):
            object.__setattr__(self, name, value)
    
        else:
>           setattr(self.__wrapped__, name, value)
E           AttributeError: 'psycopg2.extensions.connection' object has no attribute '_db_api_integration'

../../../.tox/docker-tests/lib/python3.6/site-packages/wrapt/wrappers.py:190: AttributeError

basically, you can't seem to assign new properties to objects that were originally defined in C (same issue happens with sqlite3, does not happen with mysql since its connection was defined in python)

@@ -221,7 +221,7 @@ def wrapped_connection(
"""
connection = connect_method(*args, **kwargs)
self.get_connection_attributes(connection)
return TracedConnectionProxy(connection, self)
return get_traced_connection_proxy(connection, self)
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason for these wrapper functions?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes - see my above comment. tl;dr: Can't assign new properties to the psycopg2 connection object

trace_integration()
cnx = psycopg2.connect(database="test")
self.assertIsNotNone(cnx.cursor_factory)
class TestPostgresqlIntegration(TestBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Good job using the TestBase!

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

@cnnradams cnnradams force-pushed the psycopg2-baseinstrumentor branch from 832f604 to 0d1d543 Compare May 20, 2020 19:37
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.

LGTM! Thanks for the PR

@codeboten codeboten merged commit 2d9c8df into open-telemetry:master May 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ext/psycopg2: Implement BaseInstrumentor interface
4 participants