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/{dbapi,pymysql}: Implement methods to (un)-instrument connections #624

Merged
merged 9 commits into from
May 9, 2020
Merged

ext/{dbapi,pymysql}: Implement methods to (un)-instrument connections #624

merged 9 commits into from
May 9, 2020

Conversation

mauriciovasquezbernal
Copy link
Member

Draft until #611 is merged, to avoid solving many conflicts later on.

The current integrations based on dbapi allow to patch the connect function on those libraries to return an instrumented connection, however it's not possible to enable instrumentation on a connection previously created or to disable instrumentation in an instrumented connection.

This PR adds two convenience functions, instrument_connection and uninstrument_connection that allow to enable / disable instrumentation in specific connection objects.

This PR only implements the logic in PyMySQL, it can be easily done in other instrumentations once we think it's worth to be done.

@hectorhdzg @lzchen since you folks have worked in these instrumentations before I'd like to get your opinion on this.

@mauriciovasquezbernal mauriciovasquezbernal added ext instrumentation Related to the instrumentation of third party libraries or frameworks labels Apr 28, 2020
Implement to helper methods to allow users to enable / disable instrumentation
in a single connection object.
@mauriciovasquezbernal mauriciovasquezbernal marked this pull request as ready for review April 30, 2020 01:36
@mauriciovasquezbernal mauriciovasquezbernal requested a review from a team April 30, 2020 01:36
Copy link
Contributor

@ocelotl ocelotl left a comment

Choose a reason for hiding this comment

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

Just minor comments 👍


def test_uninstrument_connection(self):
connection = mock.Mock()
# Avoid get_attributes failing because can't concatenate mock
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, what do you mean?

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a function inside the instrumentation that concatenates "connection.database" to another string, if I don't set it here it fails because mock cannot be concatenated. I'll update the comment to make it more clear.

@codeboten codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label May 8, 2020
conn = getattr(connect_module, connect_method_name, None)
if isinstance(conn, wrapt.ObjectProxy):
setattr(connect_module, connect_method_name, conn.__wrapped__)


def instrument_connection(
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a need for wrap_connect anymore now that we have this? It seems this allows for better control of instrumentation, and we can recommend it as the only way to instrument DBApi connections.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh it seems that wrap_connect is the path that should be taken for auto-instrumentation, and instrument_connection should be used for programmatic correct? Similar to what we do for flask?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's right. wrap_connect is used to instrument the whole library, so every new connection created is instrumented, this can be used by the autoinstrumentation and also by users that want to enable that a the library level. On the other hand, instrument_connection allows a more granular control by enabling the instrumentation in a single connection.

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. Just a non-blocking question.

database_type,
connection_attributes=connection_attributes,
)
db_integration.get_connection_attributes(connection)
Copy link
Member

Choose a reason for hiding this comment

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

I feel like get_connection_attributes should be renamed to something that implies action, as it actually modifies the db_integration object itself in the process.

Maybe something like "load_connection_attributes"?

@toumorokoshi toumorokoshi merged commit f15230b into open-telemetry:master May 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
instrumentation Related to the instrumentation of third party libraries or frameworks 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.

5 participants