ext/pymysql: Add Instrumentor #611

mauriciovasquezbernal · 2020-04-23T21:08:21Z

Implement a way to uninstrument dbapi connections
Update pymysql to implement the instrumentor interface and provide a way to uninstrument

How to try it:

# install
pip install -e ext/opentelemetry-ext-pymysql/
# set up test db
opentelemetry-ext-docker-tests/tests
docker-compose up

pymysql_example.py

# Configuration code
from opentelemetry import trace

from opentelemetry.sdk.trace.export import (
    SimpleExportSpanProcessor,
    ConsoleSpanExporter,
)

trace.get_tracer_provider().add_span_processor(
    SimpleExportSpanProcessor(ConsoleSpanExporter())
)

# Real example starts here
import os

import pymysql

MYSQL_USER = os.getenv("MYSQL_USER ", "testuser")
MYSQL_PASSWORD = os.getenv("MYSQL_PASSWORD ", "testpassword")
MYSQL_HOST = os.getenv("MYSQL_HOST ", "localhost")
MYSQL_PORT = int(os.getenv("MYSQL_PORT ", "3306"))
MYSQL_DB_NAME = os.getenv("MYSQL_DB_NAME ", "opentelemetry-tests")

_connection = None
cursor = None
_connection = pymysql.connect(
   user=MYSQL_USER,
   password=MYSQL_PASSWORD,
   host=MYSQL_HOST,
   port=MYSQL_PORT,
   database=MYSQL_DB_NAME,
)
cursor = _connection.cursor()

cursor.execute("CREATE TABLE IF NOT EXISTS test (id integer)")

data = ((1,), (2,), (3,))
stmt = "INSERT INTO test (id) VALUES (%s)"
cursor.executemany(stmt, data)

Run the example:

OPENTELEMETRY_PYTHON_TRACER_PROVIDER=sdk_tracer_provider opentelemetry-auto-instrumentation pymysql_example.py

If everything went fine you see some spans on the terminal.

Unwrap connect restores the original behaviour of the connect method.

- Implement instrumentor interface to be usable by autoinstrumentation - Provide mechanishm to uninstrument

hectorhdzg · 2020-04-24T22:23:35Z

ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py

+        if isinstance(conn, wrapt.ObjectProxy):
+            setattr(connect_module, connect_method_name, conn.__wrapped__)
+
+
 class DatabaseApiIntegration:


Any reason to not use BaseInstrumentor in here as well?, people could be using dbapi instrumentor directly

BaseInstrumentor is used to implement an interface that allows to instrument all the calls made to a given library, for instance pymysql. AFAIU dbapi is a specification and not a library, so having an instrumentor here doesn't make sense. If a person wants to use dpapi directly, they should use the wrap_connect and unwrap_connect functions.

Should document this? In the docstrings trace_integration() is the recommended way to instrument. Perhaps we should expose an API method to uninstrument (since there isn't really a semantic similarity between trace_integration and unwrap_connect.

The PyMySQL docstrings was wrong, trace_integration doesn't exist anymore.
PymysqlInstrumentor offers two methods, instrument and uninstrument.

I'm not sure what to do with the trace_integration on dpapi, it's very similar to wrap_connect but creates a tracer before. Maybe we could remove it and let the user play directly with wrap_connect...

ext/opentelemetry-ext-pymysql/tests/test_pymysql_integration.py

ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py

hectorhdzg

LGTM

ocelotl

Just some suggestions. 👍

ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py

ext/opentelemetry-ext-pymysql/tests/test_pymysql_integration.py

ocelotl · 2020-04-27T22:25:57Z

ext/opentelemetry-ext-pymysql/tests/test_pymysql_integration.py

+    def tearDown(self):
+        super().tearDown()
+        # ensure that it's uninstrumented if some of the tests fail
+        PymysqlInstrumentor().uninstrument()


Please enclose this between logging.disable calls to suppress unnecessary logging:

from logging import disable, WARNING, NOTSET ... def tearDown(self): ... disable(WARNING) PymysqlInstrumentor().uninstrument() disable(NOTSET)

Thanks, I followed a similar approach.

lzchen · 2020-04-28T03:23:19Z

ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py

@@ -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)


User is not returned as string in some cases?

Right, PyMySQL returns bytes and it was causing problems in the console exporter. I opened an issue about it #623.

ext/opentelemetry-ext-pymysql/CHANGELOG.md

ext/opentelemetry-ext-pymysql/src/opentelemetry/ext/pymysql/__init__.py

lzchen · 2020-04-28T03:29:19Z

tox.ini

  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]


Is the [test] for using TestBase? How does this work?

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

ext/opentelemetry-ext-pymysql/src/opentelemetry/ext/pymysql/__init__.py

lzchen

LGTM. Some non-blocking comments.

…__.py Co-Authored-By: Diego Hurtado <ocelotl@users.noreply.github.com>

- update changelog with link to previous PR - disable logging to avoid noise in teardown - rename to PyMySQLInstrumentor

mauriciovasquezbernal added ext instrumentation Related to the instrumentation of third party libraries or frameworks labels Apr 23, 2020

mauriciovasquezbernal added 6 commits April 24, 2020 16:55

ext/pymysql: Add test requirements section in setup

a27027b

Use TestBase in integration tests

6ddbeb5

ext/dbapi: Implement unwrap connect

2ab5e5b

Unwrap connect restores the original behaviour of the connect method.

ext/pymysql: Implement instrumentor interface

3a54847

- Implement instrumentor interface to be usable by autoinstrumentation - Provide mechanishm to uninstrument

update changelog

7123ebb

typo

7ea6b3b

mauriciovasquezbernal marked this pull request as ready for review April 24, 2020 21:56

mauriciovasquezbernal requested a review from a team April 24, 2020 21:56

hectorhdzg reviewed Apr 24, 2020

View reviewed changes

ext/opentelemetry-ext-pymysql/tests/test_pymysql_integration.py Show resolved Hide resolved

hectorhdzg reviewed Apr 24, 2020

View reviewed changes

ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init__.py Show resolved Hide resolved

mauriciovasquezbernal added 9 commits April 27, 2020 08:41

Add test for custom tracer provider

c76ade0

Merge branch 'master' into mauricio/instrumentor-pymysql

03fa1d5

Merge branch 'master' into mauricio/instrumentor-pymysql

e0be0de

fix tests

4a4c167

Merge branch 'master' into mauricio/instrumentor-pymysql

7a070af

update changelog

2722ebb

Merge branch 'master' into mauricio/instrumentor-pymysql

3430446

enable tests

9f3120e

ext/dbapi: Add tests for wrap_connect and unwrap_connect

249d87f

hectorhdzg approved these changes Apr 27, 2020

View reviewed changes

ocelotl approved these changes Apr 27, 2020

View reviewed changes

lzchen reviewed Apr 28, 2020

View reviewed changes

ext/opentelemetry-ext-pymysql/CHANGELOG.md Show resolved Hide resolved

lzchen reviewed Apr 28, 2020

View reviewed changes

ext/opentelemetry-ext-pymysql/src/opentelemetry/ext/pymysql/__init__.py Outdated Show resolved Hide resolved

lzchen reviewed Apr 28, 2020

View reviewed changes

ext/opentelemetry-ext-pymysql/src/opentelemetry/ext/pymysql/__init__.py Show resolved Hide resolved

lzchen approved these changes Apr 28, 2020

View reviewed changes

mauriciovasquezbernal added the do not merge label Apr 28, 2020

mauriciovasquezbernal and others added 2 commits April 28, 2020 09:20

Update ext/opentelemetry-ext-dbapi/src/opentelemetry/ext/dbapi/__init…

3625a85

…__.py Co-Authored-By: Diego Hurtado <ocelotl@users.noreply.github.com>

further changes

7712252

- update changelog with link to previous PR - disable logging to avoid noise in teardown - rename to PyMySQLInstrumentor

mauriciovasquezbernal removed the do not merge label Apr 28, 2020

update class name in entrypoint

00e29a3

mauriciovasquezbernal mentioned this pull request Apr 28, 2020

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

Merged

Merge branch 'master' into mauricio/instrumentor-pymysql

bede60b

c24t approved these changes Apr 29, 2020

View reviewed changes

c24t merged commit 2992950 into open-telemetry:master Apr 29, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ext/pymysql: Add Instrumentor #611

ext/pymysql: Add Instrumentor #611

mauriciovasquezbernal commented Apr 23, 2020 •

edited

Loading

hectorhdzg Apr 24, 2020 •

edited

Loading

mauriciovasquezbernal Apr 27, 2020

lzchen Apr 28, 2020

mauriciovasquezbernal Apr 28, 2020

hectorhdzg left a comment

ocelotl left a comment

ocelotl Apr 27, 2020

mauriciovasquezbernal Apr 28, 2020

lzchen Apr 28, 2020

mauriciovasquezbernal Apr 28, 2020

lzchen Apr 28, 2020

mauriciovasquezbernal Apr 28, 2020

lzchen left a comment

ext/pymysql: Add Instrumentor #611

ext/pymysql: Add Instrumentor #611

Conversation

mauriciovasquezbernal commented Apr 23, 2020 • edited Loading

hectorhdzg Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorhdzg left a comment

Choose a reason for hiding this comment

ocelotl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

mauriciovasquezbernal commented Apr 23, 2020 •

edited

Loading

hectorhdzg Apr 24, 2020 •

edited

Loading