-
Notifications
You must be signed in to change notification settings - Fork 595
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
ext/psycopg2: Implement BaseInstrumentor interface #694
Conversation
4fb5eb9
to
832f604
Compare
Hello @cnnradams What error do you get when you
Hey, @cnnradams, what is the error you mention above? |
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) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
ext/opentelemetry-ext-grpc/src/opentelemetry/ext/grpc/grpcext/_interceptor.py
Outdated
Show resolved
Hide resolved
trace_integration() | ||
cnx = psycopg2.connect(database="test") | ||
self.assertIsNotNone(cnx.cursor_factory) | ||
class TestPostgresqlIntegration(TestBase): |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
832f604
to
0d1d543
Compare
There was a problem hiding this 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
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