Skip to content
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.

Sign up for GitHub

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

Jump to bottom

psycopg2 and dbapi instrumentaiton fixes #246

Merged
merged 2 commits into from
Jan 20, 2021
Merged

psycopg2 and dbapi instrumentaiton fixes #246

merged 2 commits into from
Jan 20, 2021

Conversation

owais
Copy link
Contributor

@owais owais commented Dec 10, 2020 •
edited
Loading

Description

  • Updated psycopg2 instrumentation to work with registered types like JSONB. This makes the instrumentation compatible with psycopg2 when registering types manually. It also enables the instrumentation to work with frameworks like Django that register types implicitly.
  • Updated DBAPI to use statement and DB name as the span name as recommended by the spec instead of using the entire query.

Fixes #143

Type of change

Please delete options that are not relevant.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Updated tests and added new ones.
  • Manual testing with django and custom registration of types with psycopg2.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

@owais owais force-pushed the dbapi-rework branch 13 times, most recently from 4c39049 to d6a25cf Compare December 12, 2020 23:17
@owais owais mentioned this pull request Dec 15, 2020
Merged
12 tasks
@owais owais force-pushed the dbapi-rework branch 9 times, most recently from c3192d9 to c305a8e Compare January 8, 2021 11:41
@owais owais changed the title WIP: psycopg2 instrumentaiton fixes psycopg2 instrumentaiton fixes Jan 8, 2021
@owais owais changed the title psycopg2 instrumentaiton fixes psycopg2 and dbapi instrumentaiton fixes Jan 8, 2021
@owais owais marked this pull request as ready for review January 8, 2021 12:02
@owais owais requested a review from a team as a code owner January 8, 2021 12:02
@owais owais removed the request for review from a team January 8, 2021 12:02
@owais owais force-pushed the dbapi-rework branch 4 times, most recently from 5bf1d30 to ced0e41 Compare January 12, 2021 18:03
@owais owais force-pushed the dbapi-rework branch 3 times, most recently from 6f924fe to a354266 Compare January 19, 2021 20:24
aabmass
aabmass requested changes Jan 19, 2021
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jusit one blocking request

...tion/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py Show resolved Hide resolved
...tion/opentelemetry-instrumentation-dbapi/src/opentelemetry/instrumentation/dbapi/__init__.py Outdated Show resolved Hide resolved
...pentelemetry-instrumentation-psycopg2/src/opentelemetry/instrumentation/psycopg2/__init__.py Outdated Show resolved Hide resolved
instrumentation/opentelemetry-instrumentation-dbapi/tests/test_dbapi_integration.py Show resolved Hide resolved
...telemetry-instrumentation-aiopg/src/opentelemetry/instrumentation/aiopg/aiopg_integration.py Outdated Show resolved Hide resolved
@owais owais force-pushed the dbapi-rework branch 2 times, most recently from 29b0612 to d2295fb Compare January 20, 2021 10:23
@owais
Updated dbapi and psycopg2 instrumentations.
c14d9e4 
Changes:

- Update dbapi instrumentation to use the SQL statement name as the span
instead of the entire SQL query.
- Renamed TracedCursor with CursorTracing. The class was not a valid
Cursor so the name was confusing.
- Updated CursorTracing's (previously TracedCursor) traced_execution
method to accept the cursor instance as the first argument. This is
required as for some dbapi implementations, we need a reference to the
cursor in order to correctly format the SQL query.
- Updated psycopg2 instrumentation to leverage dbapi's `cursor_factory`
mechanism instead of wrapping the cursor with wrapt. This results in a
simpler instrumentation without monkey patching objects at runtime and
allows psycopg2's type registration system to work. This should make it
possible to use psycopg2 instrumentation when using the JSONB feature or
with frameworks like Django.
@owais
Copy link
Contributor Author

owais commented Jan 20, 2021

@aabmass please take another look when you can. Thanks.

aabmass
aabmass approved these changes Jan 20, 2021
View reviewed changes
Copy link
Member

@aabmass aabmass left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚢

@codeboten codeboten merged commit 8b9202b into open-telemetry:master Jan 20, 2021
@owais owais deleted the dbapi-rework branch January 26, 2021 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@codeboten codeboten codeboten approved these changes

@aabmass aabmass aabmass approved these changes

@ocelotl ocelotl ocelotl approved these changes

@srikanthccv srikanthccv srikanthccv approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Instrumentation breaks psycopg2 when registering types.
5 participants
@owais @codeboten @aabmass @ocelotl @srikanthccv