Adding psycopg2 integration #298

hectorhdzg · 2019-11-19T23:19:58Z

Took OpenCensus implementation as starting point for this one, we store more properties in traces now and we use tracer provided during trace_integration method

https://github.com/census-instrumentation/opencensus-python/tree/master/contrib/opencensus-ext-postgresql

psycopg docs
http://initd.org/psycopg/docs/index.html

#265

ext/opentelemetry-ext-postgresql/tests/test_postgresql_integration.py

Oberon00 · 2019-11-20T16:11:31Z

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

+
+    wrapt.wrap_function_wrapper(psycopg2, "connect", wrap)
+
+    class TraceCursor(pgcursor):


Why doesn't this use opentelemetry-ext-dbapi from #264? Did you run into the issue I described in https://github.com/open-telemetry/opentelemetry-python/pull/264/files#r348452864? In any case, instead of deriving from pgcursor, you might want to use a wrapt ObjectProxy https://wrapt.readthedocs.io/en/latest/wrappers.html#object-proxy. That way, you could implement a generic solution. Or you could add support for such cursor_factory instrumentations to ext-dbapi.

Another question: What happens if the user manually specifies/sets cursor_factory? Will either our or the user's cursor_factory be ignored?

I based this in current OpenCensus implementation and started working with this before adding dbapi in other PR, makes sense to reuse it as much as possible, cursor_factory in fact will be overriden like you mentioned and is current behavior in OpenCensus, filed a bug for it in OpenCensus and ensure it doesn't happen here

census-instrumentation/opencensus-python#821

cursor_factory is a not thing in the Python DB API specification, so I guess it doesn't makes sense to add it in dbapi ext, dbapi ext is currently wrapping connect method, the actual connection and the cursor, in psycopg all of these are implemented in C except connect, that is why we are hooking up to the cursor_factory, so I'm still not sure how we can reuse dbapi ext here, any suggestion would be appreciated.

Using ObjectProxy instead of pgcursor sounds like a good approach

I think this could be an extension of the DBAPI PR. looking at the chunk that actually starts a span and sets values, it's very similar to the DBApi functionality.

Although one caveat as a con is the fact that the API is slightly different in that psycopg2 accepts sql.SQL objects rather than just strings, but that could reconciled with a light wrapper on all of them.

ultimately this is an auto-instrumentation though, so as long as we don't expose APIs that people may rely on, we can probably feel safe marking this as a potential refactor.

Updated the code to use dbapi ext as much as possible, including span attributes related to connection population and actual span creation functionality

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

hectorhdzg · 2019-12-12T18:15:30Z

I would like to reuse dbapi package being added here #264, I will update this PR once is available

toumorokoshi

Overall code looks good, things that are blockers IMO are:

no basic unit test
no handling of more complex objects as the first argument of the execute call, which will lead to a casting exception.

toumorokoshi · 2019-12-30T05:35:22Z

ext/opentelemetry-ext-postgresql/setup.cfg

+# limitations under the License.
+#
+[metadata]
+name = opentelemetry-ext-postgresql


is this the right name for the package? I guess there's a few different DBAPI options available: https://docs.sqlalchemy.org/en/13/dialects/postgresql.html

Updated to psycopg2

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

toumorokoshi · 2019-12-30T05:49:57Z

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

+
+    wrapt.wrap_function_wrapper(psycopg2, "connect", wrap)
+
+    class TraceCursor(pgcursor):


I think this could be an extension of the DBAPI PR. looking at the chunk that actually starts a span and sets values, it's very similar to the DBApi functionality.

Although one caveat as a con is the fact that the API is slightly different in that psycopg2 accepts sql.SQL objects rather than just strings, but that could reconciled with a light wrapper on all of them.

ultimately this is an auto-instrumentation though, so as long as we don't expose APIs that people may rely on, we can probably feel safe marking this as a potential refactor.

hectorhdzg · 2020-01-07T00:20:14Z

@toumorokoshi and @Oberon00 I updated this to consume dbapi ext, added basic integration test, this would be super easy to test with functional tests similar to #340 would be happy to add them using similar approach, let me know if you have more feedback

codecov-io · 2020-01-07T00:35:01Z

Codecov Report

Merging #298 into master will increase coverage by 0.3%.
The diff coverage is 92.07%.

@@            Coverage Diff            @@
##           master     #298     +/-   ##
=========================================
+ Coverage   84.82%   85.13%   +0.3%     
=========================================
  Files          38       38             
  Lines        1839     1911     +72     
  Branches      217      225      +8     
=========================================
+ Hits         1560     1627     +67     
- Misses        214      219      +5     
  Partials       65       65

Impacted Files	Coverage Δ
...-ext-flask/src/opentelemetry/ext/flask/__init__.py	`88.88% <100%> (ø)`	⬆️
opentelemetry-api/src/opentelemetry/util/types.py	`100% <100%> (ø)`	⬆️
...emetry-sdk/src/opentelemetry/sdk/trace/__init__.py	`90.94% <100%> (+1.06%)`	⬆️
...ry-ext-wsgi/src/opentelemetry/ext/wsgi/__init__.py	`68.18% <33.33%> (ø)`	⬆️
...ntelemetry-api/src/opentelemetry/trace/__init__.py	`84.56% <89.36%> (+0.44%)`	⬆️
...elemetry-api/src/opentelemetry/metrics/__init__.py	`87.93% <90.9%> (+1.93%)`	⬆️

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 0ee0682...2dc0671. Read the comment docs.

ext/opentelemetry-ext-psycopg2/README.rst

lzchen · 2020-01-07T22:46:55Z

ext/opentelemetry-ext-psycopg2/tests/test_psycopg2_integration.py

+        with mock.patch("psycopg2.connect"):
+            trace_integration(tracer)
+            cnx = psycopg2.connect(database="test")
+            self.assertIsNotNone(cnx.cursor_factory)


I believe the test for trace_integration should include to see if the functions are successfully decorated.
As well, the query functions themselves and the Cursor should also have tests.
See OpenCensus

I'm only decorating the connect method and this will add the cursor factory, not sure if wrapt will add some "decorated" property, psycopg2 internally will instantiate PsycopgTraceCursor that just call dbapi traced_execution that is already tested, unit tests are hard in here because if the mixed Python/c execution, I believe functional tests would be way more useful and easier to implement, did't want to add them yet until #340 is complete and people are ok with that kind of approach

lzchen

LGTM save the basic unit tests.

lzchen · 2020-01-13T17:18:01Z

@hectorhdzg FYI [#360]

toumorokoshi

LGTM! Sounds good on deeper testing when the functional testing pattern is added in.

toumorokoshi · 2020-01-20T17:25:52Z

@hectorhdzg looks good! Can you resolve conflicts (looks like there was some styling stuff on the readmes that conflict with master) and then I can merge?

Adding psycopg2 integration

70dc15d

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

hectorhdzg commented Nov 19, 2019

View reviewed changes

ext/opentelemetry-ext-postgresql/tests/test_postgresql_integration.py Outdated Show resolved Hide resolved

Oberon00 reviewed Nov 20, 2019

View reviewed changes

condorcet reviewed Nov 21, 2019

View reviewed changes

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

Merge branch 'master' into hectorhdzg/postgres

4c452b4

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

hectorhdzg added the WIP Work in progress label Dec 12, 2019

toumorokoshi suggested changes Dec 30, 2019

View reviewed changes

Merge branch 'master' into hectorhdzg/postgres

74dfdf2

hectorhdzg requested a review from a team January 3, 2020 21:18

hectorhdzg added 4 commits January 3, 2020 14:04

Renaming ext

75ad5d6

WIP

a733f00

Merge with latest master

4780db6

Using dbapi ext

1b6999a

hectorhdzg added ext needs reviewers PRs with this label are ready for review and needs people to review to move forward. tracing and removed WIP Work in progress labels Jan 6, 2020

Remove pyscopg2 from pypy because is not supported

0fda7e0

lzchen reviewed Jan 7, 2020

View reviewed changes

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

lzchen reviewed Jan 7, 2020

View reviewed changes

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

lzchen reviewed Jan 7, 2020

View reviewed changes

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

Addressing comments

0ee0682

lzchen reviewed Jan 7, 2020

View reviewed changes

lzchen approved these changes Jan 7, 2020

View reviewed changes

lzchen mentioned this pull request Jan 14, 2020

Updating examples to use tracer_source #360

Merged

toumorokoshi approved these changes Jan 20, 2020

View reviewed changes

toumorokoshi removed the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Jan 20, 2020

hectorhdzg and others added 2 commits January 21, 2020 10:02

Merge branch 'master' into hectorhdzg/postgres

d487126

Use trace_api.DefaultTracer()

2dc0671

hectorhdzg added the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 21, 2020

toumorokoshi merged commit a40edf5 into open-telemetry:master Jan 22, 2020

c24t removed the PR:please merge This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 29, 2020

toumorokoshi pushed a commit to toumorokoshi/opentelemetry-python that referenced this pull request Feb 17, 2020

Adding psycopg2 integration (open-telemetry#298)

5463cef

hectorhdzg deleted the hectorhdzg/postgres branch March 24, 2020 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding psycopg2 integration #298

Adding psycopg2 integration #298

hectorhdzg commented Nov 19, 2019

Oberon00 Nov 20, 2019

hectorhdzg Nov 20, 2019 •

edited

Loading

hectorhdzg Nov 20, 2019 •

edited

Loading

toumorokoshi Dec 30, 2019

hectorhdzg Jan 6, 2020

hectorhdzg commented Dec 12, 2019

toumorokoshi left a comment

toumorokoshi Dec 30, 2019

hectorhdzg Jan 6, 2020

toumorokoshi Dec 30, 2019

hectorhdzg commented Jan 7, 2020

codecov-io commented Jan 7, 2020 •

edited

Loading

lzchen Jan 7, 2020

hectorhdzg Jan 7, 2020

lzchen left a comment

lzchen commented Jan 13, 2020

toumorokoshi left a comment

toumorokoshi commented Jan 20, 2020


		wrapt.wrap_function_wrapper(psycopg2, "connect", wrap)

		class TraceCursor(pgcursor):

Adding psycopg2 integration #298

Adding psycopg2 integration #298

Conversation

hectorhdzg commented Nov 19, 2019

Choose a reason for hiding this comment

hectorhdzg Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

hectorhdzg Nov 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorhdzg commented Dec 12, 2019

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

Choose a reason for hiding this comment

hectorhdzg commented Jan 7, 2020

codecov-io commented Jan 7, 2020 • edited Loading

Codecov Report

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

lzchen commented Jan 13, 2020

toumorokoshi left a comment

Choose a reason for hiding this comment

toumorokoshi commented Jan 20, 2020

hectorhdzg Nov 20, 2019 •

edited

Loading

hectorhdzg Nov 20, 2019 •

edited

Loading

codecov-io commented Jan 7, 2020 •

edited

Loading