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

Use Django 2.0 DB instrumentation #775

Merged
merged 2 commits into from
Aug 30, 2019
Merged

Use Django 2.0 DB instrumentation #775

merged 2 commits into from
Aug 30, 2019

Conversation

tpyo
Copy link
Contributor

@tpyo tpyo commented Aug 26, 2019 •
edited
Loading

This PR continues the work @zoidbergwill started in #358

Have made a couple of small fixes to the original code:

  1. Use the middleware constructor to initialise the DB connection wrapper
  2. Catch exceptions to ensure DB spans are always completed

@tpyo tpyo requested review from c24t, reyang, songy23 and a team as code owners August 26, 2019 15:45
@zoidyzoidzoid
Copy link

Awesome work, @tpyo. Will hopefully test this a bit later this week.

reyang
reyang approved these changes Aug 27, 2019
View reviewed changes
Copy link
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

Looks good overall, I have some comments regarding the attribute/status schema.

@tpyo tpyo force-pushed the add-django2-db-instrumentation branch from e2c27a8 to f1e3a59 Compare August 28, 2019 00:43
@reyang
Copy link
Contributor

reyang commented Aug 28, 2019

Please rebase and then we're ready to merge.
Thanks for contributing to this!

@tpyo tpyo force-pushed the add-django2-db-instrumentation branch from 26bcdb5 to 44f4598 Compare August 28, 2019 17:40
@reyang reyang mentioned this pull request Aug 30, 2019
Open
@reyang reyang merged commit 2e396b0 into census-instrumentation:master Aug 30, 2019
@lzchen lzchen mentioned this pull request Sep 6, 2019
Open
@shabble shabble mentioned this pull request Jan 16, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@reyang reyang reyang approved these changes

@c24t c24t Awaiting requested review from c24t

@songy23 songy23 Awaiting requested review from songy23

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@tpyo @zoidyzoidzoid @reyang @googlebot