PyMySQL Integration by lzchen · Pull Request #504 · open-telemetry/opentelemetry-python

lzchen · 2020-03-17T21:02:16Z

Integration for PyMySQL.
Fixes some documentation as well for other db integrations.
Leverages dbapi.

ocelotl

A few changes requested only 👍

ext/opentelemetry-ext-dbapi/README.rst

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

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

hectorhdzg

LGTM, like mentioned in SIG meeting this morning it would be good to have functional tests for this, would be good to create a task for it, thanks for fixing issues in other integrations

…try-python into pymy

lzchen · 2020-04-13T23:18:59Z

@hectorhdzg I've added functional tests, they are basically the exact same as Mysql except using the PyMySQL library

@ocelotl I believe I've addressed all your comments

ocelotl

I just added a few comments, but the main changes I will request is to integrate this with the current instrumentation API. This is mainly a matter of creating a class that inherits from BaseIinstrumentor. The implementation of its _instrument method should be just what you already have in trace_integration if I am not mistaken.

You'll probably need to pass a tracer argument to _instrument, I am updating the API here to do that. Please reach out if you have any questions.

ocelotl · 2020-04-17T17:27:41Z

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

@@ -146,6 +146,9 @@ def get_connection_attributes(self, connection):
        self.name = self.database_component
        self.database = self.connection_props.get("database", "")


Suggested change

self.database = self.connection_props.get("database", "")

self.database = self.connection_props.get("database", b"")

ocelotl · 2020-04-17T17:36:03Z

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

+            if hasattr(self.database, "decode"):
+                self.database = self.database.decode(errors="ignore")


Suggested change

if hasattr(self.database, "decode"):

self.database = self.database.decode(errors="ignore")

self.database = self.database.decode(errors="ignore")

mauriciovasquezbernal

Overall speaking the PR looks good.

You can address my comments before merging or we can address then in new PRs.

mauriciovasquezbernal · 2020-04-20T20:12:43Z

docs/ext/pymysql/pymysql.rst

+.. include:: ../../../ext/opentelemetry-ext-pymysql/README.rst
+
+


Currently we're not including that readme in these files anymore.

ext/opentelemetry-ext-mysql/setup.cfg

ext/opentelemetry-ext-pymysql/setup.cfg

ext/opentelemetry-ext-pymysql/setup.py

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

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

mauriciovasquezbernal · 2020-04-20T20:26:45Z

ext/opentelemetry-ext-docker-tests/tests/pymysql/test_pymysql_functional.py

+MYSQL_DB_NAME = os.getenv("MYSQL_DB_NAME ", "opentelemetry-tests")
+
+
+class TestFunctionalPyMysql(unittest.TestCase):


Also here, using TestBase could help a lot.

mauriciovasquezbernal · 2020-04-20T20:32:17Z

ext/opentelemetry-ext-docker-tests/tests/pymysql/test_pymysql_functional.py

+        with self._tracer.start_as_current_span("rootSpan"), self.assertRaises(
+            Exception
+        ):


I think two separated with statements would be a lot clearer in this particular case.

Committing some of my suggestions that are obvious.

mauriciovasquezbernal

We want to add auto-instrumentation support for PyMySQL soon. I talked to @lzchen internally and we agree to merge this PR and then I could continue with the autoninstrumentation of it.
In order to unblock it, I merged some of my obvious comments, merge master and solved the conflicts, and also updated some parts to make it work with the current status (for instance, tracer_integration taking a tracer provider instead of a tracer). I'll do more changes when implementing the instrumentor interface in follow up PRs.

ocelotl

The adaptation with the instrumentation class will be done later, marking it as good to go.

c24t · 2020-03-19T15:24:47Z

ext/opentelemetry-ext-pymysql/README.rst

@@ -0,0 +1,33 @@
+OpenTelemetry PyMySQL integration
+===============================


Suggested change

===============================

=================================

Integration for PyMySQL. Fixes some documentation as well for other db integrations. Leverages dbapi. Co-authored-by: Mauricio Vásquez <mauricio@kinvolk.io>

pymy

de2fda9

lzchen requested a review from a team March 17, 2020 21:02

lzchen added 6 commits March 17, 2020 14:17

fix lint

789eaed

tox

69af52d

dbapi

7d25e82

Fix import

0037579

add path

71c4754

fix name

225bb4c

lzchen added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Mar 18, 2020

ocelotl suggested changes Mar 19, 2020

View reviewed changes

ext/opentelemetry-ext-dbapi/README.rst Outdated Show resolved Hide resolved

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

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

hectorhdzg reviewed Mar 19, 2020

View reviewed changes

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

hectorhdzg approved these changes Mar 19, 2020

View reviewed changes

lzchen added 2 commits March 20, 2020 10:54

Merge branch 'master' of https://github.com/open-telemetry/openteleme…

0512fb5

…try-python into pymy

address comments

6e2f49f

lzchen requested a review from ocelotl March 20, 2020 19:03

lzchen added 3 commits April 13, 2020 14:18

merge

4464472

functional tests

a7b82c3

fix test

5d16a3a

mauriciovasquezbernal added the ext label Apr 17, 2020

codeboten added the instrumentation Related to the instrumentation of third party libraries or frameworks label Apr 17, 2020

ocelotl suggested changes Apr 17, 2020

View reviewed changes

mauriciovasquezbernal reviewed Apr 20, 2020

View reviewed changes

mauriciovasquezbernal added 5 commits April 23, 2020 09:02

Apply suggestions from code review

f6acff9

Committing some of my suggestions that are obvious.

Merge branch 'master' into pymy

c072146

undo change in python version for lint

6e6be34

ext/pymysql: Take TracerProvider instead of tracer

0b0132c

quick fixes for documentation

a2bf714

mauriciovasquezbernal approved these changes Apr 23, 2020

View reviewed changes

mauriciovasquezbernal mentioned this pull request Apr 24, 2020

ext/pymysql: Add Instrumentor #611

Merged

ocelotl approved these changes Apr 24, 2020

View reviewed changes

c24t approved these changes Apr 24, 2020

View reviewed changes

c24t merged commit 216bd5a into open-telemetry:master Apr 24, 2020

mauriciovasquezbernal mentioned this pull request Apr 27, 2020

PyMySQL Integration #501

Closed

		@@ -146,6 +146,9 @@ def get_connection_attributes(self, connection):
		self.name = self.database_component
		self.database = self.connection_props.get("database", "")

		if hasattr(self.database, "decode"):
		self.database = self.database.decode(errors="ignore")

		.. include:: ../../../ext/opentelemetry-ext-pymysql/README.rst

		MYSQL_DB_NAME = os.getenv("MYSQL_DB_NAME ", "opentelemetry-tests")


		class TestFunctionalPyMysql(unittest.TestCase):

		@@ -0,0 +1,33 @@
		OpenTelemetry PyMySQL integration
		===============================

	===============================
	=================================

Conversation

lzchen commented Mar 17, 2020

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hectorhdzg left a comment

Choose a reason for hiding this comment

Uh oh!

lzchen commented Apr 13, 2020

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

ocelotl Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

ocelotl Apr 17, 2020

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mauriciovasquezbernal Apr 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal Apr 20, 2020

Choose a reason for hiding this comment

Uh oh!

mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

Uh oh!

ocelotl left a comment

Choose a reason for hiding this comment

Uh oh!

c24t Mar 19, 2020

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

mauriciovasquezbernal Apr 20, 2020 •

edited

Loading