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.

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

Add Django instrumentation #593

Merged
merged 59 commits into from
May 6, 2020
Merged

Conversation

ocelotl
Copy link
Contributor

@ocelotl ocelotl commented Apr 16, 2020

Fixes #592

@ocelotl ocelotl added enhancement New feature or request instrumentation Related to the instrumentation of third party libraries or frameworks labels Apr 16, 2020
@ocelotl ocelotl self-assigned this Apr 16, 2020
@ocelotl ocelotl changed the title Issue 592 Add Django instrumentation Apr 16, 2020
@ocelotl ocelotl marked this pull request as ready for review April 20, 2020 04:11
Copy link
Member

@mauriciovasquezbernal mauriciovasquezbernal left a comment

Choose a reason for hiding this comment

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

I looks very promising too. Just some small comments around.

  • An entry for the documentation is missing
  • I don't know if that's the correct place for the example, maybe we want to move that to docs/examples but I'd like to get more feedback on it. Btw, how I can run the example? A readme would be nice.
  • I think more tests should be added.

ext/opentelemetry-ext-django/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/setup.cfg Show resolved Hide resolved
ext/opentelemetry-ext-django/setup.cfg Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/tests/test_middleware.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/tests/test_middleware.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/tests/conftest.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/example/client.py Outdated Show resolved Hide resolved
@ocelotl ocelotl force-pushed the issue_592 branch 3 times, most recently from c82762b to ca85492 Compare April 23, 2020 23:34
@simkimsia
Copy link

I'm interested in using opentelemetry for my local Django app which is 2.2. how do I run the tests for this locally so I can debug to see what's failing the tests?

@ocelotl ocelotl force-pushed the issue_592 branch 3 times, most recently from 4d8ca52 to 3ddb381 Compare April 26, 2020 01:04
@ocelotl
Copy link
Contributor Author

ocelotl commented Apr 27, 2020

I'm interested in using opentelemetry for my local Django app which is 2.2. how do I run the tests for this locally so I can debug to see what's failing the tests?

@simkimsia Sorry for the late reply. To debug what is failing here you can do this:

  1. Install tox
  2. Clone this repository
  3. Check out my fork
  4. Check out this branch (issue_592)
  5. cd opentelemetry-python
  6. Run tox

Please contact me if you have any issues.

@andrewhsu
Copy link
Member

Looks like this PR already has got stuff from blocking PR #567

@codeboten codeboten added the needs reviewers PRs with this label are ready for review and needs people to review to move forward. label Apr 28, 2020
@ocelotl ocelotl force-pushed the issue_592 branch 2 times, most recently from f3a044f to 7baf328 Compare April 29, 2020 17:18
@ocelotl ocelotl force-pushed the issue_592 branch 2 times, most recently from 74491a7 to f1908dd Compare April 29, 2020 23:49
Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

I still need to run the example, but this is looking pretty good so far.

ext/opentelemetry-ext-django/example/.gitignore Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/example/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/CHANGELOG.md Show resolved Hide resolved
ext/opentelemetry-ext-django/example/README.rst Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/example/pages/views.py Outdated Show resolved Hide resolved
ext/opentelemetry-ext-django/setup.cfg Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@ocelotl ocelotl requested a review from codeboten April 30, 2020 01:15
@lzchen
Copy link
Contributor

lzchen commented May 6, 2020

Looks good. Just a couple of comments on example files.

@ocelotl
Copy link
Contributor Author

ocelotl commented May 6, 2020

@lzchen addressed all your comments, please take a look

Copy link
Contributor

@lzchen lzchen left a comment

Choose a reason for hiding this comment

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

LGTM

@toumorokoshi toumorokoshi merged commit 38fd216 into open-telemetry:master May 6, 2020
@toumorokoshi
Copy link
Member

Thanks!

codeboten added a commit to codeboten/opentelemetry-python that referenced this pull request May 8, 2020
Initial Instrumentation

Co-authored-by: Mauricio Vásquez <[email protected]>
Co-authored-by: Mathieu Hinderyckx <[email protected]>
Co-authored-by: alrex <[email protected]>
Co-authored-by: Yusuke Tsutsumi <[email protected]>
srikanthccv pushed a commit to srikanthccv/opentelemetry-python that referenced this pull request Nov 1, 2020
…pen-telemetry#593)

Transpile to es2017 to ensure compatiblity with Node.Js 8. Otherwise use of
e.g. the optional chaining operator ?. supported since typescript 3.7 results
in js not running on nodejs 8. Es2017 is the minimum to get native await support.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request instrumentation Related to the instrumentation of third party libraries or frameworks needs reviewers PRs with this label are ready for review and needs people to review to move forward.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Django instrumentation
8 participants