Adding DB API integration + MySQL connector integration #264

hectorhdzg · 2019-11-04T18:21:37Z

Took OpenCensus implementation as starting point for this, there are significant changes now including different fields to store in the Span object and monkey patching process.
https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-mysql

MySQL Connector docs
https://dev.mysql.com/doc/connector-python/en/

c24t

The approach looks good, but is this wrapping the right library?

ext/opentelemetry-ext-mysql/src/opentelemetry/ext/mysql/version.py

ext/opentelemetry-ext-mysql/setup.cfg

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

toumorokoshi · 2019-11-05T17:46:08Z

ext/opentelemetry-ext-mysql/tests/test_mysql_integration.py

+            raise Exception("Test Exception")
+
+
+class MockSpan:


are MockSpan and MockTracer needed? I feel like we have a no-op tracer now with the API.

I wanted to test the integration code is creating the span correctly, API tracer is no-op so it will not create anything, are you suggesting to use unittest.Mock with a side effect or some other pattern to test here?

Using no-op tracer in tests but still using MockSpan to check all attributes are set up correctly

could use a Mock object which takes the Tracer as as a spec: https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock

if the purpose here is to provide a general test utility for tracers, I guess I'd rather see it shared in something like opentelemetry-api. But I'm not sure if sharing test fixtures across projects is an accepted policy.

Mocking tracer using mock library now, for Span I prefer to create my own mock class so is easier to test the integration adding correct attributes to the Span

hectorhdzg · 2019-11-05T19:03:12Z

@c24t I decided to use wrapt library to monkey patch because it was easy to understand and apply it here, I was interested in not altering original code including signatures and types, I gave a try with functools as well, some other integration is using it already in this project, but had some issues patching the cursor methods. I'm open to suggestions and ideas for this

hectorhdzg · 2019-11-11T23:07:51Z

Added dbapi extension to be shared by all libraries using Python Database API specification
https://www.python.org/dev/peps/pep-0249/

#265 (comment)

ext/opentelemetry-ext-mysql/tests/test_mysql_integration.py

Adding executemany and callproc wrap

codecov-io · 2019-11-14T01:40:26Z

Codecov Report

Merging #264 into master will not change coverage.
The diff coverage is 89.44%.

@@           Coverage Diff           @@
##           master     #264   +/-   ##
=======================================
  Coverage   84.82%   84.82%           
=======================================
  Files          38       38           
  Lines        1839     1839           
  Branches      217      217           
=======================================
  Hits         1560     1560           
  Misses        214      214           
  Partials       65       65

Impacted Files	Coverage Δ
...sdk/src/opentelemetry/sdk/trace/export/__init__.py	`86.79% <ø> (ø)`	⬆️
opentelemetry-api/src/opentelemetry/util/loader.py	`81.25% <ø> (ø)`	⬆️
...ntelemetry-api/src/opentelemetry/trace/sampling.py	`85.1% <ø> (ø)`	⬆️
...etry-sdk/src/opentelemetry/sdk/metrics/__init__.py	`99.05% <ø> (ø)`	⬆️
...try-ext-wsgi/src/opentelemetry/ext/wsgi/version.py	`100% <100%> (ø)`	⬆️
.../src/opentelemetry/ext/opentracing_shim/version.py	`100% <100%> (ø)`	⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`68.18% <100%> (ø)`	⬆️
...src/opentelemetry/ext/opentracing_shim/__init__.py	`95.9% <100%> (ø)`	⬆️
...sts/src/opentelemetry/ext/http_requests/version.py	`100% <100%> (ø)`	⬆️
...pentelemetry-sdk/src/opentelemetry/sdk/__init__.py	`100% <100%> (ø)`	⬆️
... and 15 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83d95e7...26e4983. Read the comment docs.

hectorhdzg · 2019-11-14T22:56:31Z

@c24t we talked about using wrapt in today OT SIG meeting, everyone agreed it was fine to use this library, is actually heavily used in OpenTracing integrations https://github.com/opentracing-contrib?utf8=%E2%9C%93&q=python&type=&language=
Let me know if you have strong opinions about this and i would appreciate if you can take a look a the PR again, I added a bunch of stuff since your last comment

Oberon00

Two blocking comments:

Connection properties being potentially overriden when calling connect multiple times: Adding DB API integration + MySQL connector integration #264 (comment)
Accidental exception swallowing: Adding DB API integration + MySQL connector integration #264 (comment)

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

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

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

DB integrations like psycopg

other integrations Updated versions Added capabilities to specify connection attribute names

…ry-python into newMaster

I don't have time to look into this currently.

hectorhdzg · 2019-12-12T18:01:34Z

@Oberon00 all comments have been resolved now, let me know if you have more feedback, thanks

lzchen · 2019-12-16T18:23:21Z

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

+    """Integrate with MySQL Connector/Python library.
+       https://dev.mysql.com/doc/connector-python/en/
+    """
+    connection_attributes = {


Maybe put this outside the function so it doesn't have to be defined more than once?

lzchen

LGTM! Thanks for powering through this.

toumorokoshi

Overall looks really good! There's a couple small things (like using the global logging module) that I think should be addressed, but by and large the code (and tests) look great.

ext/opentelemetry-ext-dbapi/README.rst

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

toumorokoshi · 2019-12-29T13:45:47Z

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

+        if tracer is None:
+            raise ValueError("The tracer is not provided.")
+        self.connection_attributes = connection_attributes
+        if self.connection_attributes is None:


Just to clarify: is it possible that these connection_attributes don't reflect the actual connection established, since this is wrapping the connect method and different args can be passed there?

should there be some documentation added about what the attribute names should be, in that case? I could imagine people using slightly different names for the attributes which could lead to quite a mismatch in key names.

Well the attributes names depends entirely on the dbapi library being used, if we support more popular libraries the less likely people will make mistakes, I agree having more documentation is always good so I added more details

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

toumorokoshi

Great! Thanks for addressing feedback.

toumorokoshi · 2020-01-05T17:41:43Z

amazing! Thanks @hectorhdzg!

Adding MySQL Connector integration

2c2f2ba

hectorhdzg requested review from a-feld, c24t, carlosalberto, lzchen, Oberon00, reyang and toumorokoshi as code owners November 4, 2019 18:21

Fix lint issues

47824f2

c24t reviewed Nov 5, 2019

View reviewed changes

ext/opentelemetry-ext-mysql/src/opentelemetry/ext/mysql/version.py Outdated Show resolved Hide resolved

ext/opentelemetry-ext-mysql/setup.cfg Outdated Show resolved Hide resolved

ext/opentelemetry-ext-mysql/setup.cfg Outdated Show resolved Hide resolved

toumorokoshi reviewed Nov 5, 2019

View reviewed changes

hectorhdzg added 6 commits November 5, 2019 12:07

Addressing comments

b5d81b8

Fixing issue with typings

f9f03a8

Using json to store query params

10f3f53

Using tuple query params in tests instead of dict

e6158c8

Format issue

315e583

Adding ext.dbapi package

cb8a835

Updating readme

857eed6

hectorhdzg commented Nov 12, 2019

View reviewed changes

ext/opentelemetry-ext-mysql/tests/test_mysql_integration.py Show resolved Hide resolved

hectorhdzg added ext tracing labels Nov 13, 2019

hectorhdzg and others added 2 commits November 13, 2019 17:26

Adding more unit tests

aefc210

Adding executemany and callproc wrap

Merge branch 'master' into newMaster

ebee38c

Oberon00 previously requested changes Nov 20, 2019

View reviewed changes

Oberon00 mentioned this pull request Nov 20, 2019

Adding psycopg2 integration #298

Merged

Addressing comments

9cfc808

lzchen reviewed Nov 20, 2019

View reviewed changes

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

hectorhdzg and others added 5 commits November 20, 2019 14:07

Adding check for cursor method

d47adcd

Addressing comments

ce8e261

Merge branch 'master' into newMaster

d7a1d07

Fix format issue

03a25b4

Refactor span creation so it can be used by other

4b0e20f

DB integrations like psycopg

hectorhdzg requested a review from a team December 10, 2019 00:18

a-feld removed their request for review December 10, 2019 00:56

hectorhdzg and others added 2 commits December 11, 2019 14:36

Refactored integration for easier reuse by

7a66815

other integrations Updated versions Added capabilities to specify connection attribute names

Merge branch 'master' into newMaster

7952e63

hectorhdzg changed the title ~~Adding MySQL Connector integration~~ Adding DB API integration + MySQL connector integration Dec 11, 2019

hectorhdzg added 4 commits December 11, 2019 14:51

Updating to 0.4.dev0

28108a1

Merge branch 'newMaster' of https://github.com/hectorhdzg/opentelemet…

93468c0

…ry-python into newMaster

Fixing invalid syntax in parameters

3f0c082

Ignore lint abstract-method

d480718

hectorhdzg added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Dec 12, 2019

lzchen reviewed Dec 16, 2019

View reviewed changes

lzchen approved these changes Dec 16, 2019

View reviewed changes

toumorokoshi suggested changes Dec 29, 2019

View reviewed changes

hectorhdzg and others added 3 commits January 2, 2020 12:02

Addressing comments

83d95e7

Merge branch 'master' into newMaster

f5fa462

Fixing tests

0a37e3f

hectorhdzg mentioned this pull request Jan 2, 2020

Use current logger instead of global #351

Merged

toumorokoshi approved these changes Jan 3, 2020

View reviewed changes

Addressing feedback

26e4983

toumorokoshi merged commit b01f7e8 into open-telemetry:master Jan 5, 2020

hectorhdzg deleted the newMaster branch March 24, 2020 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding DB API integration + MySQL connector integration #264

Adding DB API integration + MySQL connector integration #264

hectorhdzg commented Nov 4, 2019

c24t left a comment

toumorokoshi Nov 5, 2019

hectorhdzg Nov 5, 2019

hectorhdzg Nov 14, 2019

toumorokoshi Nov 14, 2019

toumorokoshi Nov 14, 2019

hectorhdzg Nov 20, 2019

hectorhdzg commented Nov 5, 2019

hectorhdzg commented Nov 11, 2019

codecov-io commented Nov 14, 2019 •

edited

Loading

hectorhdzg commented Nov 14, 2019

Oberon00 left a comment •

edited

Loading

hectorhdzg commented Dec 12, 2019

lzchen Dec 16, 2019

lzchen left a comment

toumorokoshi left a comment

toumorokoshi Dec 29, 2019

hectorhdzg Jan 2, 2020

toumorokoshi left a comment

toumorokoshi commented Jan 5, 2020

Adding DB API integration + MySQL connector integration #264

Adding DB API integration + MySQL connector integration #264

Conversation

hectorhdzg commented Nov 4, 2019

c24t 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

hectorhdzg commented Nov 5, 2019

hectorhdzg commented Nov 11, 2019

codecov-io commented Nov 14, 2019 • edited Loading

Codecov Report

hectorhdzg commented Nov 14, 2019

Oberon00 left a comment • edited Loading

Choose a reason for hiding this comment

hectorhdzg commented Dec 12, 2019

Choose a reason for hiding this comment

lzchen left a comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi commented Jan 5, 2020

codecov-io commented Nov 14, 2019 •

edited

Loading

Oberon00 left a comment •

edited

Loading