-
Notifications
You must be signed in to change notification settings - Fork 714
ext/pymysql: Add Instrumentor #611
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
Changes from 15 commits
a27027b
6ddbeb5
2ab5e5b
3a54847
7123ebb
7ea6b3b
c76ade0
03fa1d5
e0be0de
4a4c167
7a070af
2722ebb
3430446
9f3120e
249d87f
3625a85
7712252
00e29a3
bede60b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -125,6 +125,15 @@ def wrap_connect_( | |
logger.warning("Failed to integrate with DB API. %s", str(ex)) | ||
|
||
|
||
def unwrap_connect( | ||
connect_module: typing.Callable[..., any], connect_method_name: str, | ||
): | ||
if hasattr(connect_module, connect_method_name): | ||
conn = getattr(connect_module, connect_method_name) | ||
if isinstance(conn, wrapt.ObjectProxy): | ||
setattr(connect_module, connect_method_name, conn.__wrapped__) | ||
mauriciovasquezbernal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
|
||
class DatabaseApiIntegration: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any reason to not use BaseInstrumentor in here as well?, people could be using dbapi instrumentor directly There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should document this? In the docstrings There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The PyMySQL docstrings was wrong, I'm not sure what to do with the |
||
def __init__( | ||
self, | ||
|
@@ -184,7 +193,7 @@ def get_connection_attributes(self, connection): | |
self.name += "." + self.database | ||
user = self.connection_props.get("user") | ||
if user is not None: | ||
self.span_attributes["db.user"] = user | ||
self.span_attributes["db.user"] = str(user) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. User is not returned as string in some cases? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, PyMySQL returns bytes and it was causing problems in the console exporter. I opened an issue about it #623. |
||
host = self.connection_props.get("host") | ||
if host is not None: | ||
self.span_attributes["net.peer.name"] = host | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,5 @@ | ||
# Changelog | ||
|
||
## Unreleased | ||
|
||
- Implement instrumentor interface ([#611](https://github.com/open-telemetry/opentelemetry-python/pull/611)) | ||
mauriciovasquezbernal marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,22 +17,61 @@ | |
import pymysql | ||
|
||
import opentelemetry.ext.pymysql | ||
from opentelemetry.ext.pymysql import trace_integration | ||
from opentelemetry.ext.pymysql import PymysqlInstrumentor | ||
from opentelemetry.sdk import resources | ||
from opentelemetry.test.test_base import TestBase | ||
|
||
|
||
class TestPyMysqlIntegration(TestBase): | ||
mauriciovasquezbernal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
def test_trace_integration(self): | ||
with mock.patch("pymysql.connect"): | ||
trace_integration() | ||
cnx = pymysql.connect(database="test") | ||
cursor = cnx.cursor() | ||
query = "SELECT * FROM test" | ||
cursor.execute(query) | ||
def tearDown(self): | ||
super().tearDown() | ||
# ensure that it's uninstrumented if some of the tests fail | ||
mauriciovasquezbernal marked this conversation as resolved.
Show resolved
Hide resolved
|
||
PymysqlInstrumentor().uninstrument() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please enclose this between from logging import disable, WARNING, NOTSET
...
def tearDown(self):
...
disable(WARNING)
PymysqlInstrumentor().uninstrument()
disable(NOTSET) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I followed a similar approach. |
||
|
||
@mock.patch("pymysql.connect") | ||
# pylint: disable=unused-argument | ||
def test_instrumentor(self, mock_connect): | ||
PymysqlInstrumentor().instrument() | ||
|
||
cnx = pymysql.connect(database="test") | ||
cursor = cnx.cursor() | ||
query = "SELECT * FROM test" | ||
cursor.execute(query) | ||
|
||
spans_list = self.memory_exporter.get_finished_spans() | ||
self.assertEqual(len(spans_list), 1) | ||
span = spans_list[0] | ||
|
||
# Check version and name in span's instrumentation info | ||
self.check_span_instrumentation_info(span, opentelemetry.ext.pymysql) | ||
|
||
# check that no spans are generated after uninstrument | ||
PymysqlInstrumentor().uninstrument() | ||
|
||
cnx = pymysql.connect(database="test") | ||
cursor = cnx.cursor() | ||
query = "SELECT * FROM test" | ||
cursor.execute(query) | ||
|
||
spans_list = self.memory_exporter.get_finished_spans() | ||
self.assertEqual(len(spans_list), 1) | ||
|
||
@mock.patch("pymysql.connect") | ||
# pylint: disable=unused-argument | ||
def test_custom_tracer_provider(self, mock_connect): | ||
resource = resources.Resource.create({}) | ||
result = self.create_tracer_provider(resource=resource) | ||
tracer_provider, exporter = result | ||
|
||
PymysqlInstrumentor().instrument(tracer_provider=tracer_provider) | ||
|
||
cnx = pymysql.connect(database="test") | ||
cursor = cnx.cursor() | ||
query = "SELECT * FROM test" | ||
cursor.execute(query) | ||
|
||
spans_list = exporter.get_finished_spans() | ||
self.assertEqual(len(spans_list), 1) | ||
span = spans_list[0] | ||
|
||
self.assertIs(span.resource, resource) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -183,8 +183,9 @@ commands_pre = | |
psycopg2: pip install {toxinidir}/ext/opentelemetry-ext-dbapi | ||
psycopg2: pip install {toxinidir}/ext/opentelemetry-ext-psycopg2 | ||
|
||
pymysql: pip install {toxinidir}/opentelemetry-auto-instrumentation | ||
pymysql: pip install {toxinidir}/ext/opentelemetry-ext-dbapi | ||
pymysql: pip install {toxinidir}/ext/opentelemetry-ext-pymysql | ||
pymysql: pip install {toxinidir}/ext/opentelemetry-ext-pymysql[test] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the [test] for using TestBase? How does this work? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's a way to tell pip to install opentelemetry-ext-pymysql and the dependencies on the extra test section: https://github.com/open-telemetry/opentelemetry-python/pull/611/files#diff-3874eac15d9a093a3db07c9283d97d12R48 |
||
|
||
redis: pip install {toxinidir}/opentelemetry-auto-instrumentation | ||
redis: pip install {toxinidir}/ext/opentelemetry-ext-redis[test] | ||
|
Uh oh!
There was an error while loading. Please reload this page.